C# Refactoring of If Statement

Question

I am experimenting with different areas of C# and refactoring best practices/patterns.

As can be seen the Validate method below has 3 child validation methods.

Is there a way to redesign this method/refactor it so that the if statement are remove? (possibly using Delegate?).

Also what general code standard improvements would you suggest?

public bool Validate()
{            
     bool validDump;

     validDump = ValidateRecordIdentifiers();
     if (!validDump)
     {
         LogLogic.AddEntry(LogLogic.GetEnumDescription(
              LogMessages.StatusMessages.JobValidationFailed));
         return false;
     }

     validDump = ValidateTotals();
     if (!validDump)
     {
         LogLogic.AddEntry(LogLogic.GetEnumDescription(
              LogMessages.StatusMessages.JobValidationFailed));
         return false;
     }

     validDump = ValidateRecordCount();
     if (!validDump)
     {
         LogLogic.AddEntry(LogLogic.GetEnumDescription(
              LogMessages.StatusMessages.JobValidationFailed));
         return false;
     }

     LogLogic.AddEntry(LogLogic.GetEnumDescription(
          LogMessages.StatusMessages.JobValidationPassed));
     return true;
}

Answer 1

You can take a look at Validation Application Block and Code Contracts

Answer 2

You could modify your validate methods so that they take in the LogLogic parameter and add an entry themselves for failing.

They could still return a boolean value, and this could be used to keep your return as soon as possible.

return ValidateRecordIdentifiers(LogLogic) 
    && ValidateTotals(LogLogic) 
    && ValidateRecordCount(LogLogic);

Answer 3

    bool valid = false; 
       if(ValidateRecordIdentifiers() && ValidateTotals() && ValidateRecordCount())
        {
          valid = true;
        }


/******AN Alternate Suggestion for the above code********/
    bool valid =  ValidateRecordIdentifiers() && 
                  ValidateTotals() && 
                  ValidateRecordCount();
/*******End Alternate Suggestion*************/


   var statusMessage = (valid) ? 
       LogMessages.StatusMessages.JobValidationPassed :
       LogMessages.StatusMessages.JobValidationFailed

   LogLogic.AddEntry(LogLogic.GetEnumDescription(statusMessage));

   return valid;

See short circuiting: http://msdn.microsoft.com/en-us/library/2a723cdk%28VS.71%29.aspx

Answer 4

public bool Validate()
{
    return Validate(ValidateRecordIdentifiers, ValidateTotals, ValidateRecordCount);
}

public bool Validate(params Func<bool>[] validators)
{
    var invalid = validators.FirstOrDefault(v => !v());
    if (invalid != null)
    {
        LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationFailed));
        return false;
    }
    LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationPassed));
    return true;
}

Answer 5

You could do something simple like this:

bool validDump;
string message;
if ((!ValidateRecordIdentifiers()) ||
    (!ValidateTotals()) ||
    (!ValidateRecordCount()))
{
    message = LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationFailed);
}
else
{
    message = LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationPassed);
    validDump = true;
}
LogLogic.AddEntry(message);
return validDump;

Answer 6

This looks to me like a case for structured exception handling. It looks like an exception condition that you are handling in the sense that something invalid has been input, and it results in abandoning the process. Have you considered using try/catch in the parent function and throw within the child functions to handle this?

Example:

public bool Validate()
{
   try
   {
      ValidateRecordIdentifiers();
      ValidateTotals();
      ValidateRecordCount();
      LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationPassed));
      return true;
   }
   catch (ValidationException ex)
   {
      LogLogic.AddEntry(ex.status);
      return false;
   }
}

class ValidationException : ApplicationException
{
   public readonly LogMessages.StatusMessages status;
   ValidationException(LogMessages.StatusMessages status)
   {
      this.status = status;
   }
}

void ValidateRecordIdentifiers()
{
   if (bad)
      throw new ValidationException(LogMessages.StatusMessages.JobValidationFailed);
}

void ValidateTotals()
{
   if (bad)
      throw new ValidationException(LogMessages.StatusMessages.JobValidationFailed);
}

void ValidateRecordCount()
{
   if (bad)
      throw new ValidationException(LogMessages.StatusMessages.JobValidationFailed);
}

Edit: I generally don't like to use exception handling for errors that are not immediately reported out to the UI because exception handling can be costly, and excessive exception throwing can make the application harder to debug if you're trying to find real exception cases among a bunch of exceptions that aren't really "exceptional". But depending on your specific case, it may be appropriate. Just use with caution.

Answer 7

The first thing that jumps out is duplication: LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationFailed));

So I'd look to collapse it into something like:

public StatusMessages Validate() {

  LogMessages.StatusMessages status = LogMessages.StatusMessages.JobValidationFailed;

  if( ValidateRecordIdentifiers() && ValidateTotals() && ValidateRecordCount())
    status = LogMessages.StatusMessages.JobValidationPassed;

  LogLogic.AddEntry(status.ToString());

  return status;
}

Answer 8

Maybe:

public bool Validate() 
{

        if (ValidateRecordIdentifiers() && ValidateTotals() && ValidateRecordCount())
        {
           LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationPassed));
           return true;
        }

        LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationFailed));

        return false;
    }

Answer 9

You are writing the same error message regardless of which validation function fails. It might be more helpful to log a specific error message in each case. Otherwise you can rewrite what you already have much simpler:

if (ValidateRecordIdentifiers() && ValidateTotals() &&  ValidateRecordCount())
{
    LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationPassed));
    return true;
}
LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationFailed));
return false;

Answer 10

Framework:

class Validator
{
    Func<bool> validatorDelegate;
    Action failDelegate;

    public Validator(Func<bool> v, Action fail)
    {
        validatorDelegate = v;
        failDelegate = fail;
    }

    public bool Validate()
    {
        bool rc = validatorDelegate();
        if (!rc) failDelegate();
        return rc;
    }
}

class ValidatorCollection : List<Validator>
{
    Action successDelegate;
    Action failDelegate;

    public ValidatorCollection(Action failDelegate, Action successDelegate)
    {
        this.successDelegate = successDelegate;
        this.failDelegate = failDelegate;
    }

    public bool Validate()
    {
        var rc = this.All(x => x.Validate());
        if (rc) successDelegate();
        return rc;
    }

    public void Add(Func<bool> v)
    {
        this.Add(new Validator(v, failDelegate));
    }
}

Usage:

class test
{
    public bool Validate()
    {
        return new ValidatorCollection(
            FailAction,
            SuccessAction)
        {
            valTrue,
            valTrue,
            valFalse
        }.Validate();
    }

    public void FailAction()
    {
        LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationFailed));
    }

    public void SuccessAction()
    {
        LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationPassed));
    }

    public bool valTrue()
    {
        return true;
    }
    public bool valFalse()
    {
        return false;
    }
}

Answer 11

There's a number of different ways to write this but your method is short and readable. The suggestions posted so far are, imo, much less readable and harder to debug (where would you set a breakpoint?). I would leave this method as is and look for other refactoring opportunities.

Answer 12

public bool Validate()
{
 return LogSuccess(
  new[] {ValidateRecordIdentifiers, ValidateTotals, ValidateRecordCount }
  .All(v=>v()));
}

private bool LogSuccess(bool success)
{
 LogLogic.AddEntry(LogLogic.GetEnumDescription(success
   ? LogMessages.StatusMessages.JobValidationPassed
   : LogMessages.StatusMessages.JobValidationFailed
 );
 return success;
}

Answer 13

Value readability above all else (well, as long as it is in the same ballpark efficiency).

About the only changes I would make is to eliminate the unneeded variable, and use the function call in the conditional, and replace the ! operator with == false. This is easier to see for aging programmers like myself with bad eyesight :)

As implied by the comment of another poster, it is better to make the function read InvalidXX instead, to avoid using negation or == false and for better readability.

Also, as far as combining all the conditionals into a single "AND" statement, I would do that in lisp, but not in c#, because it will making debugging and tracing harder.

In particular, you probably don't want to put the same error message for each case - you should have a different one for each case so you know exactly what happened. Combining all cases into a single expression won't allow you to do this.

---

public bool Validate() {

     if (ValidRecordIdentifiers() == false)        { 
        LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationFailed)); 
        return false; 
    } 

    if (ValidTotals() == false)        { 
        LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationFailed)); 
        return false; 
    } 

     if (ValidateRecordCount() == false)        { 
        LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationFailed)); 
        return false; 
    } 

    LogLogic.AddEntry(LogLogic.GetEnumDescription(LogMessages.StatusMessages.JobValidationPassed)); 
    return true; 
} 

Answer 14

Your function does two things: validation and logging. You could separate them like this. This also lets you log these errors differently if you ever decide to do this.

public bool ValidateAndLog()
{
    LogMessages.StatusMessages result=Validate();
    LogLogic.AddEntry(LogLogic.GetEnumDescription(result));
    return result==LogMessages.StatusMessages.JobValidationPassed;
}

private LogMessages.StatusMessages Validate()
{
    //of course you can combine the next three ifs into one 
    if (!ValidRecordIdentifiers())
        return LogMessages.StatusMessages.JobValidationFailed;
    if (!ValidateTotals())
        return LogMessages.StatusMessages.JobValidationFailed;
    if (!ValidateRecordCount())
        return LogMessages.StatusMessages.JobValidationFailed;
    return LogMessages.StatusMessages.JobValidationPassed;
}