CODEWITHSUNDEEP
c#code-cleanup
shamim
shamim

Reputation: 6768

How to avoid repeated if else series code smell

Work on C# 4.5. My bellow syntax so many if else series it seems code smell,want a way to avoid this smell.Any kind of help will be acceptable. Thank you

public bool CheckValidCustomer()
{
    return _checkManager.IsCustomerPersonal(_customer) ? IsValidPersonalCustomer() : IsValidCompanyCustomer();
}

private bool IsValidCompanyCustomer()
{
    if (_checkManager.IsValidFinancialInfo(_customer) == false)
    {
        ProcessMessage = "Please Check Financial Info.";
        return false;
    }

    if (_checkManager.IsValidCompanyInfo(_customer) == false)
    {
        ProcessMessage = "Please Check Company Info.";
        return false;
    }

    if (_checkManager.IsValidStakeHolderInfo(_customer) == false)
    {
        ProcessMessage = "Please Check Stake Holder Info.";
        return false;
    }

    if (_checkManager.IsValidResponsiblePersonInfo(_customer) == false)
    {
        ProcessMessage = "Please Check Responsible person Info.";
        return false;
    }


    if (_checkManager.IsValidScreeningInfo(_customer) == false)
    {
        ProcessMessage = "Please Check Screening Info .";
        return false;
    }

    if (_checkManager.IsValidMyNumberUpload(_customer) == false)
    {
        ProcessMessage = "Please Check My Number Upload Info.";
        return false;
    }

    if (_checkManager.IsValidIdUpload(_customer) == false)
    {
        ProcessMessage = "Please Check Id Upload Status.";
        return false;
    }

    if (_checkManager.IsValidCustomerStatus(_customer) == false)
    {
        ProcessMessage = "Please Check Customer Status.";
        return false;
    }

    return true;
}

private bool IsValidPersonalCustomer()
{
    if (_checkManager.IsValidPersonalInfo(_customer)==false)
    {
        ProcessMessage = "Please Check Personal Info.";
        return false;
    }

    if (_checkManager.IsValidFinancialInfo(_customer)==false)
    {
        ProcessMessage = "Please Check Financial Info.";
        return false;
    }

    if (_checkManager.IsValidCompanyInfo(_customer)==false)
    {
        ProcessMessage = "Please Check Company Info.";
        return false;
    }

    return true;
}

Upvotes: 4

Views: 734

Answers (3)

Matthew Whited
Matthew Whited

Reputation: 22443

You could do something like this...

private bool IsValidCompanyCustomer()
{
    var companyValidationRules = new Dictionary<string, Func<Customer, bool>>
    {
        { "Please Check Financial Info.", _checkManager.IsValidFinancialInfo},
        { "Please Check Company Info.", _checkManager.IsValidCompanyInfo},
        { "Please Check Stake Holder Info.", _checkManager.IsValidStakeHolderInfo},
        { "Please Check Responsible person Info.", _checkManager.IsValidResponsiblePersonInfo},
        { "Please Check Screening Info.", _checkManager.IsValidScreeningInfo},
        { "Please Check My Number Upload Info.", _checkManager.IsValidMyNumberUpload},
        { "Please Check Id Upload Status.", _checkManager.IsValidIdUpload},
        { "Please Check Customer Status.", _checkManager.IsValidCustomerStatus},
    };

    var failedRule = companyValidationRules.Where(d => !d.Value(_customer))
                                           .Select(d => d.Key)
                                           .FirstOrDefault();
    if (!string.IsNullOrWhiteSpace(failedRule))
    {
        this.ProcessMessage = failedRule;
        return false;
    }

    return true;
}

private bool IsValidPersonalCustomer()
{
    var companyValidationRules = new Dictionary<string, Func<Customer, bool>>
    {
        { "Please Check Personal Info.", _checkManager.IsValidPersonalInfo},
        { "Please Check Financial Info.", _checkManager.IsValidFinancialInfo},
        { "Please Check Company Info.", _checkManager.IsValidCompanyInfo},
    };

    var failedRule = companyValidationRules.Where(d => !d.Value(_customer))
                                           .Select(d => d.Key)
                                           .FirstOrDefault();
    if (!string.IsNullOrWhiteSpace(failedRule))
    {
        this.ProcessMessage = failedRule;
        return false;
    }

    return true;
}

}

Upvotes: 0

sidecus
sidecus

Reputation: 752

My personal suggestions:

  1. Create a base class Customer, and Two subclasses PersonalCustomer/CompanyCustomer. Validation should be done within these classes - they know their implementation details.
  2. Customer base class has a member which is a chain of validation actions, each action item in the chain returns an Enum as a validation result (success o4 specific error)
  3. Customer base class has a method Validate, which calls each validation action in the chain and returns if validation fails
  4. Each subclass implements the detailed validation action, and registers it into the validation chain. No more checkmanager. caller can just call _customer.Validate() and don't have to worry what type of customer that is.
  5. A representation layer to map the enum (error code basically) to some UI specific strings, this can be done via an array/hashset - no more if/else or switch.

Upvotes: 2

Tilak
Tilak

Reputation: 30698

You can use validation rule pattern. Avoiding many if blocks for validation checking

Create a set of validation rules. And run through all of them one by one. If any one validation rule fails, complete validation fails (depending on business rules).

You can also refer to WPF Data Binding Validation(Data Validation -> Validation Process section) to get ideas for designing your own validation rules engine.

Upvotes: 3

Related Questions