Home Home Posts Rants about IT DevOps Stuff I'm working on

Linters and style checkers: how to save hours of painful debugging

Arseni Mourzenko
Founder and lead developer, specializing in developer productivity and code quality
112
articles
August 25, 2015

I'm always impressed with the consistency of screw-ups caused by the unwillingness of programmers to follow basic guidelines. To be more specific, I'm talking about my own screw-ups here.

I've been convinced a long time ago that:

  • Style checkers and linters should be used consistently on every project,
  • Style-wise, non-compliant code has no place in the source repository,
  • Linters which are too slow for pre-commit hooks should be run by CI server,
  • Such checkers should be set at highest strictness level, which makes the choice between the jsLint and the jsHint very easy, since the last one is happy to let you introduce bugs, since even the article explaining why jsHint is better contains buggy code.
  • Anything revealed by those checkers should be qualified as a error, not a warning, that is a second-class citizen which has no actual meaning in development.

Following rules enforced by style checkers and linters is not that difficult. It is amazing to see how easily those checkes prevent us from introducing subtle, difficult to find bugs. For example, I'm absolutely incapable of finding a bug in this piece of JavaScript code by just glancing at it if I omit style conventions:

var buildInit = function () {
    // Do something here.
    return
    {
        shell: true,
        acceptRange: null,
        threshold: findThreshold()
    };
};

And still, here it is, the ugly bug which could be found in less than a millisecond by a linter, and which makes the function return undefined.

But I'm not here to talk about JavaScript automatic semicolon insertion. I'm here to talk about C#, and more precisely about a particular aspect of it. If you have used C#, you have probably noticed the ArgumentException/ArgumentNullException inconsistency. Here are their respective signatures:

public ArgumentException(string message, string paramName)
public ArgumentNullException(string paramName, string message)

Doesn't it look a lot like PHP's haystack-needle mess? Well, there is a good reason for this order of arguments. In the case of ArgumentException, one has to specify a message, because .NET Framework doesn't know what is exactly wrong with the given argument. On the other hand, there is no need to specify a message in ArgumentNullException, because .NET Framework is capable of generating it on its own, at a condition to have the parameter name, so it's more often used this way:

public ArgumentNullException(string paramName)

Still, I was never particularly happy with code which looks like this:

if (summary == null)
    throw new ArgumentNullException("summary");

if (!summary.IsValid)
    throw new ArgumentException("The summary should be marked as [...]", "summary");

if (price == null)
    throw new ArgumentNullException("price");

It doesn't feel right. Thanks to named arguments, we can make it better:

if (summary == null)
    throw new ArgumentNullException(paramName: "summary");

if (!summary.IsValid)
    throw new ArgumentException(paramName: "summary", message: "The summary [...]");

if (price == null)
    throw new ArgumentNullException(paramName: "price");

This piece of code is clear and much less risky. The same technique was successfully used in different forks of Orseis to mitigate the risk of this type of bugs.

But I wasn't consistent across projects. I wasn't using the same technique everywhere, which lead to the following piece of code in my latest project:

Pair = new Pair<IAssembly>(
    new Assembly(this.FindAssemblyPathOf<WebClient>());
    new Assembly(this.FindAssemblyPathOf(typeof(WebClientProxy))));

I have completely missed the fact that Pair<T>—which was another class from the same project—was expecting two arguments: a proxy assembly, and only than a target assembly. In the piece of code below, I was passing the target first, and only then the proxy.

The result was weird. I spent two and a half hours trying to understand what is happening, with no hint whatsoever: code from unit tests seemed to work, while this code of the system test which is not very different just fails with unhelpful error message.

I finally found the issue. The corrected version of the code looks like this:

Pair = new Pair<IAssembly>(
    proxy: new Assembly(this.FindAssemblyPathOf(typeof(WebClientProxy))),
    target: new Assembly(this.FindAssemblyPathOf<WebClient>()));

If only I had a static checker which could force me to use named arguments whenever the method accepts the parameters of the same type, those two and a half hours could be shortened to a few milliseconds. As simple as that.

The morale is that while it might seem cumbersome to use style checkers and linters, they actually save hours of debugging. Not following basic guidelines because of laziness like I did is just unprofessional.