1
\$\begingroup\$

I'm still new to coding and self-taught. If anyone could take a look at my code for my Ruby Hangman game and give any tips or advice whatsoever, it would be greatly appreciated. I'm trying to improve!

The game is completely functional right now, and has a save/load feature. Repli link: https://repl.it/@MaBuCode/hangman

require 'json'

class Hangman

  def initialize
    instructions
    @miss_array = []
    random_word_picker
    word_spaces_creators
    @chance = 7
    if (File.exist?("saved_game.json"))
      game_loader
    end
    turn
  end

  def to_json
    @save_game = JSON.dump ({
      :secret_word => @secret_word,
      :spaces => @spaces,
      :chance => @chance
    })
  end

  def game_loader
    puts "Load previously saved game? Y/N?"
    until @load_response == "Y" || @load_response == "N"
      @load_response = gets.chomp.upcase
    end
    if @load_response == "Y"
      from_json("saved_game.json")
    end
  end

  def from_json(file)
    data = JSON.load File.new(file)
    @secret_word = data['secret_word']
    @spaces = data['spaces']
    @chance = data['chance']
  end

  def save_game(file)
    game_file = File.new("saved_game.json", "w")
    game_file.write(file)
    game_file.close
  end

  def random_word_picker
    @secret_word = File.readlines("5desk.txt").sample.split(/[\r\n]+/).join.split("")
  end

  def word_spaces_creators
    @spaces = []
    i = 0
    until i == @secret_word.length
      @spaces.push("_ ")
      i += 1
    end
  end

  def letter_guess
    @letter_guess = ""
    puts "Enter a single letter guess or type \"save\"to save"
    @letter_guess = gets.downcase.chomp
    if @letter_guess == "save"
      to_json
      save_game(@save_game)
      exit
    end
    until (@letter_guess.is_a? String) && 
      (@letter_guess.length == 1) && (('a'..'z').include?(@letter_guess))
      puts "Enter a single letter"
      @letter_guess = gets.downcase.chomp
    end
  end

  def guess_checker(guess)
    @first_confirm = true
    @secret_word.each_with_index do |letter, index|
      if letter == guess || letter == guess.upcase
        @spaces[index] = "#{letter} "
        if @first_confirm == true
          puts "Nice! You guessed a letter correctly"
          @first_confirm = false
        end
      end
    end
    if @first_confirm == true
      chances
      @miss_array.push("#{guess} ")
      puts "Missed letters: #{@miss_array.join}"
    end
 end

  def victory_check
    if @spaces.include?("_ ") == false
      print @spaces.join
      puts ""
      puts "Congratulations, you've guessed the word!"
    elsif 
      @chance == 0
      puts "Game Over! You didn't guess the word, it's #{@secret_word.join}"      
    else
      puts "----------------------"
      turn
    end
  end

  def turn
    puts "#{@chance} chance(s) left"
    puts @spaces.join
    letter_guess
    guess_checker(@letter_guess)
    victory_check
  end

  def chances
    puts "Miss!"
    @chance -= 1
  end

  def instructions
    puts "Welcome to Hangman. At the beginning of each game a randomly selected word will be chosen," 
    puts "you'll be allowed to guess letters until you miss 7 times. If you solve the word correctly you win."
    puts "At the beginning you will be given the option to load a previously saved game and before each guess"
    puts "you will be presented with an opportunity to save your game"
    puts "---------"
  end

end

newgame = Hangman.new


edit: Fixed a formatting error

\$\endgroup\$
2
\$\begingroup\$

As you say, the game is fully functional. But from a design perspective, it could use a little work.

  1. My number one issue is that the whole game is all in one class, and the whole game is run through the initialize method, which only finishes at the end of the game
  2. The turn method calls victory_check, which then recursively calls turn until the end of the game is reached
  3. Each method call requires Ruby to put its current context on the stack so that it can be restored when the method returns. But your methods don't return; they just call another method. Because the game has a finite length (words can only be so long, you only get seven guesses) this doesn't cause a problem. But if you were processing a file of a million records, or processing a stream of thousands of tweets, eventually the stack would fill up and your program would fail.
  4. The class has way too many responsibilities; generating the random word, printing the instructions, maintaining the game state, loading and saving, and interacting with the user. It makes it hard to test the individual parts of it in isolation.

The idea of objects is that they represent some real-world items and encapsulates their state and provide methods to allow you to interact with them. Let's break it down a bit. Here's a sample class that just looks after the game state. It doesn't do much apart from keeping track of the secret word, the number of guesses taken and maintains a printable string showing which letters you've guessed. It also has some convenience methods that allow you to see if you've been hanged or not.

It doesn't interact with the user, generate the random word, or print the instructions, as these are presentation issues that can be dealt with by another program.

class Hangword
  attr_reader :guesses
  def initialize(word)
    @word = word.downcase.chars
    @display = @word.map { '_' }
    @charmap = Hash.new { |h, k| h[k] = [] }
    @word.each_with_index { |char, index| @charmap[char].append index }
    @guesses = 7
  end

  def guess(char)
    raise 'Hanged' if hanged?

    if @charmap.include? char
      @charmap[char].each { |i| @display[i] = char }
    else
      @guesses -= 1
    end
  end

  def secret
    @word.join
  end

  def display
    @display.join
  end

  def hanged?
    @guesses.zero?
  end

  def reprieved?
    !hanged? && !@display.include?('_')
  end
end

You can create a new Hangword and then interact with it:

a = Hangword.new 'puzzle'
a.guess('z')
puts a.display
7.times { a.guess 'x' }
puts a.guesses
puts a.hanged?
puts a.reprieved?

b = Hangword.new 'puzzle'
'puzle'.chars.each { |i| b.guess i }
puts a.reprieved?
\$\endgroup\$
1
  • \$\begingroup\$ Appreciate the feedback! You explanation made a lot of sense to me and I THINK it conceptually helped me understand the fundamental issues I had in my code. I took your advice and rewrote the code from the beginning, I used part of the sample class you used above as a template for one of my classes. If you're interested in taking a quick look to see if it looks like I've gotten the hang of things it would mean a lot, cheers! repl.it/@MaBuCode/hangmanrefactored \$\endgroup\$ – mbcode Sep 15 '20 at 4:30
1
\$\begingroup\$

Consistency

Sometimes you are using 1 space for indentation, sometimes you are using 2. Sometimes you are using parentheses around the arguments of a message send, sometimes you don't. Sometimes you are using parentheses around the condition of a conditional, sometimes you don't. Sometimes you are using new-style hash syntax, sometimes you are using old-style.

You should choose one style and stick with it. If you are editing some existing code, you should adapt your style to be the same as the existing code. If you are part of a team, you should adapt your style to match the rest of the team.

Most communities have developed standardized community style guides. In Ruby, there are multiple such style guides. They all agree on the basics (e.g. indentation is 2 spaces), but they might disagree on more specific points (single quotes or double quotes).

Indentation

The standard indentation style in Ruby is two spaces. You mostly use 2 spaces, but there is one place where you use 1 space. Stick with two.

Single-quoted strings

If you don't use string interpolation, it is helpful if you use single quotes for your strings. That way, it is immediately obvious that no string interpolation is taking place.

In particular, this would also remove the escaping you need to do here:

puts 'Enter a single letter guess or type "save"to save'

Note that it is perfectly fine use double quoted strings if you otherwise needed to use escapes, e.g. here:

puts "Congratulations, you've guessed the word!"

Frozen string literals

Immutable data structures and purely functional code are always preferred, unless mutability and side-effects are required for clarity or performance. In Ruby, strings are always mutable, but there is a magic comment you can add to your files (also available as a command-line option for the Ruby engine), which will automatically make all literal strings immutable:

# frozen_string_literal: true

It is generally preferred to add this comment to all your files.

Conditional modifiers

When you have a conditional that executes only one expression, you should use the modifier form instead, e.g. this:

if (File.exist?("saved_game.json"))
  game_loader
end

should be

game_loader if File.exist?('saved_game.json')

Unnecessary parentheses around a condition

if (File.exist?("saved_game.json"))
  game_loader
end

The parentheses around File.exist?("saved_game.json") are unnecessary.

Unnecessary parentheses around a message send

(('a'..'z').include?(@letter_guess))

The parentheses around ('a'..'z').include?(@letter_guess) are unnecessary.

No whitespace between message and argument list

JSON.dump ({
  :secret_word => @secret_word,
  :spaces => @spaces,
  :chance => @chance
})

You have whitespace between the message and the argument list. This means that the parentheses will not be parsed as the parentheses around an argument list, but they will be parsed as the grouping operator. With a single argument, that doesn't make a difference, but if you look at this case:

foo (1, 2)

This is interpreted as sending the message foo with a single argument (1, 2) which is not a legal expression, and therefore an error.

The correct way is to not use whitespace between the message and a parenthesized argument list:

JSON.dump({
  :secret_word => @secret_word,
  :spaces => @spaces,
  :chance => @chance
})

New-style hash syntax

In 2007, an alternative syntax for hash literals with Symbol keys was added to Ruby. Instead of :symbol => value, you can now write symbol: value. It is generally preferred to use the new-style hash syntax for hashes that only contain Symbol keys:

JSON.dump({
  secret_word: @secret_word,
  spaces: @spaces,
  chance: @chance
})

Numeric predicates

@chance.zero?

reads more fluently than

@chance == 0

to_json argument

It is unfortunately not well-documented, but to_json should take an argument. You can ignore the argument for now, but you should add a splat parameter to your parameter list:

def to_json(*)

Linting

You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.

Rubocop was able to detect all of the style violations I pointed out, and also was able to autocorrect all of them.

Let me repeat that: I have just spent two pages pointing out how to correct tons of stuff that you can actually correct within milliseconds at the push of a button. I have set up my editor such that it automatically runs Rubocop with auto-fix as soon as I hit "save".

In particular, running Rubocop on your code, it detects 82 offenses, of which it can automatically correct 75. This leaves you with 7 offenses, of which 3 are very simple.

Here's what the result of the auto-fix looks like:

# frozen_string_literal: true

require 'json'

class Hangman
  def initialize
    instructions
    @miss_array = []
    random_word_picker
    word_spaces_creators
    @chance = 7
    game_loader if File.exist?('saved_game.json')
    turn
  end

  def to_json(*_args)
    @save_game = JSON.dump({
                             secret_word: @secret_word,
                             spaces: @spaces,
                             chance: @chance
                           })
  end

  def game_loader
    puts 'Load previously saved game? Y/N?'
    @load_response = gets.chomp.upcase until @load_response == 'Y' || @load_response == 'N'
    from_json('saved_game.json') if @load_response == 'Y'
  end

  def from_json(file)
    data = JSON.load File.new(file)
    @secret_word = data['secret_word']
    @spaces = data['spaces']
    @chance = data['chance']
  end

  def save_game(file)
    game_file = File.new('saved_game.json', 'w')
    game_file.write(file)
    game_file.close
  end

  def random_word_picker
    @secret_word = File.readlines('5desk.txt').sample.split(/[\r\n]+/).join.split('')
  end

  def word_spaces_creators
    @spaces = []
    i = 0
    until i == @secret_word.length
      @spaces.push('_ ')
      i += 1
    end
  end

  def letter_guess
    @letter_guess = ''
    puts 'Enter a single letter guess or type "save"to save'
    @letter_guess = gets.downcase.chomp
    if @letter_guess == 'save'
      to_json
      save_game(@save_game)
      exit
    end
    until (@letter_guess.is_a? String) &&
          (@letter_guess.length == 1) && ('a'..'z').include?(@letter_guess)
      puts 'Enter a single letter'
      @letter_guess = gets.downcase.chomp
    end
  end

  def guess_checker(guess)
    @first_confirm = true
    @secret_word.each_with_index do |letter, index|
      next unless letter == guess || letter == guess.upcase

      @spaces[index] = "#{letter} "
      if @first_confirm == true
        puts 'Nice! You guessed a letter correctly'
        @first_confirm = false
      end
    end
    if @first_confirm == true
      chances
      @miss_array.push("#{guess} ")
      puts "Missed letters: #{@miss_array.join}"
    end
  end

  def victory_check
    if @spaces.include?('_ ') == false
      print @spaces.join
      puts ''
      puts "Congratulations, you've guessed the word!"
    elsif @chance.zero?
      puts "Game Over! You didn't guess the word, it's #{@secret_word.join}"
    else
      puts '----------------------'
      turn
    end
  end

  def turn
    puts "#{@chance} chance(s) left"
    puts @spaces.join
    letter_guess
    guess_checker(@letter_guess)
    victory_check
  end

  def chances
    puts 'Miss!'
    @chance -= 1
  end

  def instructions
    puts 'Welcome to Hangman. At the beginning of each game a randomly selected word will be chosen,'
    puts "you'll be allowed to guess letters until you miss 7 times. If you solve the word correctly you win."
    puts 'At the beginning you will be given the option to load a previously saved game and before each guess'
    puts 'you will be presented with an opportunity to save your game'
    puts '---------'
  end
end

newgame = Hangman.new

And here are the offenses that Rubocop could not automatically correct:

Offenses:

hangman.rb:5:1: C: Metrics/ClassLength: Class has too many lines. [104/100]
class Hangman ...
^^^^^^^^^^^^^
hangman.rb:5:1: C: Style/Documentation: Missing top-level class documentation comment.
class Hangman
^^^^^
hangman.rb:31:17: C: Security/JSONLoad: Prefer JSON.parse over JSON.load.
    data = JSON.load File.new(file)
                ^^^^
hangman.rb:56:3: C: Metrics/MethodLength: Method has too many lines. [13/10]
  def letter_guess ...
  ^^^^^^^^^^^^^^^^
hangman.rb:72:3: C: Metrics/MethodLength: Method has too many lines. [14/10]
  def guess_checker(guess) ...
  ^^^^^^^^^^^^^^^^^^^^^^^^
hangman.rb:83:5: C: Style/GuardClause: Use a guard clause (return unless @first_confirm == true) instead of wrapping the code inside a conditional expression.
    if @first_confirm == true
    ^^
hangman.rb:125:1: W: Lint/UselessAssignment: Useless assignment to variable - newgame.
newgame = Hangman.new
^^^^^^^

1 file inspected, 7 offenses detected, 1 offense auto-correctable

Let's look at the simple ones first.

Unused local variable

newgame = Hangman.new

newgame is never used anywhere. Just remove it:

Hangman.new

Guard clauses

If you have a case where an entire method or block is wrapped in a conditional, you can replace that with a "guard clause" and reduce the level of nesting.

E.g. this:

def something
  if foo
    bar
    baz
    quux
  else
    42
  end
end

can become this:

def something
  return 42 unless foo

  bar
  baz
  quux
end

There are a couple of opportunities to do this in your code, and a couple more are created by following the Rubocop advice.

Here is one example:

if @first_confirm == true
  chances
  @miss_array.push("#{guess} ")
  puts "Missed letters: #{@miss_array.join}"
end
return unless @first_confirm == true

chances
@miss_array.push("#{guess} ")
puts "Missed letters: #{@miss_array.join}"

Parentheses around argument list

In general, you should use the message sending form with parentheses around the argument list, so this:

@letter_guess.is_a? String

should be

@letter_guess.is_a?(String)

We only use the whitespace form for methods of Kernel that approximate global procedures, e.g. puts, require, or methods that approximate "language extensions" such as attr_reader.

Redundant checks

Here, you are checking whether the object referenced by the @letter_guess instance variable is an instance of the String class:

@letter_guess.is_a? String

But, this comes from user input on the terminal, so it will always be a String. That is literally what gets means: get a string.

You can just delete this check, since it will always be true.

Equality with booleans

if @first_confirm == true

@first_confirm is already a boolean, there is no need to check for equality to true. This is just

if @first_confirm

Same here:

if @spaces.include?('_ ') == false

should just be

if !@spaces.include?('_ ')

Unnecessary instance variables

The instance variables @load_response, and @first_confirm are only ever used in one method. They should be local variables instead.

Prefer the block form of File methods

Several methods of File and IO take blocks as arguments, and automatically make sure to close the file handle at the end of the block. For example, this:

game_file = File.new('saved_game.json', 'w')
game_file.write(file)
game_file.close

could leak a file handle if some exception gets raised during the write. If you use the block form of File::open instead, the method will take care to ensure the file handle is always closed:

File.open('saved_game.json', 'w') do |game_file|
  game_file.write(file)
end

For example, in your json_load method, you create a file handle but never close it!

Redundant expression

In letter_guess, you assign the empty string to @letter_guess:

@letter_guess = ""

but then you immediately re-assign it without ever using it in between:

@letter_guess = gets.downcase.chomp

The first assignment is useless, just remove it.

Loops

In Ruby, you almost never need loops. In fact, I would go so far and say that if you are using a loop in Ruby, you are doing it wrong.

Here's an example:

i = 0
until i == @secret_word.length
  @spaces.push('_ ')
  i += 1
end

would be much better written as

@secret_word.length.times do
  @spaces.push('_ ')
end

Array initialization

But actually, the above could much better be written using the Array::new method with a block argument:

@spaces = Array.new(@secret_word.size) { '_ ' }

And actually, since you never mutate an element of the array, and we are using frozen string literals anyway, it is safe to just do

@spaces = Array.new(@secret_word.size, '_ ')

length vs. size

Many Ruby collections have both length and size methods, but some have only one. In general, IFF a collection has a size method, then that method is guaranteed to be "efficient" (usually constant time), whereas length may or may not be efficient (linear time for iterating through the collection and counting all the elements), depending on the collection.

In your case, you are using arrays and strings, for which both are constant time, but if you want to guarantee efficiency, then it is better to explicitly use size instead.

Print empty lines

It is more idiomatic to just use

puts

to print an empty line instead of

puts ''

String#chars over String#split

If you want to get the individual characters of a String, there is the String#chars method for that. No need to use String#split with an empty String as the delimiter.

Array#include? over multiple comparisons

If you have a pattern like this:

load_response == 'Y' || load_response == 'N'

It is more idiomatic to use

%w[Y N].include?(load_response)

The Elephant in the room

One thing I have not addressed so far, and that I unfortunately do not have to time to address, is the fundamental design of the code. Everything I mentioned so far is just cosmetics.

All work is done in the initializer. All an initializer should do is initialize the object. It shouldn't ask for user input, it shouldn't print anything, it shouldn't play a game.

Also, you are mixing I/O and logic everywhere. A method should either print something or do something. Your design makes it impossible to test the code without actually playing the game. I cannot prepare a file with guesses and feed it to a test runner, I actually have to manually play the game.

It is also strange that you have only one "object", namely the game, which is doing something. If you think about how the game is typically played, aren't the objects that are actively doing something the players and not the game? Where are the players in your design?

Unfortunately, I do not have time to dive into this.

The flow of your code is extremely convoluted. For example, I cannot figure out whether this check is redundant:

letter == guess || letter == guess.upcase

letter and guess come from different places, they are carried around as hidden state, and it is hard to figure out whether letter even can be upper case at all! And if it can, then it would be much easier to just check like this:

letter.upcase == guess.upcase

Here is another example:

File.readlines('5desk.txt').sample.split(/[\r\n]+/)

I cannot even figure out what this is supposed to do, so it should probably be extracted into a separate method with a descriptive intention-revealing name and proper documentation. readlines will return an array of lines, sample will randomly return one of those lines, so I am not sure what the split is for. You only have one line, so what are you splitting here?

Here is where the code currently stands:

# frozen_string_literal: true

require 'json'

class Hangman
  def initialize
    instructions
    @miss_array = []
    random_word_picker
    word_spaces_creators
    @chance = 7
    game_loader if File.exist?('saved_game.json')
    turn
  end

  def to_json(*)
    @save_game = JSON.dump({
                             secret_word: @secret_word,
                             spaces: @spaces,
                             chance: @chance
                           })
  end

  def game_loader
    puts 'Load previously saved game? Y/N?'
    load_response = gets.chomp.upcase until %w[Y N].include?(load_response)
    from_json('saved_game.json') if load_response == 'Y'
  end

  def from_json(file)
    data = JSON.parse(File.read(file))
    @secret_word = data['secret_word']
    @spaces = data['spaces']
    @chance = data['chance']
  end

  def save_game(file)
    File.open('saved_game.json', 'w') do |game_file|
      game_file.write(file)
    end
  end

  def random_word_picker
    @secret_word = File.readlines('5desk.txt').sample.split(/[\r\n]+/).join.chars
  end

  def word_spaces_creators
    @spaces = Array.new(@secret_word.size, '_ ')
  end

  def letter_guess
    puts 'Enter a single letter guess or type "save" to save'
    @letter_guess = gets.downcase.chomp
    if @letter_guess == 'save'
      to_json
      save_game(@save_game)
      exit
    end
    until (@letter_guess.size == 1) && ('a'..'z').include?(@letter_guess)
      puts 'Enter a single letter'
      @letter_guess = gets.downcase.chomp
    end
  end

  def guess_checker(guess)
    first_confirm = true
    @secret_word.each_with_index do |letter, index|
      next unless letter.upcase == guess.upcase

      @spaces[index] = "#{letter} "

      next unless first_confirm

      puts 'Nice! You guessed a letter correctly'
      first_confirm = false
    end

    return unless first_confirm

    chances
    @miss_array.push("#{guess} ")
    puts "Missed letters: #{@miss_array.join}"
  end

  def victory_check
    if !@spaces.include?('_ ')
      print @spaces.join
      puts
      puts "Congratulations, you've guessed the word!"
    elsif @chance.zero?
      puts "Game Over! You didn't guess the word, it's #{@secret_word.join}"
    else
      puts '----------------------'
      turn
    end
  end

  def turn
    puts "#{@chance} chance(s) left"
    puts @spaces.join
    letter_guess
    guess_checker(@letter_guess)
    victory_check
  end

  def chances
    puts 'Miss!'
    @chance -= 1
  end

  def instructions
    puts 'Welcome to Hangman. At the beginning of each game a randomly selected word will be chosen,'
    puts "you'll be allowed to guess letters until you miss 7 times. If you solve the word correctly you win."
    puts 'At the beginning you will be given the option to load a previously saved game and before each guess'
    puts 'you will be presented with an opportunity to save your game'
    puts '---------'
  end
end

Hangman.new
```
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the extensive feedback. I rewrote my code addressing all stylistic issues you identified, ran Rubocop, and I think I fixed the fundamental initialize problem I had in my code. If you're interested in taking a look to see if it seems improved, I'd greatly appreciate it. repl.it/@MaBuCode/hangmanrefactored \$\endgroup\$ – mbcode Sep 15 '20 at 4:37

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.