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.

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.