How can this IF statement code snippet be Refactored?

Question

I've read a lot of Refactoring literature as of late and a common theme I see in examples is the reduction of lots of IF statements. The below is a very simple code block that first checks to see if an email address is supplied and if so, then proceeds to validate the email address. I typically see a lot of this type of code which always looks a bit messy, especially if there are multiple variables.

if (string.IsNullOrEmpty(email)) {

    throw new ApplicationException("Email Address Required");
}

else {

    if (!ValidationService.EmailAddressIsValid(email)) {
        throw new ApplicationException("Invalid Email Address");
    }

}

My question is, is this example perfectly acceptable? Does it smell? How might one refactor this snippet?

Answer 1

throw never returns, so I would use the following for better readability:

if (string.IsNullOrEmpty(email)) {
    throw new ApplicationException("Email Address Required");
}

if (ValidationService.EmailAddressIsValid(email)) {
   throw new ApplicationException("Invalid Email Address");
}

// continue here if no error was detected

Answer 2

It's fine but you could simplify it more.

if (string.IsNullOrEmpty(email)) {
    throw new ApplicationException("Email Address Required");
} else if (ValidationService.EmailAddressIsValid(email)) {
 throw new ApplicationException("Invalid Email Address");
}

Answer 3

The first thing you could do would be to remove the lines else { and its corresponding }.

This is because the first throw statement would ensure that the code currently in the else block would never execute if the email address was missing.

Answer 4

It's hard to refactor such a small snippet - And I'll skip the usual statement about not refactoring without tests...

When I see code like this, I think

• Why does the ValidationService not hold the logic for the null check? If this is not in a class that holds validation logic, then why are we checking for required fields here?

• I would probably make the null check into a helper so the code could be more like:

  ThrowIfRequiredFieldMissing(email, "Email Address");

• remove the else, since throw cannot continue

• Create a helper for the throwing: FailIf( !Validation.EmailAdressIsValid(email), "Email Address"). This would throw your validation exception, and allow you to change exceptions later if needed (although I'd cross that bridge when you come to it)

There's lots of other possibilities, like a dictionary of Func<>s, or a strategy pattern applied to a list of fields, and on and on and on. But without more code, deciding what to do (if anything!) is just guesswork.

Answer 5

Refactoring is more than just a little bit of code styling.

You're validating a user entry, so you would probably want to define or reuse an existing validation framework instead of embedding if statements in the code. You're almost there it seems, since you have the ValidationService for the second validation.

So my proposal: Include the string.IsNullOrEmpty check in the implementation of EmailAddressIsValid.

Of course this only moves one if statement, but if you use the EmailAddressIsValid in multiple places it will reduce the if statements. Further the validation logic is then really consolidated in one place. Currently it isn't.

Another minor thing: Rename EmailAddressIsValid to IsEmailAddressValid, because it's a common convention to use "Is" as a prefix for functions that return a boolean.

Answer 6

Ugly variant, but workable

string msg = string.Empty;    
if (string.IsNullOrEmpty(email))
{
   msg = "Email Address required";
}
else if (ValidationService.EmailAddressIsValid(email))
{
   msg = "Invalid Email Address";
}

if (msg != string.Empty())
   throw new ApplicationException(msg);

Answer 7

If you would really like to make your code readable, you could even do something like this:

throwExceptionIfEmailIsEmpty(email);
throwExceptionIfEmailIsInvalid(email);
// continue to process the e-mail here...

With this code extracted into separate functions:

private void throwExceptionIfEmailIsEmpty(String email)
{
    if (string.IsNullOrEmpty(email))
        throw new ApplicationException("Email Address Required");
}

private void throwExceptionIfEmailIsInvalid(String email)
{
    if (ValidationService.EmailAddressIsValid(email))
        throw new ApplicationException("Invalid Email Address");
}

It might seem a bit exessive, but it makes it way much easier to see what's really going on!
(Inspired by Uncle Bob's Clean Code - Functions should do one thing. They should do it well. They should do it only. (or something like that) - if anyone wondered ;))