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].

I would be surprised if a "searcher" threw an exception when an employee is not enabled. I would expect `GetEmployee` to return null in this case, or return a non-enabled employee. — Greg Burghardt, Sep 20 '19 at 14:28
For example using LdapConnection (Namespace: System.DirectoryServices.Protocols) If you Bind a disabled user, a LdapException exception is thrown with a specific error. I just emulated what Microsoft does. PS: mine was just an example — pampua84, Sep 20 '19 at 14:41
I might be hung up on your specific example, but in this case Microsoft is not a good example for your example. — Greg Burghardt, Sep 20 '19 at 14:44
Sorry, but the crux of the question is whether extracting the method is a good practice or not for clean code? — pampua84, Sep 20 '19 at 15:02
I don't know much about Resharper. What is the meaning of the attribute `[AssertionMethod]` (and how is your comment related to the code example, where I don't see this attribute?) — Doc Brown, Sep 20 '19 at 16:18
I don't see any issue with assertion methods vs. the general principles of [clean code](https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29). And the symbol `ThrowIfNotEnabled` certainly communicates the purpose clearly. That being said, I don't think a login action should leak any information other than success or failure, i.e. user lockout or disablement should be indistinguishable from an incorrect password. See the [OWASP authentication cheat sheet](https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#authentication-responses). — John Wu, Sep 20 '19 at 23:32
@DocBrown I forget exactly what [AssertionMethod] accomplishes, but if you neglect it on such methods you end up with Resharper complaining about your code. (After all, such a method doesn't really do anything.) — Loren Pechtel, Sep 21 '19 at 00:29

Ewan · Accepted Answer · 2019-09-21T08:23:59.163

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.

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.
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.

score 1 · Answer 2 · answered Sep 21 '19 at 08:32

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.

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

2 Answers2