Tearing Down a Function (and Building It Back Up) - Part 2

Since this is Part Deux, I'm gonna skip past any wordy preamble and get right to our topic. After making numerous changes in Part 1, our function currently looks like this:

getThumbnail = post => {
   const smallestPossibleRedditThumbnail = {
      height: 108,
      width: 67,
   };
   const emptyThumbnailContainer = <div style={smallestPossibleRedditThumbnail}> </div>;
   if (!post.preview || !post.preview.images) 
      return emptyThumbnailContainer;
   const images = post.preview.images[0];
   if (!images.resolutions) 
      return emptyThumbnailContainer;
   const smallestProvidedImage = images.resolutions[0];
   if (smallestProvidedImage.width !== smallestPossibleRedditThumbnail.width) 
      return emptyThumbnailContainer;
   const url = smallestProvidedImage.url.replace(/&amp;/g, '&');
   return (
      <div style={smallestPossibleRedditThumbnail}>
         <img src={url} alt={'thumbnail'}/>
      </div>
   );
};

We previously renamed some of the variables for clarity, removed magic numbers, and flattened the hell out of the nested logic with copious use of return. So is there anything more to be done to this function? Well...

The function "works" perfectly in its current iteration. We've reduced some of the cognitive load that existed in its initial state. And I don't see too many line-by-line opportunities to shorten this or make its purpose far clearer.

But...

I definitely feel that it's still short of being self-documenting code. To be clear, full-on self-documenting code is a bit of a fairy tale. I've heard devs talk about it. I've seen presentations on it at software conferences. But when we're sitting down to fill that next Jira ticket, we rarely go that "extra mile" to make our code truly self-documenting.

We get our code to a point where it "works" (like the first version that I shared in Part 1). Depending upon our workload, and how much we care, we might take the time to tweak it until it reaches the state you see above. But we rarely go past that. And to be clear, many of the devs I've known wouldn't even like to go past that. They'd look at the code above, see that it passes all the unit tests, and hastily move on to the next task.

I'm not gonna burn a lotta words trying to tell you what self-documenting code is (IMHO). It's easier just to show you.

smallestPossibleRedditThumbnail = {
   height: 108,
   width: 67,
}

getThumbnailContainer = post => {
   return (
      <div style={this.smallestPossibleRedditThumbnail}>
         {this.getThumbnailImage(post)}
      </div>
   );
};

getThumbnailImage = post => {
   if (!post.preview || !post.preview.images) 
      return null;
   const images = post.preview.images[0];
   if (!images.resolutions) 
      return null;
   const smallestProvidedImage = images.resolutions[0];
   if (smallestProvidedImage.width !== this.smallestPossibleRedditThumbnail.width) 
      return null;
   const url = smallestProvidedImage.url.replace(/&amp;/g, '&');
   return <img src={url} alt={'thumbnail'}/>;
};

I've made some fairly radical changes here:

1. Regardless of what we found in the post argument, we were always returning at least the default/empty thumbnail container. So I split the function into two pieces: getThumbnailContainer() (which will always blindly return the default <div> wrapper) and getThumbnailImage() (which will dynamically determine whether to return an <img>... or nothing at all.

2. I lifted the smallestPossibleRedditThumbnail constant into the class scope. I did this because it's needed in both getThumbnailContainer() and getThumbnailImage(). Since you Don't want to Repeat Yourself, I put the value in a commonly-accessible place.

Although it may "feel" more convoluted at first, by splitting the function up I can now name each function much more accurately, and:

Accurately-named functions foster self-documenting code.

At this point, I think we're officially "done" with getThumbnailContainer(), but getThumbnailImage() still needs some love. Specifically, the first four lines are basically doing a single "thing". In other words, those four lines are all designed to drill into the post object and (safely) retrieve the smallest image resolution on the post.

The ideal for any function/method is that it should do one thing - and only one thing - and do it well.

So since we know that those first four lines are kinda doing their own thing, we can add clarity by further breaking this up:

smallestPossibleRedditThumbnail = {
   height: 108,
   width: 67,
};

getSmallestImageResolution = post => {
   if (!post.preview || !post.preview.images || !post.preview.images.length) 
      return null;
   const images = post.preview.images[0];
   if (!images.resolutions || !images.resolutions.length) 
      return null;
   return images.resolutions[0];
};

getThumbnailContainer = post => {
   return (
      <div style={this.smallestPossibleRedditThumbnail}>
         {this.getThumbnailImage(post)}
      </div>
   );
};

getThumbnailImage = post => {
   const smallestImageResolution = this.getSmallestImageResolution(post);
   if (smallestImageResolution === null) 
      return null;
   if (smallestImageResolution.width !== this.smallestPossibleRedditThumbnail.width) 
      return null;
   const decodedUrl = smallestImageResolution.url.replace(/&amp;/g, '&');
   return <img src={decodedUrl} alt={'thumbnail'}/>;
};

What was originally one function is now three - in addition to a new constant for the Reddit height/width values. Based on that sentence alone, one might think that I've made the code less clear. But consider this:

1. Chapters don't decrease a book's clarity. They increase it.

2. Paragraphs don't decrease a chapter's clarity. They increase it.

3. Sentences don't decrease a paragraph's clarity. They increase it.

You could write a book as one string of words with no chapter breaks, or paragraphs, or punctuation to delineate sentences. But no one would want to read that book. It would be a nightmare to comprehend. Coding is often the same way.

The entry point to this functionality would be in getThumbnailContainer(). So given our new structure and function names, we can pretty-well "read" the code like this.

1. We will getThumbnailContainer for the given post.

2. The container will have the dimensions of the smallestPossibleRedditThumbnail.

3. Inside the container, we will getThumbnailImage for this post.

4. We only want the smallest thumbnail (if a thumbnail even exists in that size), so we will getSmallestImageResolution for the given post.

5. We check smallestImageResolution.width on the post against the smallestPossibleRedditThumbnail.width.

6. If the two values are equal, we return an <img> to be included in the thumbnail container.

Eye of the Beholder

None of this refactoring is empirically "right" or "better". In fact, if you go back and look at the original function in Part 1 of this post, there are some people who might actually prefer that version. So my intention is not to show you The One True Way to refactor. Instead, I'd prefer to just "get you thinking".

Whether you prefer my finished version or not, here are some key concepts to consider:

• Refactoring is not a contest to blindly eliminate as many LoC as possible.
  Many refactorings will end up with fewer LoC. But an obsession with fewer LoC can easily lead to objectively worse code.
  
  

• Focus on changes in composition - not changes in logic.
  If you're hacking up the core logic of the function, then you're not refactoring it - you're rewriting it. Most of the techniques outlined here can be safely used in any codebase - whether the function was written by you or anyone else.
  
  

• Write first. Refactor second.
  When writing new code, get the core logic out of your head and onto the screen. Don't try to refactor while you're coding. You can waste a lot of time trying to "prettify" every line as you write it. For example: As I'm working through a task, I will often write numerous, nested layers of logic in a single function. And the code works just fine when I'm done. But then I stand back and look at the "finished" function and realize that the nested logic can be flattened by using short-circuit return statements.
  
  

• Smaller functions - with descriptive names - can be a key factor in creating "self-documenting" code.
  It can feel silly to create additional, single-use functions. And I'm not saying that you should always follow that practice.
  But the improvement in readability can be immense. Readability isn't just some theoretical ideal. It leads to fewer bugs. And it's easier to trace those bugs when they do exist.
  
  

• Micro-optimizations are the enemy of good refactoring.
  There's always some person (jerk) in your shop who wants to bloviate about the additional microseconds that are "wasted" by, say, a temp variable. He always has some JSPerf report about how while is 0.000039254 microseconds faster than for when run over 12 billion iterations. He'll get all up-in-arms about the wasted "memory" that's used by your temp variable. By your single little temp variable. But if that temp variable makes your code more readable, then that infinitesimal patch of memory is very well spent indeed. (BTW, this is usually the same jerk who likes to brag that he could have written your entire function - or class, or module, or application - in a single line of code. A single unintelligible line of code.)
  
  

• Budget your refactoring time.
  Most of the techniques that I've highlighted in these last two posts really don't take much time at all. The refactoring that I've done through all these illustrations can easily be accomplished in minutes. But even a handful of minutes still qualifies as time. And any time infers a certain cost. When you've got that function refactored to the point that you want to take a picture of it and submit it to the Smithsonian, you might be tempted to tackle all the other functions in the file. Don't. Trust me on this. Just... don't. Before you know it, you've burned an entire day tidying up every damn bit of code that you can get your hands on - and you haven't completed a single task that was assigned to you.