Scaling Up
One of the bigger challenges in taming legacy code is the size and scope of the existing functions. Larger functions have more dependencies and more complicated logic. You first need to write characterization tests to capture the intent before refactoring to smaller, more focused functions. The functions that I deferred so far fall in that category. I had run out of simple functions and needed to forge ahead.
The next most complicated functions were the date and time parsing utility functions deferred earlier. Of the two remaining functions, splitDateTime() and parseDateInternal(), splitDateTime() was the easier. For one, parseDateInternal() calls splitDateTime(), and it also calls other functions of the datepicker, both original and overridden.
As with the other utility functions, I started by exposing it through the _util namespace I added [ff99363]. Testing the happy path and a couple variations ended up being straightforward. The daunting part was the error path. The code contained a cryptic comment about leveraging the format of an error message as a hack (it said so in the code!) to avoid replicating parsing logic. However, I could not see where in the try block such an exception could be generated in order to test it. For the time being, I left a TODO to mark the loose end.
I chose to continue to parseDateTimeInternal(), despite not having tested the rather complex collaborator functions it relies on. Fortunately, the intent of the collaborators seemed clear, and the returns from each passed directly to the return result without additional manipulation. I only needed to understand how to pass the right parameters to trigger the desired result.
The first two tests exercised the happy path variations. I chose to throw in microseconds for the time-parsing path, which made the setup a little more complex.
The error path test introduced a new Jasmine feature, the toThrow() matcher [1cc967a]. To use this matcher (Listing 15-5), you need to structure your expectation a little differently.
Listing 15-5: Use of Jasmine’s toThrow() matcher to verify an exception error path
it('should throw an exception if it cannot parse the time',
function() {
var inputDateString = '4/17/2008 11:22:33';
expect(function() {
$.timepicker._util._parseDateTimeInternal(
dateFormat, 'q', inputDateString, undefined, undefined
);
}).toThrow('Wrong time format'),
});
Instead of providing a value to expect(), you provide a function that Jasmine invokes, capturing and storing the exception for matching.
Testing the error path found a bug. The underlying parsing used by the collaborator $.datepicker.parseTime() returns false if it cannot parse the time. However, the implementation tests the return from this function against null using the === operator. If you are not familiar with them, the == and === operators in JavaScript are similar, except that the latter one also checks the type of its operands while the former happily coerces values’ types to see if there is a match. While null is falsey7 in JavaScript and would evaluate false with the == operator, the strict equivalence comparison sees them as different. As a result, the comparison never triggered the exception. Simply changing the equality test (parsedTime === null) to a logical negation (!parsedTime) fixed the problem [c264915].
7. In JavaScript, falsey values are false, null, undefined, 0, NaN, and the empty string. All other values are truthy, including empty objects and arrays.
With test protection, I could refactor parseDateTime-Internal() to simplify it [353ff5e]. The existing timeString variable was unused, so I used it where its initializer had been duplicated to be more intent revealing. I flipped the if-else statement so that it started with the positive condition rather than the negation. This allowed me to flatten some of the nested conditionals by handling the no-time situation with a guard condition that returns early. The flattened function is much cleaner to read.
I wanted to do a couple more cleanups before leaving the time-utility functions. The splitDateTime() function returned a two-element array consisting of the date and time strings by construction. The parseDateTimeInternal() function introduced intermediate variables to provide intent-revealing names to the elements of the returned array. I decided to turn the return array into an object so that the intent-revealing names were built into the return [353ff5e]. This allowed the removal of the intermediate variable [5109082].
The beginning two variable initializations of splitDateTime() struck me as verbose and highly similar. They determined a property by giving the passed settings, if defined, priority over the Timepicker defaults. Commit [11a2545] refactored the initialization logic into a common function computeEffectiveSetting(). Of course, the new function needed to be directly tested [ea7babe], but I left all of the testing variations of splitDateTime() because they also represent important behavioral facets.