refactoring long methods with fluent interfaces

I'd like to know your opinion about using the fluent interface pattern to refactor a long method.

http://en.wikipedia.org/wiki/Fluent_interface

The fluent pattern is not included in the refactoring books.

for example, say you have this long method (with a long name as it does many things)

class TravelClub {

  开发者_运维知识库 Receipt buyAndAddPointsAndGetReceipt(long amount, long cardNumber) {
    buy(amount);
    accumulatePoints(cardNumber);
    return generateReceipt();

   }

   void buy(int amount) {...}

   void accumlatePoints(int cardNumber) {...}

   void generateRecepit() {...}

}

called as:

Receipt myReceipt = myTravelClub.buyAndAddPointsAndGetReceipt(543L,12345678L);

That could be refactored to:

class TravelClub {

   TravelClub buy(long amount) {
    //buy stuff
    return this;
   }

   TravelClub accumulatePoints(long cardNumber) {
    //accumulate stuff
    return this;
   }

   Receipt generateReceipt() {
    return new Receipt(...);
   }


}

and called as:

Receipt myReceipt = myTravelClub.buy(543L).accumulatePoints(12345678L).generateReceipt();

from my point of view this is a quite good manner to decompose a long method and also to decompose its name.

what do you think?

It has a problem in that you have to remember both to accumulate the points and perform the purchase (and generate the receipt, which is less of a problem as I assume that action has no side effects). In my mind, point accumulation should come automatically when performing a purchase. It's also rather natural that you get a receipt when performing a purchase, so in a way, your initial method was fine, except that it doesn't read very well.

If you want a fluent interface I'd introduce an extra class which gently guides the client code into doing the right thing (assuming that all purchases happen with a card and accumulate points the same way):

class TravelClub {

   OngoingPurchase buyAmount(long amount) {
      return new OngoingPurchase(amount);
   }

   private Receipt buyAndAddPointsAndGetReceipt(long amount, long cardNumber){
      // make stuff happen
   }

   public class OngoingPurchase {
      private final long amount;
      private OngoingPurchase(long amount){
         this.amount = amount;
      }
      public Receipt withCard(long cardNumber){
         return buyAndAddPointsAndGetReceipt(long amount, cardNumber);
      }
   }

}

// Usage:
Receipt receipt = travelClub.buyAmount(543).withCard(1234567890L);

This way, if you forgot to call withCard, nothing happens. It's easier to spot a missing transaction than an incorrect transaction, and you can't get a receipt without performing a complete transaction.

Edit: As an aside, it's funny to think that we do all this work to make methods with many parameters readable, when for example named parameters would make the problem go away completely:

Receipt r = travelClub.makePurchase(forAmount: 123, withCardNumber: 1234567890L);

My counter-question is then, what is the expected behavior if someone instead calls:

myTravelClub.accumulatePoints(10000000L);

without calling buy? Or generating the receipt before the purchase? I think that fluent interfaces still need to adhere to other OO conventions. If you really want a fluid interface, then the buy() method would have to return another object, not the TravelClub itself, but a "purchase object" that has the accumulatePoints() and generateReceipt() methods.

Maybe I am reading to much into the semantics of your example, but there is a reason why the wikipedia example has methods that logically can be called in any order. I think the Hibernate criteria API is another good example.

A long method is not the same as a method with a long name. In your case, the only thing I'd change is the method name:

public Receipt buy(long amount, long cardNumber) {
    buy(amount);
    accumulatePoints(cardNumber);
    return generateReceipt();
}

(and think of a more descriptive name for the private buy method) because all three things ("buying", accumulatePoints and getting the receipt) always happen together, so from the view of calling code, they can be a single operation. From an implementation perspective, having a single operation is easier, too. KISS :-)

The advantage of using a single method is that the same sequence is always called. e.g., you can't skip accumulatePoints like you could in the fluent-interface example you provided.

If the only way to call these methods would be in the same sequence as in your first block of code, keep it as a single function. If, however, any subset of manipulations can be done on TravelClub before a receipt is generated, then by all means use a fluent interface. This is one of the best ways (if not the best) to overcome the 'combinitorial explosion' code smell.

As long as you have used proper validations, Fluent interfaces are much more easier to understand, for example it could be like follows,

class TravelClub {

   TravelClub buy(long amount) {
    buy(amount);
    return this;
   }

   TravelClub accumulatePoints(long cardNumber) {
    if (!bought)
    {
        throw new BusinessException("cannot accumulate points if not bought");
    }
    accumulatePoints(cardNumber);
    return this;
   }

   Receipt generateReceipt() {
    if (!bought)
    {
       throw new BusinessException("cannot generate receipts not bought");
    }
    return new Receipt(...);
   }
}

It seems to me that part of the difficulty here is in choosing a good descriptive name that encompasses everything that a method does. The problem naturally is that sometimes you have a lot of complicated logic that you can't easily describe with a simple name.

In the case posed in your code example, I'd be tempted to simplify the name of the method itself to something a little more generalized:

Receipt Transaction(long amount, long cardNumber) 
{
    buy(amount);
    accumulatePoints(cardNumber);
    return generateReceipt();
}

So what about this logic problem I mentioned? That itself boils down to whether or not your method is very fixed in what it does. If it is only possible to complete the transaction using the Buy->Points->Receipt sequence, then a simpler name works, but so does the more descriptive name, and a fluent interface may be a reasonable alternative.

What about those cases where the customer doesn't have a rewards card, or doesn't wish to have a receipt? What about those situations where several items might be purchased in a single transaction - assuming of course that the buy method might represent a purchase item and not simply a total that was calculated elsewhere? Once you start introducing questions/choices into the sequence, the design becomes a little less obvious and the naming a lot harder. You certainly wouldn't want to use a crazy long name like:

BuyAndAddPointsIfTheCustomerHasACardAndReturnAReceiptIfTheCustomerAsksForIt(...)

Sure, it tells you exactly what it does, but it also highlights a potential problem in that the method is possibly responsible for too many things, or that it might be hiding a more complex code smell behind one of the methods that it calls. Likewise a simple method name like "Transaction" could be oversimplifying a complex problem that needs to be better understood.

A fluent interface can be of great benefit here provided it guides the developer to make sensible decisions about how to apply the fluent methods being called. If the calling sequence is important, you need to restrict the return types to the next choices in the sequence. If the calling sequence is less important, then you can use a return type with a more generalized interface that allows a selection of methods to be called in any sequence.

As to whether or not to use a fluent interface at all, I don't think it should be decided merely as a means to decompose difficult to name methods. You are making a design choice that you are going to need to live with for the lifetime of the product, and from a maintenance perspective, I've found that fluent interfaces can make the design more difficult to visualize and to organize and maintain in your code. Ultimately you need to decide if this is something you can live with as a trade-off against the benefits it gives you. For me, I usually start by asking if the use-case combinations are fixed and simple, or if they are relatively endless. If the latter, a fluent interface may help to keep your code cleaner and easier to use in multiple scenarios. I would also consider whether the code belongs to a more generalized layer such as an API for example where a fluent interface may work nicely, or something more specialized.