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!

The Copy/Paste Conundrum

We’re covering just a quick bit of refactoring today on the Devblog.  Next week we’ll be back to longer, rambling posting!

In our Angular application (which version?  remember- now It’s Just Angular!) we have a ‘setup’ module.  It allows an admin to edit certain values in the database (payment types, exchange rates, active states, etc).  Views in this module also include some reporting (charts, total items, etc).  In order to ensure everything updates when a change is made, we used a shared ‘setup.service’ class, stored the data as Behavior Subjects (each an array of objects that can be converted into an Observable with .asObservable()), then subscribe to the resulting observable in the components themselves.

I know Observables are usually though of as being used with ‘live’ data- coming from a database like Firebase, that can broadcast out updates in real time.  We don’t do that (we are using SqlServer), but found that Observables can still be useful in a single page application.  When data is saved in the ‘edit form’ area of the page, we can manually fire the Behavior Subject’s .next() function to get the latest data updated ‘live’ into the reporting area.

Anyway- that long intro was just to provide background.  The actual refactoring was in the setup.service class.  We have a whole bunch of Behavior Subject/Observable pairs initialized here (for all the types of data we want to update and subscribe to in this Setup section).  We also had multiple people adding methods for updating data to this class.  So, developer A creates a method for ‘getAuditTypes’ – to get a Json response with all the possible audit log types from the database.  It looked a little like this:

getTypes() {
    this._backendService.getData(CONFIG.auditTypesApiUrlBase)
        .subscribe(fetchedTypes => {
            this._activityTypes.next(fetchedTypes);
            this.getAuditCharts();
            this.getSetupBarCharts();
        },
        error => {
            this._notificationService.showNotification({ message: 'Error Getting Activity Types', type: 'error' });
    });
}

It uses a backendService method (which is just a helper to make sure the right authentication and headers are set on http requests) to call out, get the data, subscribe to the response, call the .next method with the new data and update any charts.  Works great- but as more views were added and data was being fetched, more of these methods were being added to the setup.service class (each person who worked with this file would just add a new method to get the data they needed).  In the end, we had almost 20 methods that were almost identical to the one above (with the exception of the api route and behavior subject name being called.  Great to get things working quickly (just a bit of copy/paste), but terrible for maintainability (edits or updates to the logic would have to be repeated almost 20 times).

So, we just refactored the method to receive a couple arguments.  Now there is one shared method to get data for the setup section (instead of 20).  It looks very similar:

getSetupData(apiUrl: string, nextTitle: string) {
    this._backendService.getData(apiUrl)
        .subscribe(response => {
            this[nextTitle].next(response);
            this.getAuditCharts();
            this.getSetupBarCharts();
        },
        error => {
            this._notificationService.showNotification({ message: 'Error Getting Activity Types', type: 'error' });
    });
}

Almost identical- except for a couple new arguments.  In the component (class) that uses this shared service, we pass parameters specific to the data being retrieved and the array being updated:

this.setupService.getSetupData('http://yourdomain.com/api/apiendpoint', '_behaviorSubjectName');

This way, we get the specific data we need to the component, but don’t have a whole bunch of unnecessary copied methods on the service.  Oh, and did I say this would be short and not rambling?  Too late… Sorry.

The refactor factor

Sometimes (ok, most times), in order to get the ball rolling on an aspect of a project, I’ll do a bad job.  I’ll create a component or module in the code that works, but is a bit of a mess.  It’s probably not the best idea- my intention is always to go back and optimize, but it doesn’t always work out that way.

But when it does- when I have the time and permission to go back and clean up the code- it’s great!  I get to review how I started out, see what I did wrong, and do my best to fix it.  It’s a great way to improve.

As an example- I’m working on an Angular 2 project right now.  We’re still getting things started, but at the very beginning, I created a few custom page interaction components- a loading ‘spinner’, a notification pop up, and a modal window.  They all worked, but each had flaws.  The most glaring was that each directly manipulated the DOM- a no-no in Angular.

And after going back to refactor, I now know why.  When dependency injection is done right (another thing I had to refactor), it’s a beautiful thing.  But when it’s not, it can create loading order and time issues.  When I cleaned up DI in the app, those 3 custom components broke.  All of a sudden, the browser shouted ‘cannot call style on null’.

And that was because I was manipulating the style directly in a component.  To show the loading spinner, I called spinner.show()- but all that really did was grab the spinner element and change display:none to display:block.  Now that everything was loading correctly, that spinner div didn’t exist when the component loaded (the template hadn’t been injected yet), so the app took the train to errortown.

So, instead of grabbing each element and working the old JS way, I realized I could just bind to native properties in the template.  The spinner.show() function would just update a boolean flag (showing) to true.  That flag is bound to the [hidden] property on my spinner div, and I was back in business.

I try to read a lot about web development and programming, and many times have see the advice that premature optimization is the root of all evil.  But when it comes to refactoring- not necessarily for performance, but for functionality and ease of use/updating- there really doesn’t seem to be a bad time to start.