Skip to main content
Code Review

Return to Answer

added 947 characters in body
Source Link
Uri Agassi
Uri Agassi
  • 6.6k
  • 1
  • 17
  • 47

Some notes about the code
This is, of course a very elaborate design for a very simple assignment, and it is not optimal (Shape assumes that all you need is @texture and @size, which might not be true to a lot of other shapes, and hold very little merit on its own - but I wanted to show some inheritance).

Note though that each method does only what it is supposed to do: create_shape just creates a shape - it does not prompt the user, and it does not print anything; draw only draws the relevant shape, etc. This makes the code more readable and maintainable.

Note also, that the class I don't want anyone to instantiate, I wrote as a module rather than a class. A module cannot be instantiated, but can be included or extended in other classes. Also, its class methods (the ones starting with self.) can be called like class methods in classes, so it is an easy way to create singleton methods.

Some notes about the code
This is, of course a very elaborate design for a very simple assignment, and it is not optimal (Shape assumes that all you need is @texture and @size, which might not be true to a lot of other shapes, and hold very little merit on its own - but I wanted to show some inheritance).

Note though that each method does only what it is supposed to do: create_shape just creates a shape - it does not prompt the user, and it does not print anything; draw only draws the relevant shape, etc. This makes the code more readable and maintainable.

Note also, that the class I don't want anyone to instantiate, I wrote as a module rather than a class. A module cannot be instantiated, but can be included or extended in other classes. Also, its class methods (the ones starting with self.) can be called like class methods in classes, so it is an easy way to create singleton methods.

Source Link
Uri Agassi
Uri Agassi
  • 6.6k
  • 1
  • 17
  • 47

You are interested in how to use classes properly, so let's talk about it.

Design your classes
Why did you choose to split the functionality as you did? Does you BuildShape have any stand alone value? Does it have any re-use value?
In ruby it is very easy to arbitrarily break functionality into different classes, because you can call methods and members which are not there, and only in runtime they are checked.
This does not mean you should arbitrarily break functionality into different classes - quite the opposite! It is the developers responsibility to design his classes well.

How should you design this?
What are the actors in this exercise?

  • We need to prompt the user to choose a shape
  • We need to draw a shape
  • We need to draw a square
  • We need to draw a triangle

So, we could break it down to 4 classes (with inheritance) - ShapeFactory, Shape, Square < Shape, Triangle < Shape.
Shape contains the functionality that is relevant to both Square and Triangle (like having texture), and the ShapeFactory is used to decide which shape to draw.

class Shape
  def initialize(texture, size)
    @texture = texture
    @size = size
  end
end

class Triangle < Shape
  def draw
    @size.times { |i| puts @texture * i }
  end
end

class Square < Shape
  def draw
    @size.times { puts @texture * @size }
  end
end

module ShapeFactory
  def self.create_shape(shape_name)
    case shape_name
    when 'square'
      Square.new('#', 5)
    when 'triangle'
      Triangle.new('*', 5)
    end
  end
end

Now all you have to do is prompt the user:

puts 'Enter either "triangle" or "square"'
shape_name = gets.chomp
print "you entered: " 
puts shape_name
until shape = ShapeFactory.create_shape(shape_name)
  puts "#{shape_name} is not a valid shape, try again"
  shape_name = gets.chomp
end
puts "#{shape_name} is a valid shape."
shape.draw