I guess the major reason is that they are not familiar with Ruby, and a minor reason, they have never been critical to code as much as I am. For them, the only standard of good code is to pass the compiler and get built up. They seldom ask questions like: why you name this variable like this? or I don't understand this part, can u make it simpler?
On the other hand, as a automation tester, I have to work with testers who do not code much. I do not need 10 depth loop, or a super complex design pattern. For every single line, I just need it to be simple, clean, clear, and easy to understand. That's why I start coding, and re-factoring.
Since I haven't got any comments, I decide to try a code review myself. According to the best practice of writing, put your work away 6 weeks after you've done it, then pick it up as a stranger, you'll find as many problems as a stranger could. I guess 2 weeks should work for this 200 lines of code.
So here it goes:
- the first question I ask : does init have to do so many things?
def initialize(number, size) @number = number @size = size @width = @size + 2 @heighth = @size*2 + 3 init_picture init_points init_sticks end
It would better to make init simple. So I move init_picture, init_sticks, and init_points to method paint_sticks, which makes paint_sticks like this:
def paint_sticks()
init_picture
init_sticks
init_points
if [0,2,3,5,6,7,8,9].include? @number
@top_stick.paint_in_picture(@picture)
end
if [2,3,4,5,6,8,9].include? @number
@middle_stick.paint_in_picture(@picture)
end
if [0,2,3,5,6,8,9].include? @number
@bottom_stick.paint_in_picture(@picture)
end
if [0,4,5,6,8,9].include? @number
@upper_left_stick.paint_in_picture(@picture)
end
if [1,2,3,4,7,8,9,0].include? @number
@upper_right_stick.paint_in_picture(@picture)
end
if [2,6,8,0].include? @number
@lower_left_stick.paint_in_picture(@picture)
end
if [1,3,4,5,6,7,8,9,0].include? @number
@lower_right_stick.paint_in_picture @picture
end
end
- then I ask: can I switch the sequence of these 3 function calls
def paint_sticks init_picture init_points init_sticks
It looks like I can. But I can't. Because init_sticks depends on init_points. So I move init_points into method init_sticks. Leaving init_picture and init_sticks there, of course they can be switched. This makes paint_sticks become: init_picture, init_sticks, following by some messy code. A better way to express it is :
def paint init_picture init_sticks paint_sticks_in_picture end
- Then I ask: what if I do a=NumberPicture.new;a.paint();a.paint();a.paint()? All calculation and drawing are done in the first paint, once pained, nothing could be changed. The following paint call doesn't have to do anything. So I update paint like this:
def paint
if !@painted
init_picture
init_sticks
paint_sticks_in_picture
painted = true
end
end
Here comes an interesting finding. For the method paint, even if you call it once in unit testing, you will get 100% code coverage. But you are not done, you need to call it again to test "else do_nothing" part. The challenging part is how to test this part. Well, I don't know. Please leave a comments if you do.
- after review, the code looks like this:
require 'pp' require 'point' require 'stick'
class NumberPicture
EMPTY = " "
attr_reader :width, :heighth, :size, :painted
attr_accessor :picture
def initialize(number, size)
if size.is_a?(Integer) and size>0
@size = size
else
raise "size #{size} must be integer and >0"
end
@number = number
@width = @size + 2
@heighth = @size*2 + 3
@painted = false
end
def paint()
if !@painted
# TODO: how to unit test this?
init_picture
init_sticks
paint_sticks_in_picture
@painted = true
end
end
private
def init_picture()
@picture = Array.new()
(0..@heighth-1).each do |i|
@picture[i] = Array.new(@width-1, 0)
(0..@width-1).each do |j|
@picture[i][j] = EMPTY
end
end
end
def init_points()
@top_left = Point.new(0, 0)
@top_right = Point.new(0, @width-1)
@middle_left = Point.new(@size+1, 0)
@middle_right = Point.new(@size+1, @width-1)
@bottom_left = Point.new(@heighth-1, 0)
end
def init_sticks()
init_points
@top_stick = HorizontalStick.new(@top_left, @size)
@middle_stick = HorizontalStick.new(@middle_left, @size)
@bottom_stick = HorizontalStick.new(@bottom_left, @size)
@upper_left_stick = VerticalStick.new(@top_left, @size)
@upper_right_stick = VerticalStick.new(@top_right, @size)
@lower_left_stick = VerticalStick.new(@middle_left, @size)
@lower_right_stick = VerticalStick.new(@middle_right, @size)
end
def paint_sticks_in_picture
# TODO: eliminate process redundant
if [0,2,3,5,6,7,8,9].include? @number
@top_stick.paint_in_picture(@picture)
end
if [2,3,4,5,6,8,9].include? @number
@middle_stick.paint_in_picture(@picture)
end
if [0,2,3,5,6,8,9].include? @number
@bottom_stick.paint_in_picture(@picture)
end
if [0,4,5,6,8,9].include? @number
@upper_left_stick.paint_in_picture(@picture)
end
if [1,2,3,4,7,8,9,0].include? @number
@upper_right_stick.paint_in_picture(@picture)
end
if [2,6,8,0].include? @number
@lower_left_stick.paint_in_picture(@picture)
end
if [1,3,4,5,6,7,8,9,0].include? @number
@lower_right_stick.paint_in_picture @picture
end
end
end
As a summary of this post, I ask questions including:
- does init have to do so many things?
- these function calls does not show clear dependency, can I change the sequence?
- what if I call this method again?
- this method looks like do_A, do_B, then messy, can I make it, do_A, do_B, do_with_A&B?
- how could this part be unit tested?
And the code does look better than I find it. :-)
1 条评论:
To a user, it's expected that the "print" can be done as many times as wanted. But here, he will have no idea what happen once do print for the second time.
发表评论