Hiding away complexity with sub functions

Question

I am having a discussion on code style, and it's starting to sound like a "matter of taste". I strongly believe otherwise, so I'm writing this to get your opinion and learn from your arguments for and against.

The issue is that of hiding complexity by replacing a bunch of lines of code with a method. This discussion is unrelated to reusable functions, and I'm trying to make the argument for simple code organization, not for factoring out common functionality.

So, take this example in loose java:

public void purchase(Customer customer, Set<Item> items, Warehouse warehouse){
     int credit = customer.getCredit();    
     int orderPrice;

     for(Item item: items){
         orderPrice+=items.getPrice();
     }

     if(orderPrice + customer.getPendingPayments() > customer.getCredit()) {   
           warehouse.reserveStock(items);
           transportCenter.allocateTransport();
           customer.bill(items, orderPrice);
     }
}

As opposed to this:

public boolean isCreditOk(Customer customer, Set<Item> items){
     int credit = customer.getCredit();    
     int orderPrice;

     for(Item item: items){
         orderPrice+=items.getPrice();
     }
     return  orderPrice + customer.getPendingPayments() > customer.getCredit();
}

public void purchase(Customer customer, Set<Item> items, Warehouse warehouse){
     if(isCreditOk(customer, items)){
           warehouse.reserveStock(items);
           transportCenter.allocateTransport();
           customer.bill(items, orderPrice);
     }
}

The whole discussion now is: would you have created the isCreditOk method or would you have left if inline? When do you do one or the other? does it depend, in a general case, on:

The length of the function to extract
How many sub-functions and sub-subfunctions isCreditOk will end up having

Again, this is not done because isCreditOk will be used often, but because it makes the code easier to read (in my opinion).

Can I have your comments? Thanks!

Note: I have found this question to be related: Is there such a thing as having too many private functions/methods?

Doc Brown · Answer 1 · 2012-08-15T05:59:24.250

Here is what Robert C. Martin (the "clean code" guy) would do:

http://blog.objectmentor.com/articles/2009/09/11/one-thing-extract-till-you-drop

So I guess he would split up your isCreditOk function further like this

public int calculateOrderPrice(Set<Item> items)
{
     int orderPrice=0;
     for(Item item: items){
         orderPrice+=items.getPrice();
     }
     return orderPrice;
}

public boolean isCreditOk(Customer customer, Set<Item> items){
     int credit = customer.getCredit();    
     int orderPrice = calculateOrderPrice(items);
     int totalPayments = orderPrice + customer.getPendingPayments();
     return totalPayments > credit;
}

And indeed, I think this is a good thing - using functions not just for reuse, but also for building abstractions. The key factor here is good naming of the functions, so you can use them as small building blocks. Then it will increase the readability of the code a lot.

Of course, there are different opinions around how far one should go when refactoring to small functions, and to be honest, I am still not coding like Bob Martin. But the more I have focused the last years on creating small functions, the more I have made the experience that it pays off more or less immediately - my code becomes more readable, needs less comments, has less bugs and is more evolvable (which means, it gets easier to introduce new features into existing code).

score 10 · Answer 2 · answered Aug 14 '12 at 11:43

Your second example is more readable (although I would invert if to reduce nesting and rename isCreditOk to isPurchaseAllowed, and possibly moved the latter to Customer). First one hides code intent, exposing implementation, thus while it's clear what code exactly, it's not clear why it does those things.

Rafael Piccolo · Accepted Answer · 2012-08-14T16:57:13.393

Divide into smaller functions is the best way for sure, at least for human readability.

By extracting a smaller function, you are naming a block of code. That really improves readability.
Readers can have a macro idea of what that function does, without having to bother with details of each step. If you want to see the details, than you look ate the sub-function. This is the Newspaper Metaphor in Uncle Bob's Clean Code.

Generally speaking, complex if statements should be extracted to their own methods. Those nasty ifs should be substituted by a meaningful name that explains what is being tested so readers don't have to parse those huge lines of code.

But besides that, there is more refactoring to be done there. If I were reading the purchase ou isCreditOk methods, I wouldn't want to see that for loop calculating the total value. That's not an easy-read. I'd have to stop my eyes at that point, mentally parse that block to try to figure out what it's doing. The for loop should be extracted to another method called getOrderPrice, getTotalPrice or something like that.

Better than that, the items set could be extract to it's own class: OrderItems. The class would have a method like getTotal that would return the sum of its own items. That's the Single Responsibility Principle, the OrderItems class is the one that handles its own items. In addition to that, the data structure (Set in this case) would be encapsulated and you are free to change to other data structures whenever you want (if you need to change it to a list, for example).

score 5 · Answer 4 · answered Aug 14 '12 at 14:56

This decision hinges on two (possibly contradictory) principles/guidelies: Single Responsibility Principle and You ain't gonna need it. Per SRP you should be thinking about class boundaries, and not method boundaries here, but for the sake of this question, let's just talk about methods. A huge benefit you get out of SRP is that each of your classes/methods only has a single reason to change.

Let's say that in the future, you add more warehouses, so you need to ask some order dispatcher object which warehouse has the stock necessary for the order. If you've gone with the giant method as implementation, that method is going to start growing. Not only are you now changing things that are unnecessarily and dangerously close to functionality that is already tested, it's going to be common to let some of this functionality bleed around the method, into unrelated code. The monolithic method will become a maintenance hassle when you decide you need to implement tax on some items, and charge shipping on orders as well, based on the total weight of the order. Breaking up this behavior into (initially) small methods with a single reason to change will make future modifications much easier and safer, because changes are made in small units of code, and invisible side effects are much more difficult to accidentally introduce.

The other side of this coin is the fact that you might not ever need this flexibility (YAGNI), and spiking up a quick and dirty solution for a proof of concept might be exactly what you really need. "Shipping your software" is a feature, some would argue the most important feature possible; spending hours of work on following principles when you don't ever see the upside just isn't worth it when you could ship instead.

The reality here is that it's a judgement call that's entirely up to you (or maybe your boss/tech lead). Modern IDEs make it really easy to navigate around multiple classes and methods, and good practices with namespaces and directory structure make it very easy to keep track of where everything is. If you just want to see all the functionality in one method in one class, then you may be a procedural programmer who doesn't want to do object-oriented design. That's fine, but you should make the decision deliberately. Any OO guideline can be abused and carried to an extreme, and learning when to apply various "rules" effectively can be a tricky skill to learn. If it were easy, everyone would do it, all you need to do is keep asking questions and reflecting on experience.

score 4 · Answer 5 · answered Aug 14 '12 at 15:43

I think splitting up functions is a good idea. But you need to be careful. If you simply split functions for splitting's sake you end up with a bit of a mess. When I've seen Uncle Bob's stuff he often seems to be splitting without thought. Decomposing a function requires care and skill, and shouldn't be done lightly.

public boolean isCreditOk(Customer customer, Set<Item> items){
     int credit = customer.getCredit();    
     int orderPrice;

     for(Item item: items){
         orderPrice+=items.getPrice();
     }
     return  orderPrice + customer.getPendingPayments() > customer.getCredit();
}

I'm sure this is just an example, but I'm going to point out where I think it went wrong. You've combined two distinct tasks, calculating the total for an order, and checking for credit. This function would be better written as:

public boolean isCreditOk(Customer customer, int amount)
{
    return amount <= customer.getCredit() - customer.getPendingPayments();
}

This also suggests that the function would do well to be a method on Customer. It'll be much easier to test this function if you can simply pass dollar amounts rather then constructing lists of items.

I aim to create functions which could in principle be reused. I don't necessarily expect to reuse them, but when a function is reusable that tells me I've a useful decomposition.

score 3 · Answer 6 · answered Aug 14 '12 at 13:01

I do prefer the second example. My reasoning is that even if this is the only place in the code you need to check the user's credit right now, does not mean that this will always be the case. Now that you have abstracted out this code it will be more reusable if you ever run into a situation where you do need it.

However, like anything else this can be taken too far. Personally I think that the final example in Robert Martin's site that @Doc Brown goes a bit too far.

score 3 · Answer 7 · answered Aug 14 '12 at 13:20

I split up function even if I don't plan to reuse the functionality. Also I try to keep function size smaller than the screen-size.

This brings 2 advantages, first I can grasp what the function does without scrolling and I can minimize the comments, because every function gets a speaking name.

score 2 · Answer 8 · answered Aug 14 '12 at 15:17

2

I would rather see IsCreditOK be a property of Customer rather than it's own method. I think it's a cleaner approach...

answered Aug 14 '12 at 15:17

Forty-Two

179

score 1 · Answer 9 · answered Aug 14 '12 at 19:31

I'd modify the Customer class to have a getAvailableCredit() method.

Ideally, I'd like to see this as the "Working" line of code:

if (order.getTotal() <= customer.getAvailableCredit() ) order.Ship();

In particular, I'd like to see a PurchaseOrder class that would encapsulate the the set of items, which would total itself, and add an relevant taxes, shipping charges etc.

I would also probably go so far as to separate the Credit functions out of the Customer class, by making a CreditAccount class which would be owned by a Customer.

Hiding away complexity with sub functions

9 Answers9

Linked