“Slogging,” courtesy Tim Gouw (Pexels)
Code should be obvious, or at least direct. We should spend a few minutes taking a look at code we just created that’s not so expressive.
Here’s a short function written in JavaScript.
export const dispTime1 = s => {
s = Number(s)
const h = Math.floor(s / 3600)
const m = Math.floor(s % 3600 / 60)
const hDisplay = h > 0 ? h + (h === 1 ? " hour " : " hours ") : ""
const mDisplay = m > 0 ? m + (m === 1 ? " minute " : " minutes ") : ""
return (hDisplay + mDisplay).trim()
}
Hmm, but isn’t dispTime obvious, at least with respect to what it seems to be doing? Looks like it’s supposed to convert a number of seconds to some sort of printable representation (perhaps to summarize how long it’s going to take someone to understand exactly how the function itself works).
It’s also out of sight, out of mind as a function: Unless someone needs to change how the output is formatted, we’ll probably never have to touch it again. So maybe we leave “well enough” alone.
This, sadly, is how we end up with a system full of similarly sloggish1 code, like sentences that someone slapped into a book without any editing.
If we do end up needing to change dispTime, we probably will hate life a little (maybe only a second or 3600) while we muck with the nonsense that’s the two lines of nested ternaries. But coding life shouldn’t be so many bits of hating life. Let’s spend a couple minutes to whittle dispTime into something not so onerous.
First things first. How about a few tests to know when we break things?
describe('dispTime converts seconds to hours & minutes string', () => {
const cases = [
{seconds: "$%x!!", expected: ""},
{seconds: 0, expected: ""},
{seconds: 1, expected: ""},
{seconds: 60, expected: "1 minute"},
{seconds: 61, expected: "1 minute"},
{seconds: 120, expected: "2 minutes"},
{seconds: "605", expected: "10 minutes"},
{seconds: 5400, expected: "1 hour 30 minutes"},
{seconds: 7200, expected: "2 hours"},
{seconds: 7530, expected: "2 hours 5 minutes"},
{seconds: 7285, expected: "2 hours 1 minute"},
]
cases.forEach(({seconds, expected}) => {
it(`converts ${seconds} seconds to ${expected}`, () => {
expect(dispTime(seconds)).toEqual(expected)
})
})
})
You’re welcome.
In the dispTime function itself: How about some variable names that don’t scream CIA-operative-kind-of-secretive? (As in, “If I tell you what H and S & M mean, I may have to kill you.”)
export const displayTime = seconds => {
seconds = Number(seconds)
const hours = Math.floor(seconds / 3600)
const minutes = Math.floor(seconds % 3600 / 60)
const hoursDisplay = hours > 0 ? hours + (hours === 1 ? " hour " : " hours ") : ""
const minutesDisplay = minutes > 0 ? minutes + (minutes === 1 ? " minute " : " minutes ") : ""
return (hoursDisplay + minutesDisplay).trim()
}
The NaN case is a little interesting. The function must return an empty string if the seconds passed in don’t convert to a number… but it’s not explicitly clear where that coercion to a string actually happens. It just happens—a happy little accident. “Programming by coincidence is no way to go through life, son2.”
We could make a reasonable guess, but knowing for sure would probably require a couple minutes debugging time. “No, that’s cool. It would take too long to look through all that.3”
An explicit guard clause to the rescue:
export const displayTime = seconds => {
seconds = Number(seconds)
if (isNaN(seconds)) return '' // <-- My name is Clause. Guard Clause.
const hours = Math.floor(seconds / 3600)
const minutes = Math.floor(seconds % 3600 / 60)
const hoursDisplay = hours > 0 ? hours + (hours === 1 ? " hour " : " hours ") : ""
const minutesDisplay = minutes > 0 ? minutes + (minutes === 1 ? " minute " : " minutes ") : ""
return (hoursDisplay + minutesDisplay).trim()
}
Once we know the horror is real (“OMG it’s a NaN!”)… get out! The if statement guards against invalid input.
Now we can move on to more mundane things. Sure, everyone immediately recognizes 3600 as the number of seconds in an hour. And 60 as seconds in an hour. No wait, I mean, seconds in a minute. Well, maybe everyone doesn’t immediately get it. You’d be surprised. Or maybe you wouldn’t.
Constants are an easy win for a lot of reasons (minimizing typing or copy/paste, and maximizing mis-typing errors, for a couple):
const SECONDS_IN_AN_HOUR = 3600
const SECONDS_IN_A_MINUTE = 60
export const displayTime = seconds => {
seconds = Number(seconds)
if (isNaN(seconds)) return ''
const hours = Math.floor(seconds / SECONDS_IN_AN_HOUR) // oh I see
const minutes = Math.floor(seconds % SECONDS_IN_AN_HOUR / SECONDS_IN_A_MINUTE) // oh, ok, sure
const hoursDisplay = hours > 0 ? hours + (hours === 1 ? " hour " : " hours ") : ""
const minutesDisplay = minutes > 0 ? minutes + (minutes === 1 ? " minute " : " minutes ") : ""
return (hoursDisplay + minutesDisplay).trim()
}
Now if it weren’t for all that pesky detail mixing into the overall algorithm for displaying the time. Don’t fear the function:
const wholeHours = seconds => Math.floor(seconds / SECONDS_IN_AN_HOUR)
const wholeMinutes = seconds => Math.floor(seconds % SECONDS_IN_AN_HOUR / SECONDS_IN_A_MINUTE)
const displayTimePart = (timePart, description) =>
timePart > 0 ? timePart + (timePart === 1 ? ` ${description} ` : ` ${description}s `) : ""
export const displayTime = seconds => {
seconds = Number(seconds)
if (isNaN(seconds)) return ''
const hours = wholeHours(seconds)
const minutes = wholeMinutes(seconds)
const hoursDisplay = displayTimePart(hours, "hour")
const minutesDisplay = displayTimePart(minutes, "minute")
return (hoursDisplay + minutesDisplay).trim()
}
Hey, that displayTime function is now darn readable, top to bottom! Except, I really don’t want to read line-by-line, top-to-bottom—i.e. stepwise. I want to digest by rapid ingestion. “Get in ma belly, code!”
Some temporary variables should be allowed to live (we don’t really want to make the species go extinct), but feel free to snuff out the ones that provide little value. Here, the temporaries require us to read stepwise through the function and then mentally link together the references to the temporaries.
Inlining is one of a half-dozen critical refactoring operations to ingrain (and can be done split-second, without the mouse in a good IDE):
export const displayTime = seconds => {
seconds = Number(seconds)
if (isNaN(seconds)) return ''
return (
displayTimePart(wholeHours(seconds), "hour") +
displayTimePart(wholeMinutes(seconds), "minute")
).trim()
}
For those keeping score, this inlining combined with the function extractions from the prior step represent degenerate examples of Fowler’s “Replace Temp With Query” refactoring pattern.
Stepwise reading and unnecessary temps are for the birds. Functions are for the cool cats that eat birds for lunch.
Are we done yet? (If you really asked that question, I’ll just explain that I’m a bit OCD when it comes to code. And for good reason. Keep that in mind as you read more Code Talk posts.)
The newly extracted function, displayTimePart, is a hot mess. A nested ternary, lots of of lots of redundant bits (lots of), some whiskers and probably a kitten or two:
const displayTimePart = (timePart, description) =>
timePart > 0 ? timePart + (timePart === 1 ? ` ${description} ` : ` ${description}s `) : "" // ew
No more “ternaries all the way down:”
const displayTimePart = (timePart, description) => {
if (timePart <= 0) return '' // another guard clause!
return timePart + (timePart === 1 ? ` ${description} ` : ` ${description}s `)
}
Adiós, ternario anidado.
And now we can focus on the redundant bits in el ternario… er, the ternary:
const displayTimePart = (timePart, description) => {
if (timePart <= 0) return ''
return `${timePart} ${description}${timePart === 1 ? '' : 's'}`
}
That change requires us to ensure we add back a space inbetween the time parts, back in the part of displayTime where we format the whole thing:
export const displayTime = seconds => {
seconds = Number(seconds)
if (isNaN(seconds)) return ''
return (
displayTimePart(wholeHours(seconds), "hour") + ' ' + // the space between us (time parts)
displayTimePart(wholeMinutes(seconds), "minute")
).trim()
}
Don’t forget, don’t fear the function. You could probably extract 40,000 of them everyday4. Here’s one that shrinks slog time a tiny bit:
const withProperPlurality = (word, count) => `${word}${count === 1 ? '' : 's'}`
const displayTimePart = (timePart, description) => {
if (timePart <= 0) return ''
return `${timePart} ${withProperPlurality(description, timePart)}`
}
Now we done? Let’s take a look.
const SECONDS_IN_AN_HOUR = 3600
const SECONDS_IN_A_MINUTE = 60
const wholeHours = seconds => Math.floor(seconds / SECONDS_IN_AN_HOUR)
const wholeMinutes = seconds => Math.floor(seconds % SECONDS_IN_AN_HOUR / SECONDS_IN_A_MINUTE)
const withProperPlurarity = (word, count) => `${word}${count === 1 ? '' : 's'}`
const displayTimePart = (timePart, description) => {
if (timePart <= 0) return ''
return `${timePart} ${withProperPlurarity(description, timePart)}`
}
export const displayTime = seconds => {
seconds = Number(seconds)
if (isNaN(seconds)) return ''
return (
displayTimePart(wholeHours(seconds), "hour") + ' ' +
displayTimePart(wholeMinutes(seconds), "minute")
).trim()
}
One more sloggy bit: the trim() call is a little bit implicit: It suggests to the reader that either of the displayTimePart function calls could result in an empty string. Or maybe that displayTimePart returns a string that needs to be trimmed. The reader takes a quick look at the helpers to confirm that it’s the former… but it’s little bit more wasted time. Yes, there are alternate ways to code it to make things more explicit, but they all seem to invite a comparable amount of slogginess.
Yes, we done, then.
It’s more code. Less sloggy doesn’t necessarily mean fewer lines of code; it means more direct code, with less repetition. The payoff is not only the immediacy of understanding, but also the compartmentalization that results. (Which in turn fosters increased design flexibility.)
The new functions (“abstractions”) isolate implementation details. They are each, barring a need for change, out of sight and out of mind. If we need only a quick sense of what displayTime does, we can glance at it, and not have to look at the helper functions. If we do need to change, say, how we calculate the number of minutes, we can immediately find that function and make a change only to it. We needn’t sift through or perform careful surgery against the calculation in a more complex context. If we change how we format a time part, we go to just that logic. And so on.
A whole system with slogginess is no fun to work with. Sloggy systems emerge because we tolerate little constant bits of sloggy, because many of us refuse to edit the slop we just typed into the codebase. The original dispTime function seemed good enough to go with, but ultimately such typical slog only wastes everyone’s time.
