Code Review chronicles: destructuring, linting and one valid reason for Typescript

Recently I was reviewing some code which was using recursion to load all data available from an endpoint (a kind of awkward way of overcoming pagination and load everything "at once" - but this is another story) and I noticed something that in my opinion was counterintuitive.

const finalResults= {items: []}

const loadRecursively = async (params, finalResults) => {
    const results = await loadStuff(params)
    if (results.next) {
        return loadRecursively({...requestParameters, pagination: results.next}, {
            ...results,
            items: [...finalResults.items, ...results.items]
        })
    } else {
      return  results
    }
}

const allAvailbleResults = await loadRecursively(params, finalResults)

I found quite confusing the fact that we were initializing an empty object which is supposed to contain the final results and passing them as a parameter of the method in charge of loading. Therefore I suggested to have a default parameter in the method definition, so to hide this behaviour.

The developer told me we could not do that because the linter we are using. (XO)[https://github.com/xojs/xo] started complaining.

Default parameters should not be passed to a function through an object literal. The foo = {a: false} parameter works fine if only used with one option. As soon as additional options are added, you risk replacing the whole foo = {a: false, b: true} object when passing only one option: {a: true}. For this reason, object destructuring should be used instead.
Linter rule

I quickly tried out a few examples in the console

const doStuff= (options = {a:false, list:[1,2,3]}) => { console.log(options)}

doStuff()
// outputs:
{ 
  a: false
  list:  [1, 2, 3]
}

doStuff({a:true})
// // outputs:
{a: true}


doStuff({somethingElse:"nope"})
// outputs:
{somethingElse: 'nope'}

So far nothing extraordinary. Everything works as expected. so where is the problem?

The problem address by the linter is exactly that if you pass an object that has a different structure, the default is simply ignored.

This might be the behaviour you want, but likely it is not.
Because if you defined such defaults it probably means that your method, or whoever is using the result of your method
is relying on those values (or at least properties) you are setting as default and if they are not there, because who invokes the method decides to pass an object with a different structure, something will crash.
Therefore using destructuring ( with defaults ) within the method is safer.

const saferDoStuff= (options) => { 
const  {a=false, list=[1,2,3]} = options 
console.log(a, list, options)
}

saferDoStuff({a:true})
// outputs:
true, [1, 2, 3] {a: true}


saferDoStuff({somethingElse: "nope"})
// outputs:
 false, [1, 2, 3] {somethingElse: 'nope'}

As you can see, by passing a parameter with a completely wrong/invalid/unexpected structure, the method still works because it can rely on the properties a and list and their defaults.

This is actually a case where Typescript really proves its point, because it would be immediately clear to the who is passing an object with different structure that the are making a mistake, and the method would crash otherwise.

type Options = { 
a:boolean,
list: number[],
somethingOptional? : string
}

const typedDoStuff = (options: Options ) =>{
console.log(options)
}

Attempting to use typedDoStuff({somethingElse: "nope"})
would not be possible because Typescript immediately complains about the incoherent type being passed!

Of course we did not switch to Typescript because of this reason, but by taking a little bit of time to understand the linter rule, we learned a bit more about possible, quite nasty bugs that could occur in our application and we were able to refactor the method even more.
In fact in the end, we realised that we did not even need that nested object as initial param and that passing around an array was more than enough, simpler and more readable.

const loadRecursively = async (params, previousResult = []) => {
    const results = await loadStuff(params)
    if (results.next) {
        return loadRecursively({...requestParameters, pagination: results.next}, [...previousResults, ...results.items]
    } else {
      return  results
    }
}

const allAvailbleResults = await loadRecursively(params)

Sometimes it is worth it, not to concentrate only on the lines of code that you are touching/fixing but expand your understanding of the context and improve the overall coding experience and application.

Be a good boy(or girl)scout!

Comments

[Michel Francis]

Great post on AI code reviews! To stay sharp, I take quick breaks with astronuts.io, It's a fun way to recharge between sessions without losing focus. Highly recommend!