4

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?

ejaenv
  • 2,117
  • 1
  • 23
  • 28
  • 1
    Just curious... isn't this reverse of Facade pattern where in you try to hide the complexities of having multiple methods or interfaces by providing simple to use method or interface. – Sandeep G B Sep 14 '11 at 07:18
  • @Sandeep, thanks for your point. True, but IMO the tray-off of this pattern is that methodsbecome very large (plus its names, plus the number of params) – ejaenv Sep 14 '11 at 08:41

6 Answers6

5

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);
gustafc
  • 28,465
  • 7
  • 73
  • 99
3

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.

waxwing
  • 18,547
  • 8
  • 66
  • 82
2

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 :-)

meriton
  • 68,356
  • 14
  • 108
  • 175
  • the good style (Clean Code book) says that a method's name should say the programmer what the method does. So IMO long methods involves long names. Maybe this is a new question in stackoverflow. – ejaenv Sep 14 '11 at 08:32
  • thanks for your point. But having a single operation causes having more than one level of abstraction. That's why the long method is decomposed in smaller methods. – ejaenv Sep 14 '11 at 08:48
  • 1
    Yes, it should say what the method does, but at what level of detail|?Is it important that a receipt is "generated" or is it just something I get when buying stuff? Isn't me getting a recept obvious from the method return type? – meriton Sep 14 '11 at 08:54
  • The implementation is already decomposed. I am talking about the public interface of the class. – meriton Sep 14 '11 at 08:58
0

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.

Eran Zimmerman Gonen
  • 4,375
  • 1
  • 19
  • 31
0

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(...);
   }
}
Low Flying Pelican
  • 5,974
  • 1
  • 32
  • 43
0

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.

S.Robins
  • 703
  • 6
  • 6