Tearing Down a Function (and Building It Back Up) - Part 2
Tags: refactoring, tutorial, javascript, codequality
Published: 2020-02-24 14:44:24

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:

```javascript
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.  

```javascript
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.

1. 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:

```javascript
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.

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

1. 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`.

1. The container will have the dimensions of the `smallestPossibleRedditThumbnail`.

1. Inside the container, we will `getThumbnailImage` for this `post`.

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

1. We check `smallestImageResolution.width` on the `post` against the `smallestPossibleRedditThumbnail.width`.

1. 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.**<br/>Many refactorings _will_ end up with fewer LoC.  But an obsession with fewer LoC can easily lead to objectively _worse_ code.<br/><br/>

* **Focus on changes in composition - not changes in logic.**<br/>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.<br/><br/>

* **Write first.  Refactor second.**<br/>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.<br/><br/>

* **Smaller functions - with descriptive names - can be a key factor in creating "self-documenting" code.**<br/>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.<br/><br/>

* **Micro-optimizations are the enemy of good refactoring.**<br/>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.)<br/><br/>

* **Budget your refactoring time.**<br/>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.<br/><br/>