TL;DR How I refactored some messy if-else code into something that rocked.

I’m constantly looking for ways to make code more readable. What I want is code that informs the reader what it does, but hides how it does it. It’s commonly called abstraction! ( Gosh! 😜 )

Below I’ve included a small code example which demonstrates this concept, this was something that I found in our code-base. I could go on to explain what the code does and why, but the code should be readable, and at this level it should be readable without too much programming knowledge. Take a look at the code marked V1.

• How well did I do on this refactor?

• Does the function getSortableColumn(nonSortableColumn) convey enough information about what it does, without having to read the function body?

The approach I’ve took is to refactor the conditions to a table. This is a common approach you can find examples of this in a number of blogs and publication ( just google it ).

Will the next developer be able to understand the code? How does it compare to the before version of the code?

Before Code

Below is the code I found, there is a bug and some redundant code as well.

• It’s possible that the sort order could be “”, which I’m sure is a bug

  // Before, this is where we start
  static sortBenefitsSummary(benefits: CurrentBenefit[] | PreviousBenefit[], selectedSort: CurrentBenefitsSummarySort | PreviousBenefitsSummarySort): CurrentBenefit[] | PreviousBenefit[] {
    let sortColumn = '';
    if (selectedSort.sortBy === 'benefitStartDate') {
      sortColumn = 'startDateUnixTime';
    } else if (selectedSort.sortBy === 'benefitName') {
      sortColumn = 'benefitName';
    } else if (selectedSort.sortBy === 'benefitEndDate') {
      sortColumn = 'endDateUnixTime';
    }
    return AccountUtils.sortBy(benefits,
      [sortColumn], [selectedSort.sortDirection]);
  }

Refactor to table/lookup

OK, it’s not quite a table, I’ve use an object literal, and I’m using it as a hash lookup.

• Step 1 : Extract function/method
• Step 2 : Give it a name that reflects what it does
• Step 3 : Create an object literal, and use it as a lookup table, from one value to another
• Step 4 : Don’t forget to have a default, for cases where the lookup fails to produce a value.

  static getSortableColumnV1(columnName: string): string {
      const nonSortColumnToSortableColumn = {
        "benefitStartDate": "startDateUnixTime",
        "benefitEndDate"  : "endDateUnixTime"
      };
      return nonSortColumnToSortableColumn[columnName] || columnName;
  }

Extract function/method

Extracting the code to it’s own function is a good step, and this might be good enough for some. But the if-else-if code below is so hard to read, the complexity it high.

getSortableColumnV2 takes an approach that I’ve see a lot, too many times, it’s a classic code smell!! A good function name is a good start, but code complexity is high on this function, it’s not an approach that I would recommend.

 
  static getSortableColumnV2(selectedSort: CurrentBenefitsSummarySort | PreviousBenefitsSummarySort) {
      let sortColumn = '';
      if (selectedSort.sortBy === 'benefitStartDate') {
        sortColumn = 'startDateUnixTime';
      } else if (selectedSort.sortBy === 'benefitName') {
        sortColumn = 'benefitName';
      } else if (selectedSort.sortBy === 'benefitEndDate') {
        sortColumn = 'endDateUnixTime';
      }
      return sortColumn;
  }

Just a bunch of if’s

The last example is ok, but some developers don’t agree with multiple return statements the issue of multiple return statements has been discussed so many times. I’m happy to go with the advice from Martin Fowler and so many other great developers. What is their advice? Multiple return statements can be the solution, as long as you follow all the other guide lines on clean code.

getSortableColumnV3 This is straight out of the classic book by Martin Fowler (refactoring), there are multiple discussion about this approach, here is one link. http://wiki.c2.com/?GuardClause, there are more.

  static getSortableColumnV3(nonSortableColumnName: string) {
    if (nonSortableColumnName ===  "benefitStartDate") return "startDateUnixTime";
    if (nonSortableColumnName ===  "benefitEndDate") return "endDateUnixTime";

    return nonSortableColumnName;
  }