# Ruby Hangman Game

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"))
end
turn
end

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

puts "Load previously saved game? Y/N?"
end
from_json("saved_game.json")
end
end

def from_json(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
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

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
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?

• 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 – mbcode Sep 15 '20 at 4:30

# 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"))
end


should be

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


# Unnecessary parentheses around a condition

if (File.exist?("saved_game.json"))
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?


@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
turn
end

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

puts 'Load previously saved game? Y/N?'
end

def from_json(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
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: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
turn
end

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

puts 'Load previously saved game? Y/N?'
end

def from_json(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
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
$$$$
`
• 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 – mbcode Sep 15 '20 at 4:37