CodeSOD: Reasonable Lint

While testing their application, Nicholas found some broken error messages. Specifically, they were the embarassing printing out JavaScript values" types of errors, so obviously something was broken on the server side.

Oh, that can't be," said his senior developer. We have a module that turns all of the errors into friendly error messages. We use it everywhere, so that can't be the problem."

Nicholas dug in, and found this NodeJS block, written by that senior developer.

const reasons = require('reasons');const handleUploadError = function (err, res) {if (err) {var code = 500;var reason = reasons([{ message: 'Internal Error'}])if (err === 'errorCondition1') {code = 400;reason = reasons([{message: 'Message 1'}]);} else if (err === 'errorCondition2') {code = 400;reason = reasons([{message: 'Message 2'}]);} else if (err === 'errorCondition3') {code = 422;reason = reasons([{message: 'Message 3'}]);// else if pattern repeated for about 50 lines// ...}return res.status(code).send({reasons: reasons});}res.status(201).json('response');};

We start by pulling in that aforementioned reasons module, and stuffing it into a variable. As we can see later on, that module clearly exports itself as a single function, as we see it get invoked like so: reason = reasons([{message: 'Internal Error'}])

And if you skim through this function, everything seems fine. At first glance, even Nicholas thought it was fine. But Nicholas has been trying to get his senior developer to agree that code linting might be a valuable thing to build into their workflow.

We don't need to add an unnecessary tool or checkpoint to our process," the senior continued to say. Just write better code."

When Nicholas ran this unnecessary tool", in complained about this line: var reason = reasons([{ message: 'Internal Error'}]). reason was assigned a value, but it was never used.

And sure enough, if you scroll down to the line where we actually return our error messages, we do it like this:

return res.status(code).send({reasons: reasons});

reasons contains the library function we use to load error messages.

This code had been in production for months before Nicholas noticed it while doing regression testing on some of his changes in a related module. With this evidence about the value of linters, maybe the senior dev will listen to reason.

[Advertisement] ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!