Today at work, my teammate reviewed the following code I had written:
In a nutshell, my build_page_data
method builds an object called categories
which contains a number of objects (each one representing a category
). The variable @page_data
contains several rows of information where, for one group_id
, there are several features
. So, there could be 10 rows each with the same group_id
, but with a distinct feature
.
My teammate - let's call him "David" - commented that Lines 7, 8 & 18 confused him and weren't terribly intuitive. I explained to him how the loop on Line 7 checked to see if we'd moved unto a new group_id
to build a new category
object, while Line 8 populated the features
attribute of the last category
object (after being sure that we'd collected all associated features). Line 18 made sure to populate the last row's features
attribute (edge case).
Did my explanation bring on a headache for you just now? Because it did for David. He asked me if there was a way to build the data without keeping track of which row I was currently on (i.e. using a pointer). He suggested that:
- If the
categories
object did not have a hash with thegroup_id
of the row I was on, then I could be sure that I was on a newgroup_id
- As long as the
group_id
didn't change with each iteration, I could simply push thefeatures
hashes into the features array for that category
I went back to the drawing board my IDE and worked on implementing David suggestions. First, in the loop iterating through @page_data
, I included a guard that made sure that either a category with the current group_id
existed, or one was created. Next, I pushed the features
of each row into the features array of their associated category. Finally I returned the completed categories
object:
I sat back and marveled at the simplicity of my refactored build_page_data
method! All edge cases were accounted for, and I had cut the size of my method in HALF (from 16 lines to 8)! This experience exemplifies why I'm so grateful for my current role - I am learning at what sometimes feels like an exponential rate, and getting paid while doing so! This refactor brought me such satisfaction, and challenged me to always ask: "Can my code be simpler? What can I do away with and still accomplish my goal?" I'm also grateful for a team that teaches me to fish challenges me to think and grow, rather than simply hand me the answers.
Top comments (4)
Theres no better feeling than slashing LOC by half + achieving better results and readability in the process. :)
BTW. Im just a JS guy, but i think ruby has this cool
||=
operator that makes some things easier.Can this:
Become this:
and do the same?
ahhh yes you are right. Your comment finally made me go learn what "double-pipe equals" means in ruby 😄 so thank you!
First off, great post! I was worried at first that "David" was going to be unnecessarily critical of your PR, but it's awesome that his comments brought you to a number of useful refactors, and even better that you shared what you learned to help other new developers 😁
There's a subtle gotcha case with the or-assign operator, but it shouldn't occur in the context of this loop: let's say you have
The or-assign operator will take the falsey value* at
something[:some_variable]
and replace it withtrue
. Similarly, the&&=
operator will reassign variables that are truthy* (useful for cases like when you want to update values in a hash, but only if they already exist … and aren'tfalse
😅)*Rubyists love expressing ourselves in almost-words.
falsey
andtruthy
describe values that the double-negative patten will cast as!!foo == false
and!!bar == true
, respectively.Glad i could help :)
The
.presence
thing is also a nice shortcut that I wish came to JS one day :)