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.

5170ca260dbd2cdfd5a887a4dba7636f

Jeremy

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