Code that fits in my head – part 2

For the second article in this series highlighting some lessons from Code That Fits in Your Head: Heuristics for Software Engineering by Mark Seeman, I am going to look at testing, and at ways of lowering the complexity of your code or to put it in another way: how to prevent your code from becoming more complex over time.

Testing

Tests are written to gain confidence that the production code works, ensuring that when someone requests a change down the line, other behavior still works as expected. Tests can be written with various scopes in mind. Unit testing can be used for business logic, where only a small area of the code base is considered. Another is outside-in testing, where the whole application is tested from the outside, as if a user interacted with it. This test covers the entire application vertically, thus covering the same logic as some unit tests.

When you write tests, always make sure they can actually fail. If you have not seen the test fail, how do you know that it is actually testing anything? You might have made a typo or misunderstood how the testing framework works. If you write a test for an already implemented feature, the test should obviously pass. However, check that it is capable of failing by changing the system under test. This can be done by commenting out code or returning some hard-coded value.

If the implementation is driven by tests, maybe through a process like Test Driven Development (TDD) then the Devil’s advocate technique might be useful. The Devil’s Advocate technique is about writing an implementation that makes the test pass but is clearly wrong. This forces you to make the test more specific or add more tests until a proper implementation is reached. This can be seen as a form of triangulation, helping you drive the desired behavior from tests.

Tests should be informative when they fail. For example, let’s assume a test that checks whether the return value of some method call is equal to, let us say, the number 14. If this test fails, it is not very useful if it simply says that the two values didn’t match. This might happen if the assertion was written like so:

Assert.True(myClass.GetNumber() == 14);

Instead, the test should be informative and tell us what it got and what it expected. This is a case of using the correct assertion for the job. The above can be fixed by using the Equals method.

Assert.Equals(14, myClass.GetNumber());

In the above example the test case has been structured using the Arrange, Act, Assert pattern. To use this pattern, split the test case into 3 phases. First arrange: set up mocks and variables. Then act: call the subject under test. Lastly assert: assertions and transformation of the result from the act phase, so it can be asserted. This can be applied by simply having an empty line between phases. Some people like adding comments that clearly define when each phase starts, but for small tests, such as the following example, it might not add value and simply take up space:

public void SomeTest()
{
    // Arrange
    var currentHour = DateTime.Hour;

    // Act
    var gottenHour = _sut.GetHour();

    // Assert
    Assert.Equal(currentHour, gottenHour);
}

From my own experience with mocking libraries like Moq I have discovered a way to make assertions of a method-call that takes objects as arguments easier to understand.

If you wish to check for a method-call with an object argument, do not simply use the Verify method. This will only inform you whether the method was called with the given argument or not, and not what it actually got. This can also be used if the value you wish to validate is a property on an object passed to the method. Save the value given and use the Equal method instead.

public void SomeTest()
{
    // Arrange
    var receivedValue = string.Empty;
    _someMock
        .Setup(_ => _.SomeMethod(It.IsAny<object width="300" height="150">()))
        .Callback<object>(_ => receivedValue = _.SomeProperty);
    
    // Act
    ...

    // Assert
    Assert.Equal(expectedValue, receivedValue);
}

Lowering Complexity

Code typically does not start out complicated – the complexity happens over time. As time progresses and the codebase increases, classes grow, and methods gets larger. As mentioned in the first article in this series, a piece of code should be of a size, so it fits in the head of the reader.

A concept for keeping the size of code down is Cyclomatic Complexity – a metric that is calculated by counting all instances of a loop or conditional in a method and adding 1. For example, the Cyclomatic Complexity of the code below is 2.

public int Power(int number, int raiseToNumber)
{
    var result = number;
    for (int i = 1; i < raiseToPower; i++)
    {
        result *= number;
    }
    return result;
}

When we humans read code, we essentially try to compile and run the code we are looking at in our head. As previously mentioned, humans can keep track of 4 to 7 things at any given time. Hence the Cyclomatic Complexity should be kept below 8, otherwise the method will become incomprehensible.

A way to keep the Cyclomatic Complexity down is moving pieces of code out into separate methods and classes. Extracting behavior from a method/class to a new one is sometimes known as Abstraction, which refers to the act of abstracting information away or to delegate responsivity to someone else. This not only lowers complexity but also helps keep any class/method clear and concise, and thus readable. However, it is important to know when an abstraction provides value and when it does not. Here is an example using an ASP.NET controller:

// CalendarController.cs
[HttpPost]
public ActionResult CreateCalenderEvent(CalendarEventDto dto)
{
    if (!DateTime.TryParse(dto.At, out var parsedDate))
    {
        return BadRequest("Invalid date");
    }
    if (dto.Location is null)
    {
        return BadRequest("Location cannot be empty");
    }
    
    var event = new CalendarEvent(parsedDate, dto.Location);

    await _calendar.Put(event);

    return Ok("Event successfully created");
}

In the above controller method, all validation is handled in place. Perhaps it would make sense to abstract this away so the CalendarEvent itself knows whether is valid. Then this property can be used in a conditional inside the above method and an appropriate response can be returned accordingly:

// CalendarEventDto.cs
public class CalendarEventDto
{
    ...
    private bool IsValid {
        get {
            return DateTime.TryParse(At, out _) && !(Location is null);
        }
    }
}

By doing this the parsed DateTime value is lost. The same goes for the Location which clearly is not null if IsValid is true. However, the compiler is no longer able to deduce that Location is not null as it is not in the same scope. The abstraction made sense in one context but not in the bigger picture and a value was Lost in Translation. Instead, the Validate method handles the validation but also the creation of the object. This way instead of returning a simple Boolean, it returns null or a stronger representation of the same data.

// CalendarEventDto.cs
private CalendarEvent? Validate()
{
    if (!DateTime.TryParse(At, out var parsedDate))
    {
        return null;
    }
    if (Location is null)
    {
        return null;
    }
    
    return new CalendarEvent(parsedDate, Location);
}

Code without using conditionals is a rare sight. The guard clause used can be trivial but sometimes it becomes very complex and hard to understand for new eyes. This can be solved by writing a comment above the conditional describing what the guard is checking. However, the comment might be overlooked when refactoring and might become outdated. Instead, you can introduce a variable that holds the guard and can be named so the conditional can be easily read.

public float GetEntryFee(string age)
{
    if (13 <= age < 18) {
        return 10.0;
    }
    return 14.0;
}

public float GetEntryFee(string age)
{
    var isTeenager = 13 <= age < 18;
    if (isTeenager) {
        return 10.0;
    }
    return 14.0;
}

Behavior should be placed where it belongs, so a given class or method has a clear responsibility. Do not add logic to a class that already has a valid concern. Let's look at an example using a repository only concerned with saving and retrieving items:

// ItemRepository.cs
public class ItemRepository : IItemRepository
{
    private readonly IEnumerable<Item> _list =  new List<Item>();

    public void Put(Item item)
    {
        _list.Append(item);
    }

    public Item? Get(string id)
    {
        return _list.SingleOrDefault(_ => _.Id == id);
    }
}

We now get a request to add logging. You might be inclined to simply add logging to the repository as this is the easiest thing to do, but it will result in altering already working and tested code to add more behavior. For example:

// ItemRepository.cs
public class ItemRepository : IItemRepository
{
    private readonly IEnumerable<Item> _list =  new List<Item>();
    private readonly ILogger<ItemRepository> _logger;

    public ItemRepository(ILogger<ItemRepository> logger)
    {
        _logger = logger;
    }

    public void Put(Item item)
    {
        _logger.LogInformation("saving item: {id}", item.Id);
        _list.Append(item);
    }

    public Item? Get(string id)
    {
        _logger.LogInformation("Attempting to retrieve item with id: \'{Id}\'", id);
        return _list.SingleOrDefault(_ => _.Id == id);
    }
}

An alternative is to use the Decorator pattern. Instead of adding more code to an existing class, a new class is made that adds new behavior and simply calls on the existing class to use that behavior. It decorates the existing class.

// LoggingItemRepository.cs
public class LoggingItemRepository : IItemRepository
{
    private readonly ILogger<LoggingItemRepository> _logger;
    private readonly IItemRepository _repository;

    public LoggingItemRepository(IItemRepository repository, ILogger<LoggingItemRepository> logger)
    {
        _repository = repository;
        _logger = logger;
    }

    public void Put(Item item)
    {
        _logger.LogInformation("saving item: {id}", item.Id);
        _repository.Put(item);
    }

    public Item? Get(string id)
    {
        _logger.LogInformation("Attempting to retrieve item with id: \'{id}\'", id);
        return _repository.Get(id);
    }
}

Doing this enables you to test the logging logic without having to rely on the repository working as this can be mocked away.

Conclusion

In this second part of articles looking at Code that fits in your head I took a look at recommendations for testing your code, and for keeping the complexity low. In the third and final part, I'll look at some tips for managing your code writing.

(Background header image courtesy of Ross Sneddon)