c# Is using the assertion method bad or not for clean code?

Question

I'm reading Uncle Bob Martin's Clean Code and now I have a question about the code I wrote. Is using a method as an assertion method a good or bad practice for clean code? Example

// Simple example
public class EmployeeGateway
{
    private readonly IWebService _service;

    public EmployeeGateway(IWebService service)
    {
        _service = service
    }

    public Employee Login(string username, string password)
    {
        var myEmployee = _service.Find(username);

        // DoStuff...

        ThrowIfNotEnabled(myEmployee.isEnabled);

        //...
    }

    // Assert Method
    private void ThrowIfNotEnabled(bool isEnabled)
    {
        if(!isEnabled)
            throw new MyCustomException(State.EmployIsNotEnabled)
    }
}

PS: In Resharper there is an attribute [AssertionMethod].

Answer 1

No its not good, it's fragile and prone to over 'genericisation'

The problem I see here is that your function just takes a bool rather than the entire employee object. So you can pass any bool in.

The name doesnt indicate that the exception throw is specific to a particular meaning of the boolean you pass either.

So there are two issues.

1. If the logic changes in the future, perhaps the flag is igored for employees who work on tuesdays, you have no way off accessing the extra data required to make your decision.

2. The function may be reused over time with other inputs. Maybe I reuse the error for part time employees, or contractors or fridges. Now its lost its origional intention and I cant correct that easily.

Instead go with

ValidateEmployeeCanLogin(Employee employee)

This is a far more descriptive name, allows for multiple validation failure reasons and is more likely to be flexible to logic changes going forward.

Just reading up on AssersionMethodAttribute, it seems to me that this is for writing compilation checks rather than validation. So if you set it all up correctly resharper will give you a compilation warning if it detects that an employee could fail your login function.

Answer 2

If an 'Assertion Method' (a term I've not heard before) helps you to reduce redundancy and keep the calling method 'clean' (focussed on its job and not other details), then I'd consider it good option for the same reasons you would pull anything out into a method. If you need to perform the same check many times of in many methods, then it's important that you don't re-implement it in 5 different places. If the check distracts from the logical purpose of the method, then hiding the details away in another method may be a good move as well, but this could be a local function if it is completely specific to one method.

The main issue I have with the use of such methods is that it is easy to compromise on the qualify of throw exceptions when using them (e.g. argument exceptions should provide the parameter name, and it's very easy to 'forget' this). Ewan's comments on what is essentially the opposite of this (a loose API and specific exception) of course also hold, and I would agree that this example is not a good one.