The Refactor Factor 2: The Rerefactoring

A while ago, I wrote part one of this saga.  I can’t remember what it was about, and am too lazy to look it up, but the theme was refactoring.  I went back to some working code and made it (arguably) better.  At least slightly less crappy.  Today, I did it again.

The aspect of our application under the knife was the sorting function.  Most of our views are just table type grids of data.  These grids have headings (name, address, admin status, etc).  Click the heading and the grid sorts according to that criteria.

I’d created the sorting logic- generally pretty easy, I mean, Javascript has a .sort method built into Array.prototype- but there were complications.  The function would not know what type of data it would be sorting, so we will have to have branches for string and date and others.  Nor would it know the parameter(s) it would be sorting by.  It would have to toggle ascending vs descending sort (click the header once for asc, again for desc).  It would have to be pretty flexible with its inputs.

And- to be added currently, hence the need for the refactoring- the ability for a second sort criteria.  We found that with only one sort criteria (for example: name), the function worked great- as long as everything was unique.  However, what about a column like ‘admin status’.  That’s just a boolean field- only two possible values.  If a user clicked to sort by ‘admin status’, they’d get one listing the first time, but then might get a totally different listing on subsequent clicks.

To make a more stable sort order, we added a defaultOrder variable on each grid component’s class.  That defaultOrder would serve as the initial sort (when the view first loads), and be passed to the sorting service as the second criteria.  So, if a user sorts by admin status, the secondary sorting is done by the defaultOrder (example: name).  The output is consistent and less confusing.

Example time- and please know that I know this is still a bit of a mess.  As I refactored, I started breaking some of the logic out into helper functions, but found that this was creating more work and complexity than simply taking care of it within the .sort call.  I may take another look, but sometimes it just makes sense to write the code in a ‘top down’ type layout- though I know this isn’t often the case.

Version 1: A no good dirty mess (that still kind of works)

orderByParamAscending(param, group) {
    return group.sort((a, b): any => {
        if (typeof a[param] === 'number') {
        //numbers- simple subtraction sort
            return a[param] - b[param];
        } else if(param.indexOf('date') !== -1 || param.indexOf('time') !== -1) {
            //date from string- convert to date obj and compare
            const first = newDate(a[param]);
            const second = newDate(b[param]);
            return +first - +second;
        } else if (typeof a[param] === 'string') {
            //strings- check for empty
            let first = '', second = '';
            //convert to lowercase and compare number code
            if(a[param]) {
                first = a[param].toLowerCase();
            }
            if(b[param]) {
                second = b[param].toLowerCase();
            }
            if(first < second) {
                return -1;
            } else if(second < first) {
                return 1;
            }
            return 0;
        } else {
            //other (usually bool)
            if(a[param] < b[param]) {
                return -1;
            } else if(b[param] < a[param]) {
                return 1;
            }
            return 0;
        }
    });
}

I know- not pretty.  We’re using Typescript, which is awesome, but for a while I couldn’t get this to work.  TS would throw an error about a function (.sort in this case) not having a unified return value.  And that’s because it’s a bad function.  But you can shut Typescript up by providing the ‘any’ return type for a function- just know that this is an almost surefire indicator of bad code.

So- how could I make it better?  Well- instead of doing all those checks on each parameter, I could create a couple variables that would hold the value- then update them based on the type.  Also, .sort works well if you return 1, -1, or 0 depending on the result of your comparison function, so once the values are properly parsed, we can compare and return one of those 3:

Version 2: Slightly simpler, but still no secondary sort

orderByParamAscending(param, group) {
    return group.sort((a, b) => {
        let first = a[param], second = b[param];

        //check if this is a datetime param or string and parse so we can sort numerically

        if(param.indexOf('date') !== -1 || param.indexOf('time') !== -1) {
            first = +newDate(a[param]);
            second = +newDate(b[param]);
        } else if(typeof a[param] === 'string') {
            first = a[param].toLowerCase();
            second = b[param].toLowerCase();
        }

        //parsing done- time to compare

        if(first < second) {
            return -1;
        } else if(second < first) {
            return 1;
        }
        return 0;
    });
}

A bit easier to follow- and we can lose the ‘any’ return type- we are now always returning a number.  We handle all the formatting of the comparators (the ‘first’ and ‘second’ variables) before doing any comparing.

But the proposed additional functionality still isn’t there.  I initially thought it would just be a case of calling the function twice on the same array- the first time with the default sort param (‘name’ for example) to make sure the data is in the right order at the outset, then call it again with the new param (‘admin status’) to re-order, with the original as a fallack.  But this does not work- the first call has no carry-over to the second (which makes sense, but I had hopes for a simple solution!).

And, in the end, the solution wasn’t that complicated- it was just testing to make sure the sort returns the right order that was a hassle.  The only real changes were to the arguments passed and to the sorting logic at the bottom of the function- nesting the secondary sort into the initial in case of a ‘tie’.

Version 3: A little more complex than V2, but a little more functionality too!

orderByParamAscending(param, group, altParam) {
    return group.sort((a, b) => {
        let first = a[param], second = b[param];
        let altFirst = a[altParam], altSecond = b[altParam];
       

        //check if this is a datetime param or string and parse so we can sort numerically

        if(param.indexOf('date') !== -1 || param.indexOf('time') !== -1) {
            first = +newDate(a[param]);
            second = +newDate(b[param]);
        } else if(typeof a[param] === 'string') {
            first = a[param].toLowerCase();
            second = b[param].toLowerCase();
        }
        //do the same for the secondary sort parameter
        if(altParam.indexOf('date') !== -1 || altParam.indexOf('time') !== -1) {
            altFirst = +newDate(a[altParam]);
            altSecond = +newDate(b[altParam]);
        } else if(typeof a[altParam] === 'string') {
            altFirst = a[altParam].toLowerCase();
            altSecond = b[altParam].toLowerCase();
        }

        //parsing done- time to compare

        if(first < second) {
            return -1;
        } else if(second < first) {
            return 1;
        } else {
          if(altFirst < altSecond) {
               return -1;
          } else if(altSecond < altFirst) {
               return 1;
          }
          return 0;
    });
}

There are other aspects we added- a ‘sortDirection’ toggle, a check for null values passed to the sort to convert them to strings (the comparison was failing on null- but some values from the database were null), but this is the meat of the function.  And I’m sure there are still improvements to be made- programming seems a little more art than science sometimes.  It can be hard to tell when you’re ‘done’ with something.  I was ‘done’ on version 1, but version 3 is much better.  At some point, you just have to move on to the next task, or your product will never ship!

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s