https://youtu.be/xDL5GrYnN_g

This is the first in what I intend to be a series of refactoring readers’ coding submissions.

To make your own Hangman Challenge submission, you can go here and follow the instructions.

“Before” version

What was good about the code

I found the class name Word to be good. It was very obvious to me what idea Word represented.

Most of the code was reasonably easy to understand. Aside from one certain line, there were no places where I was at a total loss as to what was going on.

What could be improved

Before I list what could be improved I want to thank the person who bravely submitted his code for public critique.

One of the first things that drew my attention was the Hangman#guess method. This method was pretty long and contained deep nesting.

The name of the Hangman class also lacks meaning. Word is a good abstraction because it represents a crisp idea: the word that the player is trying to guess. But there’s no such thing as a Hangman, at least not in this context, and so an opportunity for a crisp abstraction is lost.

There are some naming issues. The variables secret, letters, guesses and life aren’t as clear as they could be.

There are a couple places where there’s a lack of obvious meaning. For example, what exactly does it mean when life.zero? is true? It can be inferred that this means the player loses, but it would be better if we were to make this blatantly obvious.

There are a couple YAGNI (you ain’t gonna need it) violations. Both the Word constructor and the Hangman constructor have a superfluous feature which allows you to pass in an arbitrary masked character value and starting life value, respectively. These features aren’t needed in order to meet the requirements of the program. These features were added, presumably, “just in case”.

There are a few cases of needless attr_readers. It’s my view that attr_reader should only be used if an instance variable needs to be surfaced to a class’s public API. Otherwise the public API is being made bigger than necessary, limiting how much of the class can be refactored without risk.

This program contains a type of Ruby syntax I’ve never seen before:

secret.each_char.with_index { |char, i| @letters[i], guessed = char, true if char == letter }

Perhaps there are some cases where this type of syntax works well, but in this particular case I found it confounding.

Lastly, there’s was a bit of duplication, one “magic number”, and what you might call a “magic character”.

Here’s the original code with comments added by me.

#!/usr/bin/ruby
#
# https://github.com/jasonswett/hangman_challenge
#

# frozen_string_literal: true

class Word # good abstraction
  attr_reader :secret # needless attr_reader

  def initialize(secret, char = "_") # YAGNI violation
    @secret = secret # unclear variable name
    @letters = Array.new(secret.length) { char } # unclear variable name
  end

  def guess(letter)
    guessed = false

    # confusing syntax
    secret.each_char.with_index { |char, i| @letters[i], guessed = char, true if char == letter }

    guessed
  end

  def completed? # unclear method name
    secret == mask
  end

  def mask
    @letters.join
  end
end

class Hangman # "Hangman" isn't really an abstraction
  attr_reader :word, :guesses, :life # needless attr_readers

  def initialize(secret, life = 6) # YAGNI violation
    @word = Word.new(secret)
    @guesses = "" # unclear variable name
    @life = life # unclear variable name

    puts "#{word.mask} life left: #{life}"
  end

  def guess(letter) # long method with deep nesting
    if word.guess(letter)
      if word.completed?
        puts "#{word.secret} YOU WIN!"
      else
        if guesses.empty?
          puts "#{word.mask} life left: #{life}" # duplication
        else
          puts "#{word.mask} life left: #{life} incorrect guesses: #{guesses}"
        end
      end
    else
      @life -= 1

      if life.zero? # lack of obvious meaning
        puts "#{word.mask} YOU LOSE!"
      else
        @guesses += letter
        puts "#{word.mask} life left: #{life} incorrect guesses: #{guesses}"
      end
    end
  end
end

# hangman = Hangman.new("apple")
# %w[a b q z e p l].each do |letter|
#   hangman.guess(letter)
# end

# hangman = Hangman.new("quixotic")
# %w[a e i o u l g p r].each do |letter|
#   hangman.guess(letter)
# end

File.readlines(ARGV[0], chomp: true).each do |line|
  next if line.empty?

  if line.length == 1
    @hangman.guess(line)
  else
    @hangman = Hangman.new(line)
  end
end

“After” version

Here’s my version. I’m not saying it’s perfect, just an improvement. I invite you to see if you can identify the specific ways in which the code was improved.

#!/usr/bin/ruby
#
# https://github.com/jasonswett/hangman_challenge
#

# frozen_string_literal: true

class Game
  STARTING_LIFE_AMOUNT = 6

  def initialize(word)
    @word = word
    @remaining_life_amount = STARTING_LIFE_AMOUNT
    puts word_status
  end

  def submit_guess(guessed_letter)
    if @word.guess_correct?(guessed_letter)
      @word.correct_guesses << guessed_letter
    else
      @remaining_life_amount -= 1
      @word.incorrect_guesses << guessed_letter
    end

    if player_has_won?
      puts "#{@word.value} YOU WIN!"
      return
    end

    if player_has_lost?
      puts "#{@word} YOU LOSE!"
      return
    end

    puts complete_status
  end

  def player_has_won?
    @word.value == @word.to_s
  end

  def player_has_lost?
    @remaining_life_amount.zero?
  end

  def word_status
    "#{@word} life left: #{@remaining_life_amount}"
  end

  def complete_status
    [word_status, @word.incorrect_guess_message]
      .compact
      .join(" ")
  end
end

class Word
  attr_accessor :value, :correct_guesses, :incorrect_guesses

  def initialize(value)
    @value = value
    @correct_guesses = []
    @incorrect_guesses = []
  end

  def guess_correct?(guessed_letter)
    @value.include?(guessed_letter)
  end

  def to_s
    @value.split("").map do |letter|
      if @correct_guesses.include?(letter)
        letter
      else
        "_"
      end
    end.join
  end

  def incorrect_guess_message
    if incorrect_guesses.any?
      "incorrect guesses: #{incorrect_guesses.join("")}"
    end
  end
end

File.readlines(ARGV[0], chomp: true).each do |line|
  next if line.empty?

  if line.length > 1
    @game = Game.new(Word.new(line))
  else
    @game.submit_guess(line)
  end
end

Conclusion

Here are the issues with the original code that were fixed (or at least improved) in the refactored version:

• Long method
• Deep nesting
• Unclear variable names
• Lack of obvious meaning
• YAGNI violations
• Magic numbers
• Needless attr_readers
• Esoteric syntax
• Duplication

Again, if you’d like to make your own Hangman Challenge submission, start here.