# Building shape based on user input

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


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.

• send accepts string as well – Uri Agassi Apr 11 '14 at 20:13
• @UriAgassi Derp, yep, editing – Flambino Apr 11 '14 at 20:57
• 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. – user72364 Apr 12 '14 at 6:51
• @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 Apr 12 '14 at 17:25
• @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. – Flambino Apr 12 '14 at 17:31

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

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


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.