CODEWITHSUNDEEP
c#if-statement
asn1981
asn1981

Reputation: 237

C# Refactoring of If Statement

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;
}

Upvotes: 3

Views: 2976

Answers (15)

Fagun
Fagun

Reputation: 79

As the statement of those if conditions are the same for all, so you can do the check in one condition and do the reset job at the below.

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

    LogLogic.AddEntry(LogLogic.GetEnumDescription(
        LogMessages.StatusMessages.JobValidationFailed));
    return false;
}

Upvotes: 0

Larry Watanabe
Larry Watanabe

Reputation: 10184

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; 
}

Upvotes: 0

yu_sha
yu_sha

Reputation: 4410

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;
}

Upvotes: 1

kemiller2002
kemiller2002

Reputation: 115430

    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

Upvotes: 20

George Polevoy
George Polevoy

Reputation: 7671

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;
}

Upvotes: 0

fearofawhackplanet
fearofawhackplanet

Reputation: 53378

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;

Upvotes: 2

Aviad P.
Aviad P.

Reputation: 32629

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;
    }
}

Upvotes: 3

BlueMonkMN
BlueMonkMN

Reputation: 25601

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.

Upvotes: 1

Jamie Ide
Jamie Ide

Reputation: 49251

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.

Upvotes: 2

magnus
magnus

Reputation: 653

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;
    }

Upvotes: 1

Jeffrey Knight
Jeffrey Knight

Reputation: 5977

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;
}

Upvotes: 2

Matthew Steeples
Matthew Steeples

Reputation: 8058

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);

Upvotes: 2

Justin Rusbatch
Justin Rusbatch

Reputation: 4052

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;

Upvotes: 1

Darin Dimitrov
Darin Dimitrov

Reputation: 1038720

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;
}

Upvotes: 2

Dzmitry Huba
Dzmitry Huba

Reputation: 4521

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

Upvotes: 1

Related Questions