Simpler = Better if it_gets_the_job_done

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:

1. If the categories object did not have a hash with the group_id of the row I was on, then I could be sure that I was on a new group_id
2. As long as the group_id didn't change with each iteration, I could simply push the features 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.

Comments

[Paweł Kowalski]

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:

categories[row[:group_id]] = format_data(row) unless categories[row[:group_id]].present?

Become this:

categories[row[:group_id]] ||= format_data(row)

and do the same?

[Arit Developer]

ahhh yes you are right. Your comment finally made me go learn what "double-pipe equals" means in ruby 😄 so thank you!

[Josh]

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

something = {some_boolean: false}

something[:some_boolean] ||= true

The or-assign operator will take the falsey value* at something[:some_variable] and replace it with true. 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't false 😅)

*Rubyists love expressing ourselves in almost-words. falsey and truthy describe values that the double-negative patten will cast as !!foo == false and !!bar == true, respectively.

[Paweł Kowalski]

Glad i could help :)

The .presence thing is also a nice shortcut that I wish came to JS one day :)