-1
\$\begingroup\$

I have unit class which have some properties, and I have acceptancePolicy table which contain values. If values (from one row) are true method should return true. The method is below. How can I improve that code? What I should do with if statements?

public async Task<bool> IsReadyToComplete(Unit unit)
{
    var acceptancePolicy =  await GetAllAsync();
    foreach (var policy in acceptancePolicy)
    {
        var isReady = true;
        if (!string.IsNullOrEmpty(policy.unitBusinessUnitId) && policy.unitBusinessUnitId != unit.BusinessUnitId)
        {
            isReady = false;
        }
        if (!string.IsNullOrEmpty(policy.unitCostCenterId) && policy.unitCostCenterId != unit.CostCenterId)
        {
            isReady = false;
        }
        if (!string.IsNullOrEmpty(policy.unitLocationId) && policy.unitLocationId != unit.LocationId)
        {
            isReady = false;
        }
        if (!string.IsNullOrEmpty(policy.unitCategoryMask) && !unit.unitCategory.unitCategoryId.Contains(policy.unitCategoryMask))
        {
            isReady = false;
        }

        if (policy.MaximalPrice > 0 && policy.MaximalPrice > unit.PriceTotal)
        {
            isReady = false;
        }

        if (policy.AcceptanceLevelRequiredId > unit.unitStatusHistory.unitStatusId)
        {
            isReady = false;
        }

        if (isReady)
            return true;
    }

    return false;
}
\$\endgroup\$
1
\$\begingroup\$

Use else if
No purpose to setting it false more than once

if (!string.IsNullOrEmpty(policy.unitBusinessUnitId) && policy.unitBusinessUnitId != unit.BusinessUnitId)
{
    isReady = false;
}
else if (!string.IsNullOrEmpty(policy.unitCostCenterId) && policy.unitCostCenterId != unit.CostCenterId)
{
    isReady = false;
} 
\$\endgroup\$
0
\$\begingroup\$

My thoughts for what it's worth. Hope you find them helpful.

public static bool IsReadyToComplete(Unit unit, AcceptancePolicy acceptancePolicy)
{
    // NOTE: try not to "go get" your own values.  have them passed in.  it will mnake unit
    //       testing much easier.   
    //var acceptancePolicy = await GetAllAsync();
    // NOTE: you can turn this into a linq statement, but i feel that's mostly a readability
    //       decision.
    foreach (var policy in acceptancePolicy)
    {
        // NOTE: it looks as though your original logic only needed one policy to be pass
        //       before exiting this loop.  maybe that's ok, i just thought i'd mention it.
        if (IsReadyToComplete(unit, policy))
            return true;
    }

    return false;
}

// NOTE: break the method down to it's basic needs and pass in the policy
//       rather than going and getting it.  it's makes it significantly easier 
//       to unit test.
// NOTE: it also separates the meaning of the boolean result.  one method is true
//       if any policies pass.  the other method is true if a policy is passed.
//       so you don't have one bool var trying to accomodate both.
public static bool IsReadyToComplete(Unit unit, Policy policy)
{
    var isReady = true;

    // NOTE: in C# properties and values start with an upper case.  consistency is key.
    if (!string.IsNullOrEmpty(policy.unitBusinessUnitId) && policy.unitBusinessUnitId != unit.BusinessUnitId)
    {
        isReady = false;
    }
    // NOTE: else if will short cut your logic and make it run more efficiently
    else if (!string.IsNullOrEmpty(policy.unitCostCenterId) && policy.unitCostCenterId != unit.CostCenterId)
    {
        isReady = false;
    }
    // NOTE: you could OR these all together.  i think that's a readability choice.  i don't think
    //       there are any significant benefits from doing that.
    else if (!string.IsNullOrEmpty(policy.unitLocationId) && policy.unitLocationId != unit.LocationId)
    {
        isReady = false;
    }
    // NOTE: depending on the complexity or reusability of these, you could make a method for each if.
    //       otherwise, if this is the only place they are used, just leave them in here. it's not hard
    //       to understand or anything.
    // NOTE: the "law of demeter" would suggest that depending on properties more than one level deep
    //       can turn into quicksand if you are not careful.  that's a harder one to wrap you head around
    //       if you aren't used to thinking about it.  but fyi.
    else if (!string.IsNullOrEmpty(policy.unitCategoryMask) && !unit.unitCategory.unitCategoryId.Contains(policy.unitCategoryMask))
    {
        isReady = false;
    }
    else if (policy.MaximalPrice > 0 && policy.MaximalPrice > unit.PriceTotal)
    {
        isReady = false;
    }
    else if (policy.AcceptanceLevelRequiredId > unit.unitStatusHistory.unitStatusId)
    {
        isReady = false;
    }

    return isReady; 
}
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.