912b664b56818fd36821d006577ff3aa

Hi all,

I am looking for some critics and comments about mij code.
I used the "best of ruby quiz : quiz #2 " to create this sample.

My goal is to create clearly readable, maintainable code.
I've undergone a lot of changes since switching of to ruby.
Currently I'm trying to apply new ways i learned from reading "Practices of an Agile Developer" and "ruby design patterns" among others.

Thanks for you time :)

Gerard.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
# Ruby Quiz 2 - Scalable LCD Monitor 
# Gerard (smeevil) de Brieder - Govannon - 2008-06-12

class Lcd
  attr_accessor :line
    
  def initialize(numbers,scale)
    
    total_lines=5+((scale-1)*2) #compute the total output lines needed for scale (minimal 5)
    
    @lines=Array.new # build and reserve array spaces for the output lines
    total_lines.times do
      @lines << ""
    end
    
    @scale=scale
    numbers.scan(/./).each { |number| number_builder(get_number_sequence(number)) } 
  end

  def get_number_sequence(number) #converts a given number to a sequence used by number_builder
    
    case number
      when "0" then @sequence="24142"
      when "1" then @sequence="15151"
      when "2" then @sequence="25232"
      when "3" then @sequence="25252"
      when "4" then @sequence="14251"
      when "5" then @sequence="23252"
      when "6" then @sequence="23242"
      when "7" then @sequence="25151"
      when "8" then @sequence="24242"
      when "9" then @sequence="24252"
    end
    
    @result=''
        
    @sequence.scan(/./).each_with_index { |seq,index|
      @result+=seq
      unless index%2==0 #add extra chars for the bigger scale lcd
        (@scale-1).times{ @result+=seq }
      end
    }

    return @result    
  end
  
  def number_builder(sequence) #creates an lcd number from a given sequence and adds it to the lines array
    
    dashes=""
    spaces=""
    @scale.times do #define spaces and vertical dash size for scale
      dashes+="-"
      spaces+=" "
    end
    
    sequence.scan(/./).each_with_index do |s,i| #create the lcd number 
        case s
          when "1" then @lines[i]+=" #{spaces}  "
          when "2" then @lines[i]+=" #{dashes}  "
          when "3" then @lines[i]+="|#{spaces}  "
          when "4" then @lines[i]+="|#{spaces}| "
          when "5" then @lines[i]+=" #{spaces}| "  
        end
    end
  end
  
  def print
    @lines.each { |line| puts line unless line==''}
  end
end

def check_arguments
  
  if ARGV[0]=~/^\d+$/ 
    @scale="1"
    @number=ARGV[0]
  end

  if ARGV[0]=~/^-s$/ 
    @scale=ARGV[1]
    @number=ARGV[2]
  end

  @scale && @number && @number=~/^\d+$/ ? true : false #are all arguments accepted ?
end

########### MAIN ##############

1
2
3
4
5
6
7
8
9
10
11
12
13
if check_arguments
  @lcd=Lcd.new(@number,@scale.to_i)
  @lcd.print
else
  print "
    LCD DISPLAY
    ===========
    usage         : quiz2 -s <scale> <number>
    example       : quiz2 -s 2 12345
    -s <scale>     -s is optional
  "
end

Refactorings

No refactoring yet !

880cbab435f00197613c9cc2065b4f5a

danielharan

June 16, 2008, June 16, 2008 15:13, permalink

1 rating. Login to rate!

Can you link to information about your problem domain, or provide an explanation?

The comment for get_number_sequence isn't terribly useful - it re-iterates the what, instead of the WHY.

1
2
3
4
def get_number_sequence(number)
  @sequence = %w{ 24142 15151 25232 25252 14251 23252 23242 25151 24242 24252 }[number]
  ...
end
912b664b56818fd36821d006577ff3aa

smeevil

June 16, 2008, June 16, 2008 16:04, permalink

No rating. Login to rate!

Hi Daniel,

The "get_number_sequence" basically converts the given numbers from the command line, to a sequence that the number_builder can use to create the lcd version of that number.

Thank you for your time , it's appreciated :)

As of your request, this is the original exercise : http://www.rubyquiz.com/quiz14.html
I tried to put the text here , but the spacing of the lcd numbers did not work either here or in the code box, sorry for the jump.

1f393f50dc29119d656c91d6701ddbba

Jeremy Weiskotten

June 20, 2008, June 20, 2008 17:19, permalink

No rating. Login to rate!

Instead of case/when/then, I like to use a lookup hash.

1
2
3
4
5
6
7
8
9
10
11
12
{
  "0" => "24142",
  "1" => "15151",
  "2" => "25232",
  "3" => "25252",
  "4" => "14251",
  "5" => "23252",
  "6" => "23242",
  "7" => "25151",
  "8" => "24242",
  "9" => "24252"
}[number]

Your refactoring





Format Copy from initial code

or Cancel