2

The consensus seems to be that it's better to put an if statement guard clause at the top of a method rather than using an if else statement.

However, how can this be done with a void method and Console.WriteLine() to display an error? I would argue that example 2 is pretty ugly, especially if you have to validate multiple parameters and display separate errors when checking user input in a command line application for example.

I'm not an advanced programmer, but I'd argue the 1st example is more readable in this scenario. With that said, I can understand why people dislike complicated nested if statements.

1. If else statement

private static void ValidateInput(string[] files, char[] password)
{
    if (files != null)
    {
        if (password.Length > 0)
        {
            ... // 20 lines here
        }
        else
        {
            Console.WriteLine("Error: Please enter a password.");
        }
    }
    else
    {
        Console.WriteLine("Error: Please select a file.");
    }
}

2. If statement

private static void ValidateInput(string[] files, char[] password)
{
    if (files == null)
    {
        Console.WriteLine("Error: Please enter a password.");
        return;
    }
    if (password.Length == 0)
    {
        Console.WriteLine("Error: Please select a file.");
        return;
    }
    ... // No indentation for the next 20 lines.
}
7
  • 7
    The second example looks much better, IMO. The fundamental mistake here is that your business logic (validation of files, passwords) is intertwined with user interface logic (printing messages to the console). This is what exceptions are for. ValidateInput should through an exception. A separate component, higher up, should catch that exception and react appropriately. This can include responding by logging to the console. – Alexander Dec 29 '20 at 20:23
  • 1
    Alternative consideration: on a form, it's much nicer to shower a user all the mistakes, rather than having them submit over and over, and revealing only one mistake at a time (which ever your logic caught first, short-circuiting the rest). In that case, you should make a ValidationResult object, and have each if statement staple a new ValidationError object to it. Your function should then return this ValidationResult, so that a higher component can then either proceed (if there's no errors), or display all the errors (e.g. make all the errored form fields red). – Alexander Dec 29 '20 at 20:26
  • What action is performed in the 20 lines omitted? – whatsisname Dec 30 '20 at 2:04
  • Or, as an alternative to Alexander's good comment: scratch the void and return an error code instead. It's a question of preference and api-design. However, don't mix user interface with logic. – mtj Dec 30 '20 at 5:39
  • @whatsisname I just modified an example I found elsewhere on StackExchange, but it could be things like more validation and then calls to another method. – user382081 Dec 30 '20 at 8:48
9

There's no great consensus on this, and it can vary from language to language.

That said, I'd encourage you to prefer the quick return pattern (#2 in the OP). Separating the condition from the error message make them hard to keep together in your head, which causes bugs and makes the code harder to read.

The nested conditional version also isn't great when you have many conditions. You'll end up with different guidelines for different situations, causing confusion and friction and increased cognitive load since there isn't a single pattern to identify.

1
  • You make a good point. I'll start adopting the 2nd style. – user382081 Dec 30 '20 at 8:47
1

Good answer here already. I would like to suggest going a step further and refactor this to something along the lines of:

private static void ValidateInput(string[] files, char[] password)
{
     string message = GetValidationMessage(files,password);
     if(message.Length>0)
     {
         Console.WriteLine("Error: " + message);
     }
}

private static string GetValidationMessage(string[] files, char[] password)
{
    if (files == null)
    {
        return "Please enter a password.";
    }
    if (password.Length == 0)
    {
        return "Please select a file.";
    }
    // ... 
}

(in C#, of course).

This is more DRY (since it does not repeat the Console.WriteLine statement multiple times), and makes a clear separation between the responsibilities of validating and processing the validation's result.

Alternatively, one could use exceptions here, but for a validation, failing is not really an "exceptional" situation, so it is debatable if this brings any real benefits.

In case you want to collect and print out several validation messages at once, you could refactor this to

private static void ValidateInput(string[] files, char[] password)
{
     var messages = GetValidationMessages(files,password);
     foreach(string message in messages)
     {
         Console.WriteLine("Error: " + message);
     }
}

private static IEnumerable<string> GetValidationMessages(string[] files, char[] password)
{
    if (files == null)
    {
        yield return "Please enter a password.";
    }
    if (password.Length == 0)
    {
        yield return "Please select a file.";
    }
    // ... 
}

This makes most sense when the different validation rules are mostly independent from each other, so one root cause does not does not produce half a dozen subsequent messages.

4
  • Note that GetValidationMessage could return an array of strings, one for each error. In which case append an "s" to the name. – user949300 Dec 30 '20 at 15:16
  • @user949300: an IEnumerable is more flexible than an array with less code, it lets the caller decide if they want an array or something else. And I appended already an "s" to GetValidationMessage in that second variant, didn't you notice? – Doc Brown Dec 30 '20 at 15:47
  • The problem with IEnumerable (and some similar Java classes, though some of them have a hasNext()) is that it doesn't have an easy way to count the number of errors. So, the most important question, "can I continue?", is hard to answer. – user949300 Dec 30 '20 at 18:52
  • @user949300: if you read my former comment again, more thoroughly, you may find out by yourself why this is a misconception. – Doc Brown Dec 30 '20 at 21:54
1

That it's a void method is irrelevant. We still have to return or throw on each branch; we still want to make early exit conditions orthogonal (apart from other concerns, the alternative could lead to more cases, more lines of code & deeper nesting); we still don't want to execute any code unnecessarily. If anything, void methods make this pattern even easier, because we don't need a sensible return value when you don't want to throwing an error or exception.

0

A series of guard clauses forms a list. A single nested if structure forms a tree.

The former is a one-dimensional data structure; the latter is two-dimensional.

The mental effort to understand a one-dimensional structure is comparable to that required to understand a two-dimensional structure only for trivial n (as in your example, where n is 2). If you were validating a series of 10 fields, a series of guard clauses would still be easy to reason about, while a nested if structure would be nearly incomprehensible-- you'd spend most of your effort scrolling up and down and matching up brackets. It would be very easy to put logic into the wrong else block because it is so far removed, literally, from the source code that governs when it executes.

Since a one-dimensional structure scales better, I see the guard clause approach as more maintainable and extensible.

That does not always mean it is preferable. A nested if structure may be clearer for logic where n is small and the code is not likely to change.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy