Refactoring My Old Ruby Code

For my first blog post I thought it would be fun to look back and refactor some code I wrote when I first started learning Ruby. It’s from a lesson that any Flatiron School student will remember well: Hashketball.

First, you’re given a bunch of information about two basketball teams and told to build a nested hash. Then, you’re instructed to write methods that return certain information about a player or team. For example: player_numbers takes in an argument of a team name and returns an array of the jersey numbers for that team.

At the time, this lesson was really hard for me. I’d never worked with such a big nested hash before and was pretty new to the idea. You’re instructed to use .each and binding.pry to get comfortable with iteration. This was great at the time, as it helped me learn what to expect from iteration and I got really comfortable using binding.pry.

There are seven methods to write, and my original code came out to 128 lines, with tons of repetition and if statements. Now that I’m a few months into learning Ruby, I know there are much better, more efficient ways to write this code. So I’m going to take my knowledge of helper methods to shorten it!

The first thing I want to do is create some helper methods to make everything easier on myself. My previous code had me iterating through the game hash, then through that data, then through that data… but now I know the programming rule DRY, don’t repeat yourself.

I’m going to write a method, all_players, which simply takes the home team’s players and the away team’s players and merges them into one.

def all_players
  game_hash[:home][:players].merge(game_hash[:away][:players])
end

Now I can access all of the players and their stats in one method, no more repetitive iteration. I’m going to use this method to refactor the player_stats method. This one takes an argument of a player’s name and returns only their stats.

def player_stats(name)
  all_players[name.to_sym]
end

This method is part of the test but is in itself a helper method. I can use it to simplify num_points_scored and shoe_size, making them much easier to read. They both take an argument of a player’s name and return one of their stats.

def num_points_scored(name)
  player_stats(name)[:points]
end

def shoe_size(name)
  player_stats(name)[:shoe]
end

How beautiful! How clean! Next up we have some methods that take an argument of a team name, so I’ll create two more helpers for this. One that returns both team’s data, and another that takes an argument of a team’s name and returns only its data.

def all_teams
  game_hash.values
end

def find_team(team_name)
  all_teams.find { |team| team[:team_name] == team_name }
end

Wonderful, now I can refactor the team_names method. This one simply returns both team’s names, so I can use my all_teams helper method.

def team_names
  all_teams.map { |team| team[:team_name] }
end

So satisfying! Now I can use my other helper, find_team to shorten both team_colors and player_numbers.

def team_colors(team_name)
  find_team(team_name)[:colors]
end

def player_numbers(team_name)
  find_team(team_name)[:players].map {|name, stats| stats[:number]}
end

Finally we have the monster method big_shoe_rebounds, which is just painful to look at:

def big_shoe_rebounds
  shoe_sizes_w_names = {}
  game_hash.each do |location, team_data|
    team_data.each do |attribute, data|
      if attribute == :players
        data.each do |name, stats|
          stats.each do |k, v|
            if k == :shoe
              shoe_sizes_w_names[name] = v
            end
          end
        end
      end
    end
  end
  player_w_big_shoes = shoe_sizes_w_names.key(shoe_sizes_w_names.values.max)
  game_hash.each do |location, team_data|
    team_data.each do |attribute, data|
      if attribute == :players
        data.each do |name, stats|
          if name == player_w_big_shoes
            stats.each do |k, v|
              if k == :rebounds
                return v.to_i
              end
            end
          end
        end
      end
    end
  end
end

Yikes. That’s 34 lines for one method! The big problem here is that this method is doing two separate things:

1. finding the player with the biggest shoe size
2. finding that player’s number of rebounds

There’s a philosophy in programming: a method should do one thing, and do it well. With this in mind, I decided to first create a method that finds the player with the biggest shoe size.

def player_w_biggest_feet
  all_players.max_by { |name, stats| stats[:shoe] }
end

Now I can use this method to find their rebounds, and pass the final test. However, I could also use it to find any other data about the player with the biggest feet, if I ever wanted to.

def big_shoe_rebounds
  player_w_biggest_feet[1][:rebounds]
end

And that’s it! In the end, I took my code from 128 lines down to 43, and every method is only 3 lines. The result is the same, but now the code is much easier to read and understand.

To summarize, I learned a lot about the basics of Ruby the first time I did this lesson. Knowing how .each works is really important and builds a great foundation for understanding more abstract methods like .map and .find. Refactoring and working with it a second time around was a lot more fun than I was expecting! It can be a little cringe-inducing to look back at your early code, but a lot can be learned from it. Thanks for reading!