First Thought Code

For me, code usually starts with a first approach. Often that first try might work, but it isn't code I'd be proud to ship or share. With this post, I thought I'd put you inside my head as I recently experienced this process.

I was working on very simple Node api wrapper over a database query with an optional parameter to filter the result set.

const select = "select id, partNumber, name, size from inventory"

function whereClause(req) {
    return req.params.id ? `where id = ${req.params.id}` : ''
}

router.get('/', executeQuery(req => `${select} ${whereClause(req)}`))

router.get('/:id', executeQuery(req => `${select} ${whereClause(req)}`))

There was a request to add a second optional filter with a query location parameter. My first thought was adding more if statements to the whereClause function

function whereClause(req) {
    if (req.params.id || req.query.location){
        let clause = 'where '
        if (req.params.id) 
            clause += `id = ${req.params.id}`
        if (req.query.location){
            if (req.params.id)
                clause += ` and location = ${req.query.location}`
            else
                clause += `location = ${req.query.location}`
        }
        return clause
    }
    return ''
}

I went on to the next thing, but the code nagged me. "This would be hard to test." [Yes I admit I had no unit tests and I was thinking of the manual testing consequences.]

Finally, I came back to do some refactoring. What don't I like, and these are the kinds of things I'd say in a code review of someone elses code too.

1. The intent is a bit hidden by the higher than needed cyclomatic complexity.
2. References to parameters are used in many places and it could be easy to make an accidental mistake.
3. What happens when a third optional parameter is added?

Things I want to think about when refactoring this code:

1. How could we reduce the number of conditional branches?
2. How can you clarify the intent?

I decided to take on the last one first

function whereClause(req) {
    if (req.params.id || req.query.location){
        let clause = 'where '
        if (req.params.id)  
            clause += idClause(req)
        if (req.query.location){
            if (req.params.id)
                clause += ' and '
            clause += locationClause(req)
        }
        return clause
    }
    return ''
}

function idClause(req) {
    return req.params.id ? `id = ${req.params.id}` : ''
}

function locationClause(req) {
    return req.query.location ? `location = ${req.query.location}` : ''
}

That's a little better, but it really just pushed the if statements around. I still haven't addressed most of the concerns. Next, I decided to use array and join to hide some conditionals and deal with a potential third clause.

function whereClause(req) {
    if (req.params.id || req.query.location){
        const clauses = []
        if (req.params.id)  
            clauses.push(idClause(req))
        if (req.query.location)
            clauses.push(locationClause(req))
        return `where ${clauses.join(' and ')}`
    }
    return ''
}

function idClause(req) {
    return req.params.id ? `id = ${req.params.id}` : ''
}

function locationClause(req) {
    return req.query.location ? `location = ${req.query.location}` : ''
}

This is better, but I still have a more cyclomatic complexity than I want. And I still reference parameters in almost as many places. Plus I've added a new state that is modified in the array. My next attempt was to use some array destructuring.

function whereClause(req) {
    const clauses = [
        ...idClause(req),
        ...locationClause(req)
    ]
    return clauses.length ? `where ${clauses.join(' and ')}` : ''
}

function idClause(req) {
    return req.params.id ? [`id = ${req.params.id}`] : []
}

function locationClause(req) {
    return req.query.location ? [`location = ${req.query.location}`] : []
}

I really liked this. I almost quit at this point but the use of parameter in multiple spots still nagged at me. Finally, I ended up with this.

function whereClause(req) {
    const clauses = [
        ...clause(req.params.id, 'id'),
        ...clause(req.query.location, 'location'),
    ]
    return clauses.length ? `where ${clauses.join(' and ')}` : ''
}

function clause(optional, field) {
    return optional ? [`${field} = ${optional}`] : []
}

How is this better?

1. Now you can see immediately that there are at most two clauses.
2. If a third clause came a long, there would only be one place to change.
3. Each of the optional parameters is only referenced one time, so there are fewer chances for mistakes.
4. The cyclomatic complexity is lower.
5. There are fewer lines of code.

How is it worse?

1. It relies upon language and library features that may not be as obvious immediately.
2. It has aspects that are more terse.

At least for me, writing software is often an iterative process: get it working, make it better. If I had tests, and I should, this would take place in a red/green/refactor loop. Make sure you review your code before you commit a feature and look for ways to be more defensive and better express intent. Adding comments should be your last means to tell why you did something.