7
\$\begingroup\$

I am just learning how to code in Ruby now with no prior experience. This is 3 towers of Hanoi where the user selects where to choose from and where to go with each disc, and not to have a bigger disc on top of a smaller one. It works (I've played it), but I repeat my code A LOT! Is there a better way to write this without all the repeating but still making sense for a beginner?

tower1 = []
tower2 = []
tower3 = []

puts "How many discs would you like to start with?"
discs = gets.chomp.to_i

(1..discs).each do |disc|
  tower1 << disc
end
final = tower1.reverse
tower1.reverse!
moves = 0

while true
  puts "Here is how the game board looks right now:"
  puts "Tower 1: #{tower1}"
  puts "Tower 2: #{tower2}"
  puts "Tower 3: #{tower3}"

  puts "Please select what tower you want to chose from:"
  select = gets.chomp.to_i
  puts "Please select what tower you want to place disc:"
  destination = gets.chomp.to_i

  if select == 1 && destination == 1
    puts "You can't put the same disc where you got it from!"
    puts ""
  end
  if select == 1 && destination == 2
    if tower2.length == 0 || tower1.last < tower2.last
      tower2 << tower1.pop
      moves += 1
    else
      puts "Invalid move. You can't place a bigger tile on top of a smaller one."
      puts "Try again!"
      puts ""
    end
  end
  if select == 1 && destination == 3
    if tower3.length == 0 || tower1.last < tower3.last
      tower3 << tower1.pop
      moves += 1
    else
      puts "Invalid move. You can't place a bigger tile on top of a smaller one."
      puts "Try again!"
      puts ""
    end
  end

  if select == 2 && destination == 2
    puts "You can't put the same disc where you got it from!"
    puts ""
  end
  if select == 2 && destination == 1
    if tower1.length == 0 || tower2.last < tower1.last
      tower1 << tower2.pop
      moves += 1
    else
      puts "Invalid move. You can't place a bigger tile on top of a smaller one."
      puts "Try again!"
      puts ""
    end
  end
  if select == 2 && destination == 3
    if tower3.length == 0 || tower2.last < tower3.last
      tower3 << tower2.pop
      moves += 1
    else
      puts "Invalid move. You can't place a bigger tile on top of a smaller one."
      puts "Try again!"
      puts ""
    end
  end
  if select == 3 && destination == 3
    puts "You can't put the same disc where you got it from!"
    puts ""
  end
  if select == 3 && destination == 1
    if tower1.length == 0 || tower3.last < tower1.last
      tower1 << tower3.pop
      moves += 1
    else
      puts "Invalid move. You can't place a bigger tile on top of a smaller one."
      puts "Try again!"
      puts ""
    end
  end
  if select == 3 && destination == 2
    if tower2.length == 0 || tower3.last < tower2.last
      tower2 << tower3.pop
      moves += 1
    else
      puts "Invalid move. You can't place a bigger tile on top of a smaller one."
      puts "Try again!"
      puts ""
    end
  end
  if tower2 == final || tower3 == final
    puts "You win the game! You did it in #{moves} moves!"
    break
  end
end
\$\endgroup\$
7
\$\begingroup\$

When you represent three towers using three variables, it's hard to generalize the code. What you want is an array of three elements:

puts "How many discs would you like to start with?"
discs = gets.to_i
final = discs.downto(1).to_a
towers = [nil, final.dup, [], []]

This allows you to write a function to handle moves in general. (Note that you neglected to check whether the source tower actually contains any discs.)

def move(towers, from, to)
  if from <= 0 || towers[from].nil?
    puts "No such tower: #{from}\n"
  elsif to <= 0 || towers[to].nil?
    puts "No such tower: #{to}\n"
  elsif towers[from].empty?
    puts "Tower #{from} has no discs to move!\n"
  elsif from == to
    puts "You can't put the same disc where you got it from!\n"
  elsif !towers[to].empty? && towers[from].last > towers[to].last
    puts "Invalid move. You can't place a bigger tile on top of a smaller one."
    puts "Try again!\n"
  else
    towers[to] << towers[from].pop
    return true
  end
end

The main loop then becomes much simpler. Also, you should structure the loop with the proper termination condition rather than while true.

moves = 0
while towers[2] != final && towers[3] != final
  puts "Here is how the game board looks right now:"
  1.upto(3) { |t| puts "Tower #{t}: #{towers[t]}" }

  puts "Please select what tower you want to chose from:"
  select = gets.to_i
  puts "Please select what tower you want to place disc:"
  destination = gets.to_i

  if move(towers, select, destination)
    moves += 1
  end
end
puts "You win the game! You did it in #{moves} moves!"
\$\endgroup\$
  • \$\begingroup\$ Nice refactoring! Especially how the rule checking flows. Thanks for sharing. \$\endgroup\$ – Mohamad Dec 12 '14 at 0:10
  • 1
    \$\begingroup\$ Thanks! I really appreciate this. I'm trying to wrap my head around what works better for me at this point of my coding education: array or hash. Nice to see what I was trying to do but much cleaner \$\endgroup\$ – Jgoo83 Dec 12 '14 at 1:16
  • \$\begingroup\$ Quick question @200_success: why do you have nil to start the towers array? I know its important because I tried to run it without it and it didn't work. Can you just let me know what it does. \$\endgroup\$ – Jgoo83 Dec 12 '14 at 14:12
  • \$\begingroup\$ You want the towers to be 1, 2, 3 rather than 0, 1, 2. I prefer not to have to write ±1 all over the code, so I put a nil at 0 to shift everything over. \$\endgroup\$ – 200_success Dec 12 '14 at 15:36
6
\$\begingroup\$

If you use a hashmap you can replace your entire logic with a couple of lines. It also makes other parts of the program simpler. Here's a working example:

  select      = towers[select]
  destination = towers[destination]

  if destination.none? || select.last < destination.last
    destination << select.pop
  else
    puts "Invalid move. You can't place a bigger tile on top of a smaller one."
  end

But more on that later. First, some observations.

Simplify rule checking

Consider your logic to check if the player is putting the disk in the same tower, which is not a legal move. This is repeated in several places.

  if select == 1 && destination == 1
    puts "You can't put the same disc where you got it from!"
    puts ""
  end

No repetition necessary. It doesn't matter what the tower is. What matters is that select is equal to destination. You can also use the \n carriage return character instead of puts "" to add a blank line. Result:

puts "You can't put the same disc where you got it from!\n" if select == destination

Use idiomatic Ruby

It's more to idiomatic to use none? and any? when checking array lengths for boolean operations. So instead of this:

if tower3.length == 0

You can better express your intent with:

if tower3.none?

Switching to a hash simplifies a bunch of things

You don't have to iterate over the discs to inject them into the tower. It's also redundant to call reverse twice. You can replace the following:

(1..discs).each do |disc|
  tower1 << disc
end
final = tower1.reverse
tower1.reverse!

With this - just create the array and duplicate it:

towers[1] = (1..discs).to_a.reverse
final = towers[1].dup

The hashmap allows you to print the state of the game succinctly. Instead of this:

  puts "Tower 1: #{tower1}"
  puts "Tower 2: #{tower2}"
  puts "Tower 3: #{tower3}"

You can:

  towers.each do |tower_number, tower|
    puts "Tower #{tower_number}: #{tower}"
  end

Simplified version

Now you can see the benefit of using a hash. Here's a rough version of my refactoring. This can further be improved, but it's a very good start. I also added error checking to see if the user selects a tower that does not exist.

towers = { 1 => [], 2 => [], 3 => [] }
puts "How many discs would you like to start with?"
discs = gets.chomp.to_i

towers[1] = (1..discs).to_a.reverse
final = towers[1].dup

while true
  puts "Here is how the game board looks right now:"
  towers.each { |tower_number, tower| puts "Tower #{tower_number}: #{tower}" }

  puts "Please select what tower you want to chose from:"
  select = gets.chomp.to_i
  puts "Please select what tower you want to place disc:"
  destination = gets.chomp.to_i

  if !towers.include?(select) || !towers.include?(destination)
    puts "No such tower!\n"
    next
  elsif select == destination
    puts "You can't put the same disc where you got it from!\n"
    next
  end

  select, destination = towers[select], towers[destination]

  if destination.none? || select.last < destination.last
    destination << select.pop
  else
    puts "Invalid move. You can't place a bigger tile on top of a smaller one."
    puts "Try again!\n"
  end

  if towers[2] == final || towers[3] == final
    puts "You win the game!"
    break
  end
end

Final thoughts

My second answer shows you an object oriented approach to this game, which is arguably cleaner.

\$\endgroup\$
  • 3
    \$\begingroup\$ I like your towers Hash better than my towers Array. Note that this solution takes advantage of the Ruby feature that Hash#each enumerates elements in their insertion order. \$\endgroup\$ – 200_success Dec 12 '14 at 0:14
  • \$\begingroup\$ @200_success good point about the insertion order. I didn't think of that. That's something I tend to take for granted. \$\endgroup\$ – Mohamad Dec 12 '14 at 0:16
  • \$\begingroup\$ @Kozby you're welcome. Also be sure to checkout 200_success' refactoring. It's very neat. \$\endgroup\$ – Mohamad Dec 12 '14 at 0:31
  • \$\begingroup\$ I just looked it over. Was really nice to see the difference between the array approach i was trying with and the hash approach you suggested \$\endgroup\$ – Jgoo83 Dec 12 '14 at 1:27
5
\$\begingroup\$

I would like to add a refactoring using object-oriented design. This is not to say it's "better"; that's not the point. I want to show another way of thinking about the problem, and demonstrate Ruby's flexibility.

I've combined elements of my first answer with those of 200_success' answer. First, let's isolate the game inside its own class:

class Towers
  attr_reader :towers, :winning_tower
  attr_accessor :moves

  def initialize(discs)
    @winning_tower = discs.downto(1).to_a
    @towers = (1..3).each_with_object({}) { |n, towers| towers[n] = [] }
    @towers[1] = @winning_tower.dup
    @moves = 0
  end

  def move(from, to)
    if !towers.include?(from)
      puts "Tower #{from} does not exist."
    elsif !towers.include?(to)
      puts "Tower #{to} does not exist."
    elsif from.equal?(to)
      puts "You can't put the same disc where you got it from!"
    elsif tower(to).any? && tower(to).last < tower(from).last
      puts "Invalid move. You can't place a bigger tile on top of a smaller one."
    else
      tower(to) << tower(from).pop
      self.moves += 1
    end
  end

  def over?
    winning_tower == tower(2) || winning_tower == tower(3)
  end

  def tower(number)
    towers[number]
  end
end

Now we can run the game:

puts "How many discs would you like to start with?"
discs = gets.to_i
game = Towers.new(discs)

until game.over?
  puts "Here is how the game board looks right now:"
  game.towers.each { |number, tower| puts "Tower #{number}: #{tower}" }

  puts "Please select tower you want to chose from:"
  from = gets.chomp.to_i
  puts "Please select tower you want to place disc:"
  to = gets.chomp.to_i

  game.move(from, to)
end

puts "Game over! You've won in #{game.moves} moves!"

I have separated the game logic from the narration logic. The narration responsibility arguably does not belong in the game class. You may want to narrate in another language, for example.

\$\endgroup\$
1
\$\begingroup\$

Here is my own noobie version of Towers:

def towersofhanoi

puts 'Welcome to Towers of Hanoi'
puts 'The object of this game is to stack all the discs'
puts 'so they are in order with smallest on top'
puts ''
puts 'How many discs would you like?'
numdiscs = gets.to_i
puts ''
if (numdiscs < 3) | (numdiscs > 10)
  puts 'please enter a reasonable number of discs'
  numdiscs = gets.to_i
end
moves = -1
stack1 = Array(1..numdiscs).reverse
stack2 = Array.new
stack3 = Array.new
wincheck = stack1

until (stack2 === wincheck) | (stack3 === wincheck)
moves += 1
puts "you have made #{moves} moves"
puts "here are your stacks:"
puts ''
idx = numdiscs - 1
while idx >= 0
  printf '|'.center(10)
  printf '|'.center(10)
  printf '|'.center(10)
  puts ''
  printf stack1[idx].to_s.center(10)
  printf stack2[idx].to_s.center(10)
  printf stack3[idx].to_s.center(10)
  puts ''
  idx -= 1
end
puts ''
print 'Move a disc from stack '
movefrom = gets.to_i
print 'And put it onto stack '
moveto = gets.to_i
puts ''
if movefrom == moveto
  puts 'That was a useless move'
elsif movefrom == 1 && moveto == 2 && stack1.count != 0 
  stack2 << (stack1.pop)
elsif movefrom == 1 && moveto == 3 && stack1.count != 0
  stack3 << (stack1.pop)
elsif movefrom == 2 && moveto == 1 && stack2.count != 0
  stack1 << (stack2.pop)
elsif movefrom == 2 && moveto == 3 && stack2.count != 0
  stack3 << (stack2.pop)
elsif movefrom == 3 && moveto == 1 && stack3.count != 0
  stack1 << (stack3.pop)
elsif movefrom == 3 && moveto == 2 && stack3.count != 0
  stack2 << (stack3.pop)
else
  puts 'that is not a valid move'
  moves -= 1
end
if (stack1 != stack1.sort.reverse) | (stack2 != stack2.sort.reverse) | (stack3 != stack3.sort.reverse)
  if movefrom == 1 && moveto == 2 
    stack1 << (stack2.pop)
  elsif movefrom == 1 && moveto == 3 
    stack1 << (stack3.pop)
  elsif movefrom == 2 && moveto == 1 
    stack2 << (stack1.pop)
  elsif movefrom == 2 && moveto == 3 
    stack2 << (stack3.pop)
  elsif movefrom == 3 && moveto == 1
    stack3 << (stack1.pop)
  elsif movefrom == 3 && moveto == 2
    stack3 << (stack2.pop)
  end
  puts 'larger discs cannot be placed on smaller discs'
  moves -= 1
end
end
moves += 1
puts "YOU WIN! You solved #{numdiscs} disc difficulty"
puts ''
idx = numdiscs - 1
while idx >= 0
  printf '|'.center(10)
  printf '|'.center(10)
  printf '|'.center(10)
  puts ''
  printf stack1[idx].to_s.center(10)
  printf stack2[idx].to_s.center(10)
  printf stack3[idx].to_s.center(10)
  puts ''
  idx -= 1
end
puts ''
puts "your score is #{moves}"
puts ''
if moves == (2**numdiscs - 1)
    puts "That\'s a perfect score!"
else
puts "a perfect score is " + (2**numdiscs - 1).to_s
end
end

towersofhanoi

Write a Towers of Hanoi game. Keep three arrays, which represents the piles of discs. Pick a representation of the discs to store in the arrays; maybe just a number representing their size. In a loop, prompt the user (using gets) and ask what pile to select a disc from, and where to put it. After each move, check to see if they have succeeded in moving all the discs, to the final pile. If so, they win!"

Hash keys does seem like a much better way to solve this:

  • I like how clean your code is, but I have noticed that your game crashes if you attempt to move from an empty stack with a nil error.
  • I used the printf command in my code to show to game stacks vertically rather than horizontally
  • I also made the win condition the condition of my game loop, rather than a break out within the loop, which I think is a little nicer.
  • I like the way you count moves better than the way I implemented it (i did it as an afterthought).

You can calculate a "perfect score" using 2n - 1, where n is the number of discs.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. "Hash keys does seem like a much better way to solve this" is the only part of this answer that could be considered a "review". Could you elaborate on that advice? \$\endgroup\$ – 200_success May 10 '16 at 20:46
  • 1
    \$\begingroup\$ I apologize, I'm new. I thought that since the OP and I were likely given the same prompt that an alternate solution would count as review, since that is what the other replies also seem to be. I will add additional commentary to help with review. \$\endgroup\$ – Ace.C May 10 '16 at 21:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.