I was recently part of a team implementing a new cloud service. One of the objectives of this team was to frequently release software to production. This might be a minor patch or the introduction of a significant new feature. After some careful investments in automated builds, deployments and tests, we were able to accomplish this goal. We could essentially release at any time, as long as all of our automated tests passed.

An important part of our process was a gated check-in for the main branch, which we used to build production releases. To pass the gated check-in, all the code had to compile, pass static analysis, pass all the unit tests and conform to the coding style. Enforcing a coding style might seem a bit rigid, but I feel that it was an important element of our process and resulted in a higher quality product. But it does not come without drawbacks. After having lived with the process for approximately one year, I'll describe some of the advantages and disadvantages I have observed from enforcing a coding style as part of a gated check-in.

Since we were using C# and Visual Studio, we used StyleCop to define and enforce our coding style. StyleCop was included via a NuGet package in each project and we had one global StyleCop settings file at the root of our source directory. We decided on the StyleCop rules as a team over the course of a couple of meetings where we were reviewing a lot of code. I feel that the rules we settled on were pretty lightweight, mostly concerned with formatting (e.g., whitespace and the use of braces), class layout (e.g., the ordering and grouping of fields, methods, properties, etc.) and member naming (e.g., PascalCasing for method names and camelCasing for local variables, etc.). We stayed away from esoteric rules like every member must be commented. We felt rules like this encouraged counterproductive behaviours like this:

public class SomeResource
{
    /// <summary>
    /// The Name
    /// </summary>
    string Name { get; set; }

    /// <summary>
    /// The ID
    /// </summary>
    int ID { get; set; }
}

The Drawbacks

I think the drawbacks were, generally, few and far between. At first, it was certainly a bit annoying when I was focused on coding a feature and my check-in would be rejected because of some trivial style issue. This took some getting used to but eventually we all got in the habit of running StyleCop in addition to static analysis locally, before committing our check-in. This really kept the focus on the fact that our main branch had to be production-quality code that could be released to our production environment at any time.

I think the biggest drawback was interpersonal. Even though we discussed the rules as a team and I felt that we had defined our style guide by consensus, some rules were more appealing to some people than other rules. Some people found certain rules too rigid and the fact that these rules were strictly enforced (if you didn’t follow the rule you couldn’t check in) created resentment. I think one should be especially sensitive to this if there are differences in age or programming backgrounds among the team members. It is also probably more of an issue on a newly formed team, rather than a team that has worked together for years and has a well-established style. Deferring to an industry standard style guide (like Framework Design Guidelines or MSDN for .NET) can certainly help. The defaults of tools like StyleCop and ReSharper are also pretty good places to start. Compromising and reaching consensus on items like this, however, is ultimately part of team building and, in retrospect, this was probably more indicative of other issues that needed to be addressed rather than being the problem in and of itself.

One technical problem that we had was to make sure each project included the StyleCop NuGet package. When adding a new project it would be easy to forget to include the StyleCop NuGet package and, if so, our style guide would not be applied to that project on check-in. We had already customized our build to interrogate project setup ahead of compiling to ensure, among other things, that static analysis was correctly configured, so we used this to also check that StyleCop was included. It would be nice if there were facilities in the automated build to support such global configurations without having to resort to customizations, but we were not aware of any at the time. Using a check-in policy might be an interesting alternative.

Code Review is About Code Review

I think the biggest benefit was that enforcing a consistent style as part of the check-in made subsequent code reviews strictly about code review and not about style. This made for more effective code reviews. When I would code review another class it was immediately familiar; the code was formatted the same way I would format it and I was not distracted by minor stylistic concerns. It is also easy for someone to "contribute" to a code review by pointing out these tedious stylistic concerns. This can feel nitpicky and judgmental too which can make people defensive or withdrawn. And it doesn't make the code better, other than perhaps making it more maintainable. I like what Erik Dietrich has to say about code reviews in his article How We Get Coding Standards Wrong:

Keeping methods and classes small and focused, principles like DRY and SOLID, and other good design principles are much more important standards to which to aspire, but they’re often less concrete and harder to enforce. It’s much easier and more rote for a code reviewer to look for casing issues or missing comments than to analyze code for good software practice and object-oriented design. The latter is often less cut-and-dry and more a matter of degrees, so it’s frequently glossed over in favor of more tangible, simple things. Problem is, those tangible, simple things really aren’t all that important to the health of your applications and projects over the long haul.

Code reviews should be about code review. Does the code do what is intended? Are exceptions handled appropriately? Have authentication and authorization been considered? Is there encapsulation? And so on. Things that affect the functioning, reliability, security, performance and maintainability of the code. These aspects can take great concentration and stylistic distractions only detract from an effective code review. So we basically used a tool to take care of the “simple things”, as Erik calls them, before the code was even committed to source control.

I noticed lately when looking at a class that was not part of our project, I was distracted by extra whitespace and properties intermixed with private fields. I don't think there was actually anything functionally wrong with the class, but because the style was not consistent it made it much more difficult for me to understand.

Maintaining consistency for things like class layout is tedious and I don't want to have to memorize these, arguably arbitrary, rules. I want a tool to check these and provide reminders so that I don't have to think about it. ReSharper is an excellent tool for providing these reminders in context as you code. Plugins like Visual Studio Productivity Power Tools, which will format code when the document is saved, can go a long way to automating stylistic concerns as well.

Adding People to a Team

We never had an opportunity to put this to a test, but I do feel that having the basic stylistic conventions of the team codified in a tool would be useful for on-boarding someone new to a team. The new person can learn the conventions naturally as he/she works on code and contributes to the project. A build warning that the person can work on by themselves is more gentle than being told "don't do that" in front of the team. Isn't that a nice way to on-board with a team? Start working on code and don't worry too much about the style. If the style doesn't follow our conventions the build will remind you. It is also more productive in that the person can just learn on their own without having to have someone mentor them in adopting the conventions of the team. It also means that the conventions are not some secret handshake known by the elders of the group and passed on to the next generation.

An In-Context TODO List that Actually Gets Done

One trick I used when experimenting with changes that I didn't necessarily want to check-in, or when I wanted a reminder to do something before checking in, was to annotate the relevant code with a //TODO comment. StyleCop will complain that all comments must start with a space (i.e., // TODO), so I would be safe in knowing that these changes would not pass the gated check-in without me resolving them first. I found myself regularly using this trick as an excellent way to make quick to-do annotations right in context without breaking my flow. This allowed me to be very thorough, especially remembering to write tests for edge cases.

Conclusion

I’m no longer working on a team with a gated check-in, or even a strict coding style. I certainly miss it. I miss the consistency and quality it brought to our code. I think it made us more productive and improved the quality of the product. Perhaps all it did was contribute to a culture along the lines of Vince Lombardi’s famous quote:

Perfection is not attainable. But if we chase perfection, we can catch excellence.

We had the luxury of being a new project, so enforcing style as part of our check-in gate was relatively easy. Doing this for a project with a significant amount of legacy code, or dependencies that may be out of your control, could be daunting, or near impossible, and not worth the investment. But perhaps try it for new code added to your project and see how you like it.