Tuesday, September 1, 2009

To Return Early or Not From a Method?

I remember always being told to not return early from a method.  However, I think it makes sense in some cases when you would otherwise have a set of nested “if” statements.

Take this example I ran across while working in our production system:

private void GetDiscountCode(Registration registration, Registrant registrant)
{
    if (registration.EventProfile.DiscountCodesEnabled &&
        registration.EventProfile.DiscountCodes.Count > 0)
    {
        if (tbDiscountCode.Text.Trim() != string.Empty)
        {
            DiscountCode discountCode = registration.EventProfile.DiscountCodes.Find(tbDiscountCode.Text);
            if (discountCode != null)
            {
                if (discountCode.Active)
                {
                    if (!discountCode.Limited
                        || discountCode.DiscountsUsed < discountCode.MaximumAllowed)
                    {
                        if (registrant.DiscountCodeApplied != discountCode.DiscountCodeId)
                        {
                            if (registrant.DiscountCodePercentage <= discountCode.DiscountPercentage)
                            {
                                registrant.DiscountCodeApplied = discountCode.DiscountCodeId;
                                registrant.DiscountCodePercentage = discountCode.DiscountPercentage;
                                registrant.DiscountCodeAmountPerLineItem = discountCode.DiscountAmount;
                                
                            }
                        }
                    }
                    else
                    {
                        errorMessages.Add(string.Format("The {0} you entered is no longer available", registration.EventProfile.DiscountCodeLabel));
                    }
                }
                else
                {
                    errorMessages.Add(string.Format("The {0} you entered is not currently active", registration.EventProfile.DiscountCodeLabel));
                }
            }
            else
            {
                errorMessages.Add(string.Format("The {0} you entered is not valid for this event", registration.EventProfile.DiscountCodeLabel));
            }
        }
        else
        {
            registrant.DiscountCodeApplied = -1;
            registrant.DiscountCodePercentage = 0;
        }
    }
}

After refactoring (with the same behavior):

private void GetDiscountCode(Registration registration, Registrant registrant)
{
    if (!registration.EventProfile.DiscountCodesEnabled ||
        registration.EventProfile.DiscountCodes.Count == 0)
        return;

    string codeText = tbDiscountCode.Text.Trim();
    if (codeText == string.Empty)
    {
        registrant.DiscountCodeApplied = -1;
        registrant.DiscountCodePercentage = 0;
        registrant.DiscountCodeAmountPerLineItem = 0;
        return;
    }

    DiscountCode discountCode = registration.EventProfile.DiscountCodes.Find(codeText);
    if (discountCode == null)
    {
        errorMessages.Add(string.Format("The {0} you entered is not valid for this event",
            registration.EventProfile.DiscountCodeLabel));
        return;
    }

    if (!discountCode.Active)
    {
        errorMessages.Add(string.Format("The {0} you entered is no longer available",
            registration.EventProfile.DiscountCodeLabel));
        return;
    }

    if (discountCode.Limited && (discountCode.DiscountsUsed >= discountCode.MaximumAllowed))
    {
        errorMessages.Add(string.Format("The {0} you entered is no longer available",
            registration.EventProfile.DiscountCodeLabel));
        return;
    }

    if (registrant.DiscountCodeApplied != discountCode.DiscountCodeId)
    {
        if (registrant.DiscountCodePercentage <= discountCode.DiscountPercentage)
        {
            registrant.DiscountCodeApplied = discountCode.DiscountCodeId;
            registrant.DiscountCodePercentage = discountCode.DiscountPercentage;
            registrant.DiscountCodeAmountPerLineItem = discountCode.DiscountAmount;
        }
    }
}

I tend to think the refactored code is easier to read and maintain.

Submit this story to DotNetKicks

0 comments:

Post a Comment