Consistency
Sometimes you are using 1 space for indentation, sometimes you are using 2. Sometimes you are using whitespace around operators, sometimes you don't, sometimes you are using whitespace on one side of the operator, but not the other. Sometimes, you use space after a comma, sometimes you don't. Sometimes you use one empty line after a method, sometimes two, sometimes none.
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.
Whitespace around operators
There should be 1 space either side of an operator. You sometimes use 1 space, sometimes no space, and sometimes space only on one side.
For example here, you have the exact same expression within three lines with two different spacing styles:
i+=1
else
i +=1
They are inconsistent with each, and both of them don't comply with community guidelines. They should both be:
i += 1
Space after comma
There should be 1 space after a comma. You sometimes use 1 space, sometimes no space.
E.g. here:
@updated_comp_guess = [" "," "," "," "]
should be
@updated_comp_guess = [" ", " ", " ", " "]
Space in blocks
In a block literal, there should be a space after the opening curly brace and one before the closing curly brace:
@code = Array.new(4) { rand(1..6) }
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 'The letter "H" indicates one of the numbers you guessed is in the correct position. The letter "h" indicates you guessed a correct number but it is NOT in the correct position'
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 value != " "
@guess[idx] = value
end
should be
@guess[idx] = value if value != " "
Same here:
until @code.length == 4
@code = gets.chomp.each_char.map(&:to_i)
end
should be
@code = gets.chomp.each_char.map(&:to_i) until @code.length == 4
Unnecessary parentheses
compare_array[(compare_array.index(@guess[i]))]= " "
The parentheses around compare_array.index(@guess[i]) are unnecessary.
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 98 offenses, of which it can automatically correct 76. This leaves you with 22 offenses, of which 11 are very simple.
Here's what the result of the auto-fix looks like:
# frozen_string_literal: true
class Game
def initialize
puts '---------------------------------'
puts 'Welcome to Mastermind'
puts 'The goal is to either create a 4 digit code (Code maker role) containing numbers ranging from 1 through 6 or to guess a code (Codebreaker role) created by the computer within 12 turns to win.'
puts 'After each guess you will be given an accuracy score indicating how close you were to guessing the code correctly.'
puts 'The letter "H" indicates one of the numbers you guessed is in the correct position. The letter "h" indicates you guessed a correct number but it is NOT in the correct position'
puts '----------------------------------'
@game_over = false
@turn = 1
until @comp_guess_mode === 'Y' || @comp_guess_mode === 'N'
print 'Is the computer the code breaker? Y/N'
@comp_guess_mode = gets.chomp.upcase
end
game_mode
turn_sequence
end
def game_mode
if @comp_guess_mode == 'Y'
human_code_generator
else
code_generator
end
end
def code_generator
@code = Array.new(4) { rand(1..6) }
end
def human_code_generator
@code = ''
puts 'Please enter a 4 digit code'
@code = gets.chomp.each_char.map(&:to_i) until @code.length == 4
end
# computer_guesser method that tests if the computer's guess matches the human's
# by iterating through the array, if a direct match ('H') is found it will keep that number in the next guess
def computer_guesser
@updated_comp_guess = [' ', ' ', ' ', ' ']
if @turn == 1
@guess = Array.new(4) { rand(1..6) }
else
i = 0
while i < 4
if @guess[i] == @code[i]
@updated_comp_guess[i] = @guess[i]
i += 1
else
i += 1
end
end
end
@guess = Array.new(4) { rand(1..6) }
@updated_comp_guess.each_with_index do |value, idx|
@guess[idx] = value if value != ' '
end
puts "Guess: #{@guess.join}"
end
def codebreaker_guess
@guess = []
until @guess.length == 4
puts 'Enter your 4 digit guess'
@guess = gets.chomp.each_char.map(&:to_i)
puts "Guess: #{@guess.join}"
print "Your guess was not 4 digits long, please guess again \n" if @guess.length != 4
end
end
def turn_display
puts '-------------------------'
puts "It's turn number: #{@turn}"
end
# Repeats the following guess/check sequence for 12 turns
# or until the code and guess are matched
def turn_sequence
while @turn < 13 && @game_over == false
turn_display
if @comp_guess_mode == 'Y'
computer_guesser
else
codebreaker_guess
end
guess_checker
@turn += 1
victory_check
end
end
def guess_checker
@guess_accuracy = []
@i = 0
@h_counter = 0
while @i < 4
if @guess[@i] == @code[@i]
@guess_accuracy.push('H')
@h_counter += 1
@i += 1
else
@i += 1
end
end
if @i == 4
i = 0
compare_array = @code.clone
while i < 4
if compare_array.include?(@guess[i])
compare_array[compare_array.index(@guess[i])] = ' '
@guess_accuracy.push('h')
i += 1
else
i += 1
end
end
@guess_accuracy.pop(@h_counter)
puts "Guess accuracy: #{@guess_accuracy.join}"
end
end
def victory_check
if @guess[0..3] == @code[0..3]
puts "Code was guessed correctly, it's #{@code}, codebreaker wins"
@game_over = true
elsif @turn == 13 && @game_over == false
puts 'Code was not guessed correctly, code maker wins'
@game_over = true
end
end
end
game = Game.new
And here are the offenses that Rubocop could not automatically correct:
Offenses:
game.rb:3:1: C: Metrics/ClassLength: Class has too many lines. [116/100]
class Game ...
^^^^^^^^^^
game.rb:3:1: C: Style/Documentation: Missing top-level class documentation comment.
class Game
^^^^^
game.rb:4:3: C: Metrics/MethodLength: Method has too many lines. [14/10]
def initialize ...
^^^^^^^^^^^^^^
game.rb:7:121: C: Layout/LineLength: Line is too long. [202/120]
puts 'The goal is to either create a 4 digit code (Code maker role) containing numbers ranging from 1 through 6 or to guess a code (Codebreaker role) created by the computer within 12 turns to win.'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
game.rb:8:121: C: Layout/LineLength: Line is too long. [125/120]
puts 'After each guess you will be given an accuracy score indicating how close you were to guessing the code correctly.'
^^^^^
game.rb:9:121: C: Layout/LineLength: Line is too long. [186/120]
puts 'The letter "H" indicates one of the numbers you guessed is in the correct position. The letter "h" indicates you guessed a correct number but it is NOT in the correct position'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
game.rb:13:28: C: Style/CaseEquality: Avoid the use of the case equality operator ===.
until @comp_guess_mode === 'Y' || @comp_guess_mode === 'N'
^^^
game.rb:13:56: C: Style/CaseEquality: Avoid the use of the case equality operator ===.
until @comp_guess_mode === 'Y' || @comp_guess_mode === 'N'
^^^
game.rb:41:3: C: Metrics/AbcSize: Assignment Branch Condition size for computer_guesser is too high. [<12, 12, 11> 20.22/17]
def computer_guesser ...
^^^^^^^^^^^^^^^^^^^^
game.rb:41:3: C: Metrics/MethodLength: Method has too many lines. [19/10]
def computer_guesser ...
^^^^^^^^^^^^^^^^^^^^
game.rb:50:11: C: Style/IdenticalConditionalBranches: Move i += 1 out of the conditional.
i += 1
^^^^^^
game.rb:52:11: C: Style/IdenticalConditionalBranches: Move i += 1 out of the conditional.
i += 1
^^^^^^
game.rb:80:3: C: Metrics/MethodLength: Method has too many lines. [11/10]
def turn_sequence ...
^^^^^^^^^^^^^^^^^
game.rb:94:3: C: Metrics/AbcSize: Assignment Branch Condition size for guess_checker is too high. [<16, 13, 11> 23.37/17]
def guess_checker ...
^^^^^^^^^^^^^^^^^
game.rb:94:3: C: Metrics/MethodLength: Method has too many lines. [27/10]
def guess_checker ...
^^^^^^^^^^^^^^^^^
game.rb:102:9: C: Style/IdenticalConditionalBranches: Move @i += 1 out of the conditional.
@i += 1
^^^^^^^
game.rb:104:9: C: Style/IdenticalConditionalBranches: Move @i += 1 out of the conditional.
@i += 1
^^^^^^^
game.rb:107:5: C: Style/GuardClause: Use a guard clause (return unless @i == 4) instead of wrapping the code inside a conditional expression.
if @i == 4
^^
game.rb:114:11: C: Style/IdenticalConditionalBranches: Move i += 1 out of the conditional.
i += 1
^^^^^^
game.rb:116:11: C: Style/IdenticalConditionalBranches: Move i += 1 out of the conditional.
i += 1
^^^^^^
game.rb:117:8: W: Layout/EndAlignment: end at 117, 7 is not aligned with if at 111, 8.
end
^^^
game.rb:135:1: W: Lint/UselessAssignment: Useless assignment to variable - game.
game = Game.new
^^^^
1 file inspected, 22 offenses detected
Let's look at the simple ones first.
Case equality operator
You use the case equality operator in a couple of places. Because of the way case equality is defined for strings, your code works purely "by accident". You should use the normal equality operator instead.
This:
until @comp_guess_mode === "Y" || @comp_guess_mode === "N"
should be this:
until @comp_guess_mode == "Y" || @comp_guess_mode == "N"
Note that you use the correct equality operator for the exact same check here:
if @comp_guess_mode == "Y"
Identical expressions in all branches
There are three places where you have the same expression in both branches of a conditional expression. That is unnecessary clutter, you can just pull the expression out of the conditional:
if @guess[i] == @code[i]
@updated_comp_guess[i] = @guess[i]
i+=1
else
i +=1
end
should be
if @guess[i] == @code[i]
@updated_comp_guess[i] = @guess[i]
end
i +=1
And as we said above, a conditional with only one expression should use the modifier form (note that this transformation would again be performed automatically by Rubocop if you ran it again):
@updated_comp_guess[i] = @guess[i] if @guess[i] == @code[i]
i +=1
Unused local variable
game = Game.new
game is never used anywhere. Just remove it:
Game.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 where the improvement is not very big:
def game_mode
if @comp_guess_mode == "Y"
human_code_generator
else
code_generator
end
end
def game_mode
return human_code_generator if @comp_guess_mode == "Y"
code_generator
end
but here the gain is somewhat bigger:
def guess_checker
@guess_accuracy = []
@i=0
@h_counter = 0
while @i<4
if @guess[@i] == @code[@i]
@guess_accuracy.push("H")
@h_counter += 1
@i+=1
else
@i+=1
end
end
if @i == 4
i = 0
compare_array = @code.clone
while i < 4
if compare_array.include?(@guess[i])
compare_array[(compare_array.index(@guess[i]))]= " "
@guess_accuracy.push("h")
i+=1
else
i+=1
end
end
@guess_accuracy.pop(@h_counter)
puts "Guess accuracy: #{@guess_accuracy.join}"
end
end
def guess_checker
@guess_accuracy = []
@i = 0
@h_counter = 0
while @i < 4
if @guess[@i] == @code[@i]
@guess_accuracy.push('H')
@h_counter += 1
end
@i += 1
end
return unless @i == 4
i = 0
compare_array = @code.clone
while i < 4
if compare_array.include?(@guess[i])
compare_array[compare_array.index(@guess[i])]= ' '
@guess_accuracy.push('h')
end
i += 1
end
@guess_accuracy.pop(@h_counter)
puts "Guess accuracy: #{@guess_accuracy.join}"
end
Redundant checks
But actually, the entire thing is even simpler: since you loop and increment i 4 times, it will always be 4, so the condition will always be true and you can just remove it altogether.
Equality with booleans
@game_over == false
@game_over is already a boolean, there is no need to check for equality to false. This is just
!@game_over
Unnecessary instance variables
The instance variables @updated_comp_guess, @i, @h_counter, and @guess_accuracy are only ever used in one method. They should be local variables instead.
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
while i < 4
updated_comp_guess[i] = @guess[i] if @guess[i] == @code[i]
i += 1
end
would be much better written as
4.times do |i|
updated_comp_guess[i] = @guess[i] if @guess[i] == @code[i]
end
This will make the guess_checker method look like this:
def guess_checker
guess_accuracy = []
h_counter = 0
4.times do |i|
if @guess[i] == @code[i]
guess_accuracy.push('H')
h_counter += 1
end
end
compare_array = @code.clone
4.times do |i|
if compare_array.include?(@guess[i])
compare_array[compare_array.index(@guess[i])] = ' '
guess_accuracy.push('h')
end
end
guess_accuracy.pop(h_counter)
puts "Guess accuracy: #{guess_accuracy.join}"
end
which gives us again the opportunity to use guard clauses:
def guess_checker
guess_accuracy = []
h_counter = 0
4.times do |i|
next unless @guess[i] == @code[i]
guess_accuracy.push('H')
h_counter += 1
end
compare_array = @code.clone
4.times do |i|
next unless compare_array.include?(@guess[i])
compare_array[compare_array.index(@guess[i])] = ' '
guess_accuracy.push('h')
end
guess_accuracy.pop(h_counter)
puts "Guess accuracy: #{guess_accuracy.join}"
end
Redundant expressions
In computer_guesser, if @turn == 1, you initialize @guess, and then you initialize it again without ever using it in between. The first initialization can just be removed, turning this:
if @turn == 1
@guess = Array.new(4) { rand(1..6) }
else
4.times do |i|
updated_comp_guess[i] = @guess[i] if @guess[i] == @code[i]
end
end
@guess = Array.new(4) { rand(1..6) }
into this:
unless @turn == 1
4.times do |i|
updated_comp_guess[i] = @guess[i] if @guess[i] == @code[i]
end
end
@guess = Array.new(4) { rand(1..6) }
Code duplication
Array.new(4) { rand(1..6) }
Appears multiple times in your code. It should be extracted into a method.
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.
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 codes and 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.
Here is where the code currently stands:
# frozen_string_literal: true
class Game
def initialize
puts '---------------------------------'
puts 'Welcome to Mastermind'
puts 'The goal is to either create a 4 digit code (Code maker role) containing numbers ranging from 1 through 6 or to guess a code (Codebreaker role) created by the computer within 12 turns to win.'
puts 'After each guess you will be given an accuracy score indicating how close you were to guessing the code correctly.'
puts 'The letter "H" indicates one of the numbers you guessed is in the correct position. The letter "h" indicates you guessed a correct number but it is NOT in the correct position'
puts '----------------------------------'
@game_over = false
@turn = 1
until @comp_guess_mode == 'Y' || @comp_guess_mode == 'N'
print 'Is the computer the code breaker? Y/N'
@comp_guess_mode = gets.chomp.upcase
end
game_mode
turn_sequence
end
def game_mode
return human_code_generator if @comp_guess_mode == 'Y'
code_generator
end
def code_generator
@code = Array.new(4) { rand(1..6) }
end
def human_code_generator
@code = ''
puts 'Please enter a 4 digit code'
@code = gets.chomp.each_char.map(&:to_i) until @code.size == 4
end
# computer_guesser method that tests if the computer's guess matches the human's
# by iterating through the array, if a direct match ('H') is found it will keep that number in the next guess
def computer_guesser
updated_comp_guess = [' ', ' ', ' ', ' ']
unless @turn == 1
4.times do |i|
updated_comp_guess[i] = @guess[i] if @guess[i] == @code[i]
end
end
@guess = Array.new(4) { rand(1..6) }
updated_comp_guess.each_with_index do |value, idx|
@guess[idx] = value if value != ' '
end
puts "Guess: #{@guess.join}"
end
def codebreaker_guess
@guess = []
until @guess.size == 4
puts 'Enter your 4 digit guess'
@guess = gets.chomp.each_char.map(&:to_i)
puts "Guess: #{@guess.join}"
print "Your guess was not 4 digits long, please guess again \n" if @guess.size != 4
end
end
def turn_display
puts '-------------------------'
puts "It's turn number: #{@turn}"
end
# Repeats the following guess/check sequence for 12 turns
# or until the code and guess are matched
def turn_sequence
while @turn < 13 && !@game_over
turn_display
if @comp_guess_mode == 'Y'
computer_guesser
else
codebreaker_guess
end
guess_checker
@turn += 1
victory_check
end
end
def guess_checker
guess_accuracy = []
h_counter = 0
4.times do |i|
next unless @guess[i] == @code[i]
guess_accuracy.push('H')
h_counter += 1
end
compare_array = @code.clone
4.times do |i|
next unless compare_array.include?(@guess[i])
compare_array[compare_array.index(@guess[i])] = ' '
guess_accuracy.push('h')
end
guess_accuracy.pop(h_counter)
puts "Guess accuracy: #{guess_accuracy.join}"
end
def victory_check
if @guess == @code
puts "Code was guessed correctly, it's #{@code}, codebreaker wins"
@game_over = true
elsif @turn == 13 && !@game_over
puts 'Code was not guessed correctly, code maker wins'
@game_over = true
end
end
end
Game.new
```
