How to write clean code, practically

People often talk in the abstract about refactoring while writing code. But doesn’t that mean you just have a huge pull request with lots of unrelated changes muddled together? Or worse, several separate pull requests that conflict with each other?

It doesn’t have to be like that.

There’s an exercise I’ve used with great success which trains you to write code in the right order. It’s called Baby Steps. You start a 2 minute timer and start making your change whatever way seems sensible. When the timer goes off, if you have compiling code with green tests you may commit, otherwise revert. After all, you’re only losing 2 minutes of work. The idea is not to get faster at pressing the keys the next time around, it’s to learn what was blocking the change you tried to make, and focus in on that next iteration. You might need to keep note of why you’re doing various tasks and how they fit into the bigger picture – this methodology was formalized by the Mikado method.

When doing this, you’ll often find you’ve written a whole sequence of changes which are pure refactors*. You could pull request and merge them without actually changing the behavior of the application, but they change the shape of the code to make space for the feature/bugfix you’re currently working on. By the end of this process, your feature/bugfix can be a simple single-line fix. You should also find you hit the timer less often as you get used to identifying smaller problems mid-way through and resetting early. I’m not going to say you should always work to a 2 minute timer, but I’d certainly advocate the overall style of development. Taking turns at driving when the timer goes off works great in a pair to keep you both engaged. Note that this doesn’t replace other methodologies. If it’s a big change, spike it. Still use TDD, BDD or whatever you’re using this week to drive your development.

To summarize, use pure refactors* first to make space for your the changes you’re working on. Then make your behavior changes separately. Baby Steps is a great way to practice doing this that works on real code.

Refactoring an API? Try Expand, Migrate, Contract refactoring.

* By “pure refactor” I mean that the behavior of the application doesn’t change. I haven’t added features or taken them away. I would advocate all refactors being done this way, but recognize the term “refactor” is sometimes used just for general code changes.
Thanks to Alastair Smith for pointing out the links that inspired this post
Advertisements
Posted in Uncategorized | Leave a comment

Semantic versioning APIs of .NET assemblies

Some time ago I wrote a post on this topic, and decided it was time for an update.

Recap

  • Package managers are awesome.
  • Having many small libraries is good design.
  • Updating libraries in a complex dependency graph is annoying.
  • Throwing away compile time safety (i.e.using binding redirects) is not the answer.
  • Setting the .NET assembly version the same for assemblies with compatible APIs can alleviate this pain.
  • Let’s automate it!

I also previously described a way to version your versioning system, which is fortunate, because I’ve decided it’s due an upgrade.

Why change the old system?

Firstly, the old system didn’t reward efforts to avoid source-level breaks, i.e. ensuring dependent projects don’t need to change their source code even if they have to recompile. This means changes are either saved up, and then really painful all at once, or a constant stream of breaking changes become the norm. Even figuring out what those cases are needs careful thought to get right, and automatically detecting them needs a lot more effort.

The second reason is more fluffy. There’s something slightly unsettling about a library’s major version increasing rapidly. However many iterations you go through, that version never goes back down, and it just looks weird.

New system

The new system has a Nuget Package version of Major.Minor.Patch and an assembly version of Major.Minor.0.0. The assembly file  version can still be whatever you want, but Major.Minor.Patch.0 seems sensible to me. Here’s the rules for when to increment those version parts:

  • Increment Patch when the API is a superset of build with a compatible API
  • Increment Minor when the API has changed in a way that breaks source compatibility (requires recompilation), but is unlikely* to require source code changes to compile.
  • Increment Major when the API has changed in a way which requires recompilation and is likely* to require source changes for consuming code to compile.

* The definitions of likely and unlikely are a bit flexible here and I’d recommend erring on the side of caution. I would define it as “there is no common coding pattern for consuming the changed member which will be broken by the change”. I would like to refine this over time, but here are a few examples of what I’m thinking about.

  • Patch: Nothing changed at all, or only internal/private things changed.
  • Patch: All the existing public members remain and some more were added.
  • Patch: A default parameter’s value has changed. Default parameters are inlined at compilation time, so it’s not part of the method signature. However, depending on the change, it may be worth making this a minor/major version bump since it’s changing behaviour in a subtle way.
  • Minor: A public class now implements an interface that it didn’t previously.
  • Minor: A public static method now returns type T instead of interface I where T implements I.
  • Minor: A public method has turned into an extension method in the same namespace. It’s possible there’s an extension method with the same name in consuming code, but that seems unlikely.
  • Major: A public method now returns a long instead of an int. Some uses could still work, but if a calling method doesn’t use “var”, or returns the variable itself recompilation would be needed. This seems likely.

This encourages people to make changes in a backwards compatible way without completely setting the API in stone, and means that consumers get a grace period to react to things becoming obsolete.

Consuming packages

A library which itself has dependencies, should regularly update to LatestMajor.LatestMinor.0. This is the least constraining for any consumers since it means a consumer is free to use any patch version from 0 to LatestPatch, rather than being forced to upgrade for every single bugfix.

An application at the top of the dependency chain is able to update as often as it likes, to any version, though it will find that only certain ranges of dependencies are compatible with its other dependencies. I’d recommend regularly updating everything to the absolute latest version.

What I aim to do for my own libraries in future, is have them automatically build when a dependency releases a new major/minor version, attempt the update to LatestMajor.LatestMinor.0 and commit it back to source control if it builds and passes the tests. On TeamCity for example, it should be possible to just do the update/commit as a build step that commits to a branch called dep/{name}/{version}, and have TeamCity auto-merge branches matching dep/* that pass the tests.

Setting the version automatically

For the old scheme from the previous blog post, I had a tool to detect what the semantic version should be. I am currently rewriting that tool to work under the new scheme (using Roslyn). At first, everything will just be either a major, or a patch bump. Ideally there’d be an easy manual override to classify something as a minor version. This would allow it to be used for a custom scenario like “No-one actually ever calls this method – I’ve checked”. Over time, I hope to add detection of common changes which should only increase the minor version. I also intend to build other tools to help make this scheme very convenient, for example storing a summary of the public API making it easy to see changes over time.

You can follow my progress at: https://github.com/GrahamTheCoder/Semver-AssemblyCompatibility

 

 

Posted in C# | Tagged , , , , , , | 1 Comment

How not to use dependency injection frameworks

Just like any tool, a DI framework can be misused. Here’s a quick guide to understand why you might want such a tool and how to avoid shooting yourself in the foot with it.

The goal

Inversion of Control (IoC) is a very general concept which I’d describe as ‘shifting responsibility’. The goal is to make the code reusable without having to think of every possible use up front. Essentially, the Open Closed Principle.

For example, using a test framework like NUnit means that the framework (rather than your code) has control over when your code is called. By keeping the mechanism of running the test separate from the test itself, your code will be reusable with a different test framework.

Another example (more relevant to the main topic) is making a class’s dependency configurable, so rather than a class Logger calling File.WriteAllText, it might call WriteText on a member of type IPersistentStore. If the behavior of the IPersistentStore is controlled by something outside the class, then we’ve inverted control of what the Logger will achieve, or shifted the responsibility of where the logger persists the given data. Here are two examples of Dependency Injection in action:

public interface IPersistentStore
{
    void WriteText(string text);
}

public class Logger
{
    public IPersistentStore PersistentStore { private get; set; }

    public Logger(IPersistentStore persistentStore)
    {
        PersistentStore = persistentStore;
    }

    public void LogError(Exception exception)
    {
        PersistentStore.WriteText(exception.Message);
    }
}

Obviously, only one of the solutions (public setter, or constructor) need be used. The danger of the public setter is that the caller may forget to call it – therefore you may want to set a sensible default if you use this style. I’m personally of the mind that a constructor should take everything required for the object to work correctly.

Another method which I should mention is the Service Locator Pattern (SLP). Long story short, you introduce another layer of indirection by injecting an object that knows how to locate/create a concrete instance of an IPersistentStore. It’s considered an anti-pattern by many so beware of hiding dependencies and turning what would otherwise be compile-time errors into runtime failures using it. The factory pattern will probably serve you better.

Side effects of using constructor injection and the factory pattern may include:

  • Writing code against contracts of interfaces rather than implementations that are subject to change. i.e. Loose coupling.
  • Fewer ‘just in case’ configuration parameters added for possible future functionality.
  • More testable code – as different varieties of test doubles can be used.
  • Confusion when this concept is over-applied, as you trawl through 20 classes and interfaces just to print Hello World.

Using frameworks

Dependency Injection frameworks are tools that can cut down on some of the boilerplate code that you might end up writing to achieve the above goal.

Some pretty well-respected people dislike DI frameworks. That should be a good hint that there are at least some pitfalls to avoid. Since this is an area where people disagree the most important piece of advice I can give is to leave yourself open to changing your mind later. You should always consider the cost of switching when starting to use a new technology.

One of the main objections I’ve heard is from people who have locked themselves into their DI framework by scattering references to it all over their code. This is not necessary to get them to work but is easily done, and is a slippery slope once you’ve started.

If you are doing DI, and you’re hitting problems like:

  • Too many very similar looking factories.
  • Implementing singleton patterns all over the place.

Then a good DI framework may be able to help you.

Many frameworks have various “automagic” binding options which match things up based on name. Switch them off, do not be tempted. You will be scattering an important responsibility all over your codebase and putting constraints on choosing appropriate names for the context.

Initialization is a responsibility in its own right, so it should have its own classes. Ideally you should write down these “bindings” of concretions to abstractions as “high up” as possible. I’d also encourage you to put all the bindings in a separate initialization phase of your application which runs before any normal application actions occur. This way you can test that phase independently.

In summary:

  • Use constructor injection
  • Segregate interfaces
  • Write and test your application initialization as its own responsibility
  • If you use a DI framework:
    • Switch off the automagic features
    • Minimize your dependency on it (especially static classes)
Posted in Principles | Tagged , , , | Leave a comment