Minimize the amount of changes in a changeset

When creating a new change set, always try to minimize the amount of changes it introduces.

Do this by only modifying what needs to be modified to implement a feature or fix a bug. Keep formatting and refactoring changes for another commit.

The story of how I came to this practice

Many years ago I read Clean Code. One of the things it discussed was to check in our code a little cleaner than when we checked it out. This meshed with the idea of continual improvement through refactoring which I was already following.

I took this principle to heart and made formatting changes, introduced constants, removed commented code and made small refactorings with my change sets. I kept big refactorings for different commits… well most of the time anyways…

Then later on an open source project I contributed to, I was asked during code reviews to keep those changes out of the change sets. I was surprised since I thought I was helping improve the code’s quality. The project’s maintainers reasoning was that the change set needed to include only the changes required to correct the bug that was addressed or implement the new feature we were working on. Refactorings should be kept as different endeavors or for instances where a piece of code was being completely rewritten during the course of normal work.

Over time I started to see this made more sense than my previous practice and eventually started following this practice as well.

One thing I like to do now is keep notes of what changes I want to make, complete my current work and then make a separate commit for formatting and refactoring changes. If I don’t take notes, too often I forget all the little changes I wanted to do and never get around to doing them.

The why

When a commit needs to be read later on to understand the changes, revert them or when doing a blame/annotate to find out why a change was made it’s much easier if all the changes are related to the commit message and associated ticket number.  Otherwise you need to reason for each modification if it was done in relation to the commit’s purpose or as a “bonus” refactoring.

Also, including unnecessary changes makes for longer commits which make for longer code reviews. In my experience the longer the review, the more chances some stuff will get glanced over.

Finally, every change we make, even when having a robust test suite risks introducing regressions. If you later find out your latest changes introduced a bug and it turns out it was in a refactoring, it’s much easier to revert or correct this refactoring without needing to revert a bug fix or feature at the same time.

Never let stray errors in the console

When doing web development I fix all the errors that show up in the console. Often they don’t seem to impede the actual use of the software, but nevertheless I think it’s a good practice to fix them all. This following example should illustrate why.

I was doing some maintenance on an Angular project at work. I noticed that there were a couple of errors showing up in the console, but the web site still worked great, even with the errors.

The reason I was working on this project was because of a bug that was reported by QA. There was a display error that was happening and it was related to animations that were used in the Angular transitions, these are used when switching from one component to another. The error manifested itself with Android phones and iPhones. This bug probably slipped by because everything was working fine on desktop browsers.

After investigating, I saw that there was an error in the console about calling a property on undefined. On a view, this can be fixed using the ?. operator. The view used the ?. operator elsewhere but there was one place it was missing. I was sure this couldn’t be what was messing up the display of the animated transition.

I searched and searched but couldn’t find the problem.

So I decided that while I was there, I might as well fix this error that showed up in the console. I changed the . operator for ?. and lo and behold this also fixed the display problem.

By correcting it, it also corrected the other problem, which was probably caused by having an error during the Angular rendering.

This example demonstrates that you should never let console errors live, even if they don’t seem to have any negative effect on the web site.

Bug reproduction steps

To properly fix a bug, you must be able to reproduce it. If the developers can’t reproduce it they can only guess at its cause and their corrections are just potential fixes that can’t be validated.

If someone writes a report for a bug they can’t reproduce, how can they validate that the developer assigned to the task has satisfactorily fixed the problem? They can’t. So why report and track something that can’t be closed except by a leap of faith?

Maybe because the developer will find the reproduction steps? They might, but then again who stands a better chance of doing this?

The best person to reproduce a bug is the person who first encountered it. To give an analogy, it’s like if you found a secret glade in a forest and you can’t find it again. So you ask someone who’s never been there to find it for you and you don’t give them any directions. Even if they find one on their own, is it the same one you found?

Another situation is the QA or reporter can reproduce it, but the developer can’t. This might be because the reproduction steps aren’t clear or just missing. It could also be that there is a difference between the environments or actions of the reporter and the developer.

Sometimes the bug reporter has produced the issue but hasn’t tried to reproduce it consistently. They write down with they did but haven’t tested their steps further to see if they reproduce the original results every time.

All of these situations can be greatly mitigated by writing good reproduction steps and testing them before submitting the bug report.

How to write reproduction steps:

• Most bugs report should include reproduction steps. If reproduction can’t be achieved, in most cases, testing should continue.
• Steps should be clear and concise.
• Each step should be absolutely necessary to reproduce the bug. This removes clutter but also forces the reporter to distill the bug to it’s most minimal conditions, which helps to define it correctly.
• No assumptions should be made while writing the steps. This can be very hard as we all have implicit assumptions that are not apparent at first glance. For example, if a user was created for testing purposes, how was it created? What sort of user is it? Is the username included in the report?
• Before submitting the report, the reproduction steps themselves should be tested. Did they reproduce the issue? Were some required operations omitted because the seemed evident? For example if navigation to a screen occurred and there are several ways to navigate to this screen, which one was used?
• During the testing of the reproduction steps the scenario should be altered a little. Does the scenario still reproduce the bug with the alterations. If so the altered steps are either unnecessary or could be generalized further. Examples of this could be testing with a different user or changing the sequence of some operations.

In addition to the reproduction steps bug reports for mobile applications should include the OS and device name.

Bug reports for web applications should include the browser and it’s version in the case of Internet Explorer or non evergreen browsers.

All bug reports should include the version number of the application that was under test as well as the environment the bug was found on: dev, qa, prod.

Taking these steps will minimize the back and forth between QA and development and will reduce the time required to close bugs.

 

SonarLint Visual Studio Extension / C# Linter

I’m currently working on a legacy code base that’s not a shining example of clean code and best practices.

I try to improve the code as I work, a careful balancing act where I do my best not to impact productivity by spending too much time on refactoring and reformatting.

To help me with this task I decided to look into linters. Note that I’m not using ReSharper which would help with code quality, but that’s another story.

I tried StyleCop, but with the default settings I just wasn’t impressed. Many rules are in my opinion needlessly restrictive. It also seemed to simply dump more than a thousand warnings in the error window.

I tried SonarLint next and I must say I was thoroughly impressed. Though the errors are also sent to the error window they are also shown inline in the file. The parsing by SonarLint was fast and running the Visual Studio extension only had a very small impact on performance even while working with a solution containing thousands of files and hundreds of thousands of lines of codes.

While I haven’t run ReSharper in years, performance was one of the two reasons I stopped using it. On large solutions like the one I’m currently working with, it would slow everything down to a crawl, even when disabling solution-wide analysis.

SonarLint isn’t exactly what I was expecting it to be. I thought it would be more for style guidelines but it has instead given me deeper insights. I’m specifically referring to my last two examples.

Things like needlessly nested if statements:

if (FirstProperty == false) 
{
    if (NeedlesslyDeeplyScoped == true)
    { }
}

Using a Count() instead of an Any:

if (files.Count(f => f.IsDeleted) > 0)
// to
if (files.Any(f => f.IsDeleted))

To foreach statements that can run on null variables:

List<Foo> myCollection = null;

if (IsWeatherRainy) 
{
    myCollection = new List<Foo>();
    // add stuff
}

// myColllection null on at least one execution path
foreach (var item in myCollection)

Or overlapping method signatures with optional parameters that can’t be used:

// This method's signature overlaps the second one
// and the default parameter value can't be used
public File GetFile(bool firstParam, 
                    bool secondParam = false)
{
    return new File();
}

public TestFile GetFile(bool firstParam)
{
    return new TestFile();
}

This last one having been found three times in a single file.

SonarLint is available on the Visual Studio marketplace. It’s free and open source with a repository on GitHub. It’s created by a company that also sells a similar product (though geared towards CI / Build servers) but I haven’t found any downsides to using the free Visual Studio Extension.

By the way, I haven’t been asked to write this by the company that makes SonarLint.