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.
0 comments:
Post a Comment