4 proven ways how tests reveal awful code

One of my favorite shows when I was younger was CSI: Las Vegas.

Many of the episodes had similar plots:

1. A murder happened.
2. The CSI team investigates.
3. They identify one suspect.
4. But that guy hasn’t done it.
5. After further investigation, they identify the actual murderer.

In software development, you may think about unit testing as a way to prevent bugs in code.

But what if you start looking at them as a way to perform code diagnostics?

After all, they can reveal a lot about code design.

Let me show you what I mean.

1. Too many mocks

Often, you see a class with too many constructor parameters.

public OrderService(
    IOrderRepository orderRepository,
    IInvoiceRepository invoiceRepository,
    ICustomerService customerService,
    IPriceCalculator priceCalculator,
    ILogger logger,
    IEmailSender emailSender,
    IReportGenerator reportGenerator,
    ITaxCalculator taxCalculator)
{
}

The class with 8 dependencies 100% violates the Single Responsibility Principle.

This becomes more obvious when you look at the setup/arrange part of your unit test.

[Fact]
public void Order_can_be_created()
{
    // Arrange
    var orderRepositoryMock  = new Mock<IOrderRepository>();
    var invoiceRepositoryMock = new Mock<IInvoiceRepository>();
    var customerServiceMock = new Mock<ICustomerService>();
    var priceCalculatorMock = new Mock<IPriceCalculator>();
    var loggerMock = new Mock<ILogger>();
    var emailSenderMock = new Mock<IEmailSender>();
    var reportGeneratorMock = new Mock<IReportGenerator>();
    var taxCalculatorMock = new Mock<ITaxCalculator>();

    var orderService = new OrderService(
        orderRepositoryMock.Object,
        invoiceRepositoryMock.Object,
        customerServiceMock.Object,
        priceCalculatorMock.Object,
        loggerMock.Object,
        emailSenderMock.Object,
        reportGeneratorMock.Object,
        taxCalculatorMock.Object
    );

    // Act …

    // Assert …
}

Every test that checks the logic of the OrderController will have a dozen lines of setup code.

You could put the setup into a separate method. And call this method from your test methods.

But this just hides the problem.

🔎Code diagnostic 1: A huge setup in the test means that the class has too many responsibilities.

2. Struggle to write tests

If you struggle to write a unit test for the code, the problem is not in the test method.

The problem is in the code.

The following method:

public static async Task ProcessOrderAsync(int orderId)
{
    OrderDetails orderDetails = null;

    // Get order details from database
    using SqlConnection connection = new SqlConnection("db_connection_string");
    await connection.OpenAsync();
    SqlCommand command = new SqlCommand(
        $"SELECT * FROM Orders WHERE OrderID = {orderId}", connection);
    SqlDataReader reader = await command.ExecuteReaderAsync();

    if (await reader.ReadAsync())
    {
        orderDetails = new OrderDetails
        {
            OrderId     = orderId,
            TotalAmount = Convert.ToDecimal(reader["TotalAmount"]),
            // other relevant properties
        };
    }

    if (orderDetails == null)
        throw new InvalidOperationException($"Order with ID {orderId} not found.");

    // Calculate discount
    decimal discountPercentage = 0.0M;
    switch (orderDetails.TotalAmount)
    {
        case >= 1000:
            discountPercentage = 0.2M;
            break;
        case >= 500:
            discountPercentage = 0.15M;
            break;
        case >= 200:
            discountPercentage = 0.1M;
            break;
    }

    decimal discountAmount = orderDetails.TotalAmount * discountPercentage;

    // Ensure the discount does not exceed a certain limit
    decimal maxDiscount = 100.0M;
    if (discountAmount > maxDiscount)
        discountAmount = maxDiscount;

    // Apply discount via API
    using HttpClient client = new HttpClient();
    string apiUrl =
        $"https://example.com/apply-discount/{orderId}?discount={discountAmount}";
    HttpResponseMessage response = await client.PostAsync(apiUrl, null);
    if (!response.IsSuccessStatusCode)
    {
        throw new InvalidOperationException(
            $"Failed to apply discount for Order ID {orderId}. " +
            $"HTTP Status Code: {response.StatusCode}");
    }

    // Update order status to “Processed”
    SqlCommand updateCommand = new SqlCommand(
        $"UPDATE Orders SET Status = 'Processed' " +
        $"WHERE OrderID = {orderId}", connection);
    await updateCommand.ExecuteNonQueryAsync();
}

Has a lot of responsibilities:

1. It fetches the order details from the database (Database Logic)
2. Applies discount for the order (Business Logic)
3. Makes an HTTP call to an API (Networking Logic)
4. Updates the order in the database (Database Logic)

Several problems make writing any meaningful tests for this method impossible:

1. The connection string is hard-coded - you can’t replace it with the connection string to the test database.
2. Business logic is entangled with the I/O processing - you can’t check that in isolation.
3. Network call calls the production endpoint - the only way to test this is to have test data mixed with real data at your production endpoint.

🔎Code diagnostic 2: If you have trouble writing a test for the code where business logic is mixed with the calls to API/DB, then the problem is in the code. It needs to be decoupled.

3. How simple is too simple

I avoid unit testing simple delegate classes.

Let me explain why.

Take a look at the following class.

public class AccountService
{
    private IAccountRepository _accountRepository;

    public AccountService(IAccountRepository accountRepository)
    {
        _accountRepository = accountRepository;
    }

    public void Save(Account account)
    {
        _accountRepository.SaveAccountToDatabase(account);
    }

    public void Update(Account account)
    {
        _accountRepository.UpdateAccount(account);
    }
}

AccountService is a simple class. Its only purpose is to delegate method calls. The Save method calls the IAccountRepository.

Here’s the test.

[Fact]
public void Save_calls_repository_save_to_database()
{
    // Arrange
    var repositoryMock = new Mock<IAccountRepository>();
    var sut = new AccountService(repositoryMock.Object);

    // Act
    sut.Save(new Account());

    // Assert
    repositoryMock
        .Verify(x => x.SaveAccountToDatabase(It.IsAny<Account>()), Times.Once);
}

Simple.

In this example, the AccountService class exists because of the convention to have multiple layers:

• UI layer
• Service layer
• Repository layer

It’s ok to have multiple layers.

But the problem is that the sole purpose of the layer is to delegate calls.

Even though there is a test for this delegate method, the test by itself is not relevant enough to exist. At this moment, there is no logic that you can change.

🔎Code diagnostic 3: Don’t have layers for the sake of having layers. Instead, remove layers that don’t contain any logic. Remove unnecessary classes. There is plenty of other code that has to be maintained.

4. Living on the edge

A common problem in codebases is deciding how to test code at the edge of your application.

Here is a method that checks whether the account is valid and then saves it to the database.

public bool Save(Account account)
{
    try
    {
        bool isAccountValid = _accountValidator.IsAccountValid(account);
        if (!isAccountValid)
            return false;

        _accountRepository.SaveAccountToDatabase(account);
        return true;
    }
    catch (Exception ex)
    {
        _logger.LogError(ex, "Error while saving account to database");
        return false;
    }
}

Explanation:

1. The method checks if the account is valid using the IAccountValidator.
2. If the account is valid, it uses IAccountRepository to save it to the database.
3. If anything goes wrong, the exception will be captured with the ILogger.

The unit test that checks that the account was saved to the database looks like this.

[Fact]
public void Save_saves_account_to_database()
{
    var repositoryMock = new Mock<IAccountRepository>();
    var validatorStub  = new Mock<IAccountValidator>();
    validatorStub
        .Setup(x => x.IsAccountValid(It.IsAny<Account>()))
        .Returns(true);

    var sut = new AccountService(
        repositoryMock.Object,
        new Mock<ILogger>().Object,
        validatorStub.Object);

    sut.Save(new Account());

    repositoryMock.Verify(
        x => x.SaveAccountToDatabase(It.IsAny<Account>()),
        Times.Once);
}

The test uses validatorStub and repositoryMock to fake dependencies.

It then verifies that the SaveAccountToDatabase method was called.

At first, the test looks fine.

But here’s how it can break during refactoring:

• You change the IsAccountValid method, add a new parameter - test breaks ❌
• You change the class that does validation - test breaks ❌
• You change the SaveAccountToDatabase - test breaks ❌

This is a common issue that happens when you use Moq/NSubstitute for test doubles (a topic for another day).

But the main issue I have with this test is the name.

The unit test cannot check that the account was created in the real database.

For that, you need an integration test.

You need to set up a test database, call the Save method, and then check that the new account is in the database.

🔎Code diagnostic 4: You need to balance your test efforts by using different tests: unit, integration, and UI tests.

To sum up

Tests can talk.

And you should listen to them.

Because they can tell where the new bug is.

But they can also help you design better code.