Building shape based on user input

Question

The purpose of the below code is to build a shape based on the user input, specifically:

1. Prompt the user to enter a shape - either a triangle or a square
2. Determine which "texture" to use for the shape - currently set at #'s for squares, *'s for triangles
3. Build the shape based on the selected texture

Sample output for "Square":

#####
#####
#####

Sample output for "Triangle":

*
**
***
****
*****

How can I completely redo the structure and flow of the program to better follow convention, especially in regards to the way the classes are structured? I still want use multiple classes however, as the main thing i'm trying to do here is work out how to use classes properly. Any other general refactoring welcome too.

Also sorry for the tab spacing. It's set to 2, but is a lot more when I paste it here.

# Shape Builder v0.1
class BuildShape
 def buildShapeCheck
 if @text == "square"
 self.buildSquare
 else
 self.buildTriangle
 end
 end
 def buildSquare
 (1..3).each do
 puts @texture*5
 end
 end
 def buildTriangle
 j = 1
 (1..5).each do
 puts @texture*j
 j += 1
 end
 end
end
class Textures < BuildShape
 def shapeCheck
 @texture = ""
 if @text == "square"
 @texture = "#"
 buildShapeCheck
 else
 @texture = "*"
 buildShapeCheck
 end
 end
end
class UserInput < Textures
 def initialize(text)
 @text = text
 end
 def printInput
 print "you entered: " 
 puts @text
 self.testIfValid
 end
 def testIfValid
 if @text == "square" || @text == "triangle"
 puts "#{@text} is a valid shape."
 shapeCheck
 else
 puts "#{@text} is not a valid shape, try again"
 newUserInput = UserInput.new(gets.chomp.downcase)
 newUserInput.testIfValid
 end
 end
end
puts "Enter either \"triangle\" or \"square\""
user_input = gets.chomp
newUserInput = UserInput.new(user_input.downcase)
newUserInput.printInput

Answer 1

Ok, so... there's a lot to talk about here. I've simply gone through it line by line, and added comments. So prepare for a pretty long review.

As for the indentation, the reason it's different when you paste it here is probably that you're still using tabs. It should be soft tabs, i.e. just 2 space characters. Not 1 tab character set to be 2 characters wide, but actual spaces.

Anyway, long review incoming.

# BuildShape is a poor name for a class. Classes should generally be nouns,
# but "Build shape" is an imperative. It's true that the class does build a
# shape, but the class in itself is not "the act of building".
class BuildShape
 # This class has no "initialize" method, but because its parent
 # class (which is Object, when nothing else is specified) has one,
 # I can still call "BuildShape.new.buildSquare" (for instance). But
 # if I do, things will be weird, because @text and @texture haven't
 # been defined. And without an initialize method (or attribute
 # setters), there's no way to define them.
 # Don't duplicate the name of the class in the names of its methods.
 # There's simply no need to do that; you don't need to name a file
 # on your computer after the folder it's in.
 # Also, Ruby uses underscored method names, so if anything, it should
 # be called "build_shape_check"
 def buildShapeCheck
 if @text == "square"
 self.buildSquare # no need for "self." here
 else
 self.buildTriangle # or here
 end
 end
 # This produces a 5x3 rectangle - not a square!
 # It looks like a square because of the font you use to
 # show it, but that's coincidental and may not hold true
 # in all situations. So either make the method produce a
 # real NxN square, or make it very clear in the comments
 # that "by square, I do not actually mean a square" (i.e.
 # admit the deceit and its reasons)
 def buildSquare
 (1..3).each do # you could also just use "3.times do"
 puts @texture*5
 end
 end
 # again: Underscore naming style
 def buildTriangle
 j = 1 # This is completely unnecessary...
 (1..5).each do # ...if you just add |j| as a block argument
 puts @texture*j
 j += 1 # And then this can go too
 end
 end
end
# When I said "classes should be nouns" I meant *singular* nouns.
# The String class is not called "Strings" for instance.
# If anything, this should be called "Texture"
# But more importantly, this class doesn't make sense. You use
# class inheritance because you have a generic class and want to
# make a specialization of that class. E.g. you have a class called
# "Vehicle", and you make a class called "Car" or "Boat". Those are
# specializations of a common thing: a vehicle.
# But here, you seem to be making a new class for a completely
# different reason - a reason I can't quite figure out.
class Textures < BuildShape
 # Again, no initialize method...
 # Again you're kinda-sorta repeating the name of the class
 # in the method name, because - due to inheritance - this class
 # is a BuildShape. It'd be enough to call the method "check".
 # ... buuut, this method isn't checking anything, so it'd
 # still be a terrible name.
 def shapeCheck
 # This is unnecessary; @texture *will* be set to either "#"
 # or to "*" - there's no reason to set it to anything else
 # beforehand.
 @texture = ""
 # Another thing is that this method is pointless in many ways.
 # You can't set the @texture variable anywhere else, which means
 # @texture is 100% dependent on what @text is. Which, in turn,
 # means you can just figure out the right texture when you're
 # drawing the shape. You don't even need the @texture variable;
 # the methods that print the shape are already different depending
 # on whether you want a triangle or a square; they can just print
 # the correct character themselves.
 if @text == "square"
 @texture = "#"
 # Don't call this here, *and* in the else-block: Call it *after*
 # the if-else instead, since you want to call independent of
 # what the @text is
 buildShapeCheck
 else
 @texture = "*"
 buildShapeCheck # Again: Delete this
 end
 end
end
# Ok, this is a better name of a class. UserInput sounds like a
# class name. But of course, your inheritance chain is saying that
# "UserInput is a kind of Textures, which is a kind of BuildShape"
# Do you see how that doesn't make sense?
# It also highlights why the other classes are problematic: You
# can't use them on their own. Only after 2 levels of inheritance
# do you get the class that actually solves the task. The preceding
# classes don't solve anything by themselves, you're simply
# treating them as stepping stones to the class you actually want.
# They're dependent on you extending them, which, to reuse the
# analogy from earlier, is like saying that vehicles don't work
# until someone builds a boat; it's backwards.
class UserInput < Textures
 # Yay! An initialize method!
 def initialize(text)
 @text = text
 end
 # Ok, it prints the input - but it also checks the input
 # and that's not at all obvious. Your methods should only
 # do what it says on the tin.
 def printInput
 print "you entered: " 
 puts @text
 self.testIfValid
 end
 # Uh, no. The name of this method is again pretty wrong.
 # Does it mean "it only does its test, if things are valid"?
 # The Ruby-like name for this thing would be "valid?"
 # And again, the method isn't just testing; it's also
 # the method that actually produces the output! AND it
 # completely duplicating code from *outside* the class.
 # What's worse, it creates a instance of itself, within
 # itself, and... honestly, I don't know how to best explain
 # how weird this is. Sorry, but that the case.
 def testIfValid
 if @text == "square" || @text == "triangle"
 puts "#{@text} is a valid shape."
 shapeCheck
 else
 puts "#{@text} is not a valid shape, try again"
 newUserInput = UserInput.new(gets.chomp.downcase)
 newUserInput.testIfValid
 end
 end
end
# If you just used single quotes, you wouldn't have to escape
# the inner double quotes...
puts "Enter either \"triangle\" or \"square\""
# Terrible naming: "user_input" is *not* a UserInput object;
# that's "newUserInput"! Which, again, shouldn't be CamelCased
# and just plain shouldn't be called that.
user_input = gets.chomp
newUserInput = UserInput.new(user_input.downcase)
# So... when you say printInput, you actually mean
# "do everything and print the *output*"?
newUserInput.printInput

Here's a different way of doing things:

def print_triangle(size = 5)
 size.times { |count| puts "*" * count }
end
def print_square(size = 5)
 size.times { puts "#" * size }
end
while true # loop until further notice
 puts 'Please type either "triangle" or "square"'
 type = gets.chomp.downcase
 if %w(triangle square).include?(type) # did the user enter something valid?
 send("print_#{type}") # call the proper method
 break # stop looping
 else
 puts "Sorry, that didn't make sense. Try again."
 end
end

And done. I know this doesn't use any class hierarchy or anything like that, but that's also to illustrate that it isn't always necessary. Just because you can doesn't mean you should.

• Uri Agassi: send accepts string as well
• Flambino: @UriAgassi Derp, yep, editing
• user72364: Thanks, I definitely knew everything was written really badly, just started with Ruby a day or so ago, coming from casually playing around with C in the past. Appreciate the long and detailed answer. I knew I didn't have to use classes here either, I set the challenge arbitrarily to see if I could figure out how they work. The weird parts in the code that you mentioned are there because I was having issues getting the classes to talk with each other without using inheritance, and probably forgot to delete some parts later. Anyway, cheers for the response.
• Flambino: @user72364 No prob. I can certainly appreciate the wish to use inheritance and so forth, however, I'd pick a different task to really have a chance to do so. Although Uri's answer makes good use of classes, the task is still so simple that it may not be the best case-study for object-oriented design. (cont'd)
• Flambino: @user72364 You could try something like the vehicle example I used in my answer, and make a little race where each vehicle type has a given speed. User inputs a number of minutes, and the program prints how far each vehicle got in that time. While the speed * time calculation is common for all vehicles, some have extra concerns: A plane needs to taxi before take-off, taking X minutes; a car will run out of gas after X minutes and needs to refuel (which takes some time); a ship is just slow but steady. So while they're all vehicles, they're still qualitatively different.

Answer 2

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

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.

• user72364: Thanks, I knew the classes weren't needed at all, but this is a good overview of how to use inheritance correctly, which is one the things I was trying to figure out. Cheers.