Refactoring Day 30 : Return ASAP

This topic actually came up during the Remove Arrowhead Antipattern refactoring. The refactoring introduces this as a side effect to remove the arrowhead. To eliminate the arrowhead you return as soon as possible.

   1: public class Order

   2: {

   3:     public Customer Customer { get; private set; }

   4:  

   5:     public decimal CalculateOrder(Customer customer, IEnumerable<Product> products, decimal discounts)

   6:     {

   7:         Customer = customer;

   8:         decimal orderTotal = 0m;

   9:  

  10:         if (products.Count() > 0)

  11:         {

  12:             orderTotal = products.Sum(p => p.Price);

  13:             if (discounts > 0)

  14:             {

  15:                 orderTotal -= discounts;

  16:             }

  17:         }

  18:  

  19:         return orderTotal;

  20:     }

  21: }

The idea is that as soon as you know what needs to be done and you have all the required information, you should exit the method as soon as possible and not continue along.

   1: public class Order

   2: {

   3:     public Customer Customer { get; private set; }

   4:  

   5:     public decimal CalculateOrder(Customer customer, IEnumerable<Product> products, decimal discounts)

   6:     {

   7:         if (products.Count() == 0)

   8:             return 0;

   9:  

  10:         Customer = customer;

  11:         decimal orderTotal = products.Sum(p => p.Price);

  12:  

  13:         if (discounts == 0)

  14:             return orderTotal;

  15:  

  16:         orderTotal -= discounts;

  17:  

  18:         return orderTotal;

  19:     }

  20: }

This is part of the 31 Days of Refactoring series. For a full list of Refactorings please see the original introductory post.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Sean Chambers

I am a Senior software developer from Palm Coast, Florida. An advocate of Domain Driven Design, Behavior Driven Development, creator of FluentMigrator and community activist. I am married to my beautiful wife Erin and am the proud father of two wonderful children. I currently reside at ACI, a local insurance industry/mortgage software company that excels in creating solutions using Agile methodologies.
This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • Jordi

    Why?

    (and again the bahavior in the two examples is different, because the Customer field may not be updated in the second example)

    Why not do this?:
    Customer = customer; // assuming the first example was the correct one
    return products.Sum(p => p.Price) – discounts;

  • Pavan Kulkarni

    Sean,
    Great job here! I’ve been following your refactoring series all along. However, I don’t agree to this particular refactoring method. Pre-mature returns, I’ve been told, should never be encouraged.

  • Udit Handa

    Even I disagree on this. What if we are doing begin and end of function logging. We have to write the same code everywhere we return.

  • http://richarddingwall.name Richard Dingwall

    Arrowheads are one of the most common smells I see in legacy code. Replacing huge nested IFs with guard clauses and early bail-outs is always one of the first things I do, and it works wonders to simplify long functions down so they are ready for extract method etc.

    Udit: I would do begin and end of function logging as in the caller or with a using() block.

  • http://azurecoding.net mbrown77@gmail.com

    @Udit: A Try Finally works great if you’re doing Pre/Post Processing.

  • http://www.lostechies.com/members/colinjack/default.aspx colinjack

    To me thats introducing a guard.

  • Jake Tummond

    Enjoying the series, although I seem to be reading from end to beginning.

    Why make this function impure by passing a Customer object? I would extract everything but the assignment into a pure static function.

    Second, it seems some corner cases are uncovered? What happens if discount > orderTotal? Do you send customer some money? Maybe it’s an exception?

    Finally, I think Sum will return zero for an empty list.

    I would probably rewrite as:
    {
    decimal orderTotal = products.Sum(p => p.Price);
    return Math.Max(0, orderTotal – discount);
    }

  • Jake Tummond

    Also, FWIW, I hate reading code with early returns. I worked with a fellow who made it his mission to refactor deeply nested code this way. I found it impossible to read and since he left, I’ve been slowly doing it the right way. If your code is deeply nested, I don’t think early returns are the way to fix it. It is better to study the problem an fix it by modularizing.

  • George

    I’ve been coding for about 20 years, and I disagree with this one, although I may be in the minority these days. I “bend over backward” to code so that every method’s expected code path (aside from exceptions) has exactly one point-of-return – at the end. I usually manage to keep the code arrowheads to a minimum as well. My opinion and inclinations are aligned with Jake Tummond’s FWIW statement. I deplore that advice is being given out to impressionable beginners and junior developers to code like this.

  • Cameron Scott

    Great work dude!!! How do you write such article son complex problem so easily.
    Hair loss