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