Disclaimer: The opinions expressed herein are my own personal opinions and may not represent my employer’s (Infozone) view.
I’ve recently been in projects with strong enforcement on code style using StyleCop (C#) and TsLint (Type Script) and I feel the need to write about my experience and try to reason about the pros and cons of linting in general and the pain I’ve felt adhering to some of the rules. In addition I will touch on the topic of the use case of linting and where/how to use it.
What is linting
Linting is statically checking your code for potential errors, both programatical errors and stylistic errors. The idea is to minimize the risk of bugs (programatical checks) and to streamline the style of a code base (stylistic checks). There are numerous linting tools out there and to mention a few – JsLint, TsLint, CssLint and StyleCop.
Linting has good intentions
All in all I do believe that the intention behind any linting tool is good, it strives for a code base that has a reduced number of bugs and has a consistent look and feel. It has good uses when the development teams are large, and possible, spread out. Another good candidate to use linting is when working in dynamic languages where static analysis otherwise is limited. I see linting more as a sanity check rather than strict rules.
All this apart, I do see issues in how linting is applied in practice.
Linting in practise – frustration and consequenses
All projects that I’ve worked in that have used some form of linting have used it as part of the build, usually in the CI (Continous Integration) process, with treat errors as build errors. Let’s halt for a second and reflect upon what I just wrote – linting as a build step. Treat linting errors as build errors. This means that if a linting rule fails, the build breaks, it means that the new version will not be deployed/published/committed. You might wonder why this is a bad thing, and it might not be, but it most likely is, and here is why.
If the rules for linting is carefully crafted to only fail on things that matters, things of importance, then breaking a build as a cause of linting might be reasonable. Things that matters are rules that checks for potential bugs, i.e. programatical errors. All other rules, i.e stylistic rules, are only guildelines and should not be enforced as part of a build. Imagine if a deploy failed because of an extra space before a comment in the code or that some private field was placed after a public one (yes, this actually happens). Surely, this cannot have been the intent when introducing linting to a project and surely this does not mean that the code is not production ready?
Emphasize good design over code style
Linting, to me, usually gives an inaccurate picture on what is important in a code base. Focus should not lie in code style but rather on good design. A great tool here is code reviews which gives the opportunity to talk about the code, to reflect about the code and to discuss it. This is to me far more important and should, if any, be enforced before letting code out into production. Code reviews will also spot style issues easily and can easily be corrected by the reviewer. As time progresses, developers will be better and better on following the projects style guidelines. Linting can be a good aid apart from code reviews, but I’d recommend sticking with a carefully thought through set of rules, rules that actually matters.
Rules that matters are, to me, programatical rules that avoids potentially harmful code. Examples of this can be duplicate declarations of a variable, the lack of strict usage in JavaScript, not all code paths having a return value, and so on. These type of rules matters since they can help prevent sloppy mistakes. Stylistic rules should only be used as guidelines (in lack of a better word) and can be defined as a separate linting process that can be run manually before checking in code.
Frustrations with large rulesets
I’ve read somewhere on StackOverflow that “the larger the ruleset, the less prone a developer is on following them“. Often, projects use the full suite of rules just because the more the better or maybe out of lazyness. This really handcuffs developers, forcing them to wrestle with meaningless code style rules instead of focusing on good design and actual functionality. I curse on the inside each time I have to rearrange using statements so that they can be in alphabetical order before the code is production ready.
I’ll list some of my pains from the real world here below:
- Global Company level rulesets that cannot be overridden on project basis. Seriously, different projects have different needs, different experience and different people. Trust them to do their job.
- StyleCop does not support C# 6 meaning that one cannot use new language features like string interpolation, expression bodies etc.
- Totally meaningless rules:
- The variable name ‘xyFields’ (not the actual name) begins with a prefix that looks like Hungarian notation. Remove the prefix or add it to the list of allowed prefixes.
- All non-static readonly private fields must be placed before all non-static non-readonly private fields.
- Using directives must be sorted alphabetically by the namespaces.
- There must be one white space character between code and comment
Talk about focusing on the wrong thing? Looks like hungarian notation? Well, it is not hungarian notation, its a commonly accepted business abbrevation. Don’t guess my variable name intentions.
Summary
If you are going to introduce linting in your project, spend time in finding a good rule set that actually matters. Do not enforce style rules as a part of your build process (unless the rules are few and important) as style will not introduce bugs. Make sure that the linting rules does not hinder your developers ability to do their job and focus on good design and functionality. Do trust in your team. Favour code reviews over stylistic linting.
It is OK to have slightly different flavours in a large code base in terms of style, afterall, you will almost certainly see different flavours in how developers write code anyway (naming, for vs while vs foreach loops and such).
