17

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?

Community
  • 1
Miquel
  • 281
  • 1
  • 8
  • 6
    This is something I've spent a lot of time thinking about, and I finally came to an answer that makes me happy. No reason to retype it: http://caffeineoncode.com/2012/04/how-long-should-functions-be/ – riwalk Aug 14 '12 at 13:58
  • 2
    And btw, you're calling `customer.getCredit()` more than once in both solutions. The variable `credit` never gets used and you're (at best) repeating yourself. There's a lesson here: don't spend too much time focusing on the little things. The `getCredit()` thing is a lot bigger of a problem. – riwalk Aug 14 '12 at 14:07
  • Where is the knowledge whether credit will be ok? –  Aug 14 '12 at 15:19

9 Answers9

20

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

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • 10
    I followed Robert Martin's advice once. Not twice, but once. My code had me using the "Go To Definition" feature of visual studio like there was no tomorrow. Eventually, there's a point where you need to stop writing functions and start writing code. – riwalk Aug 14 '12 at 13:59
  • 1
    @Stargazer712: Bob Martin takes it to the extreme, of course. But typically, I don't *start* with refactoring to functions. First I write/add some code to a function, and when seeing that the function now does too many things at once, then I refactor until I feel it is small enough. Often, this will make a lot of comments obsolet (at least when choosing good names). And it saves debugging time, because readable code to my experience has much less bugs. – Doc Brown Aug 14 '12 at 14:30
  • 9
    I agree with @Stargazer712. That dude has some serious dogma issues. I threw that book in the trash because its like he has no common sense. Its his way or no way. – Rig Aug 14 '12 at 14:35
  • 2
    @Rig: IMHO throwing a book into trash is not really a sign of beeing open for new ideas of in coding. Perhaps you are a little bit too self-confident in your believe what "common sense" is? But alas, I guess you are not alone, Bob Martin's ideas are a foundation of endless discussions and holy-wars in the community. – Doc Brown Aug 14 '12 at 14:50
  • 2
    @DocBrown I am very open to new ideas. I spend a lot of free time learning best practices and new techniques. The book is poor because the way he proposes those ideas as the only possible solution, the writing is pretty poor, and I don't need 3 word sentences to tell me what is right or wrong. There are better books on the market to describe quality code which left me surprised how much hype that book has. I don't need or want to be told. Explain. Don't tell. – Rig Aug 14 '12 at 14:54
  • 8
    The problem with the example Uncle Bob shows there is that he's increased the scope of all the local variables into instance members. As a result whatever gain he might have by his function decomposition is lost because of the increased variable scope. – Winston Ewert Aug 14 '12 at 15:46
10

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.

Sergei Rogovtcev
  • 211
  • 1
  • 3
  • Agreed. To be honest, I was focused on illustrating a point, and this does not come from a real system. Still I hope the example illustrates my point. – Miquel Aug 14 '12 at 11:55
6

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

Rafael Piccolo
  • 202
  • 1
  • 6
  • I'm sorry, but -1. If you see a function that is 12 lines long (including the whitespace!) and say "there is more refactoring to be done there," then you have some serious issues. The fact that you then say that you should use a class instead of a 12 line function, it makes my -1 even more solid. When you start talking about encapsulating the data structure (just in case it changes!), it makes me wish I had another vote to cast. – riwalk Aug 15 '12 at 14:29
  • @Stargazer712 He is not talking about 12 lines long functions, he asked about concepts and best practice. The 12 lines long function he posted was just a **simplified example**, just to **illustrate** his question. We all post simplified versions of our problems when writing a question. Would you prefer a full program with hundreds of lines in this tinny space? – Rafael Piccolo Aug 15 '12 at 15:04
  • He's asking which example is better, which means that **yes, he is** talking about the 12 line long function. Even if I were to believe that he was metaphorically speaking about a massively large block of code, your example of encapsulating the data structure (just in case it changes!) *still* warrants the downvote. Your attitude towards programming reminds me of this blog: http://chaosinmotion.com/blog/?p=622 – riwalk Aug 15 '12 at 15:35
  • Well, I didn't said *never* use collections, or *always* encapsulate *every* collection. These radical words are dangerous in software development. Things change, even the ones you never thought, so, the more prepared you are for changes, the better when they come. It's up to the developer to decide when to do it or not. `Set` has its limitations, if the order of the items becomes important, good luck in changing the same thing in dozens of places. Plus, there is the encapsulation issue: the collection should know how to handle its own items, not external classes. – Rafael Piccolo Aug 15 '12 at 17:08
  • Any good IDE will help you change something like that in minutes. You're suggesting that he program for requirements that **do not exist**. You're envisioning a beautiful architecture in your head that will make things easier, but the problem is that the ideas stay there--*in your head*. When someone else comes along, it is HIGHLY unlikely that they will take the time to understand the mountains of code that you've written. What is much more likely is that they will just tag something on, and the result is lava flow programming accompanied by mountains of useless code. – riwalk Aug 15 '12 at 17:37
  • I don't know where is this mountain of code you said, but the point I was trying to make was not about changing structures, but about encapsulation. For example, if there is an OrderProcessor class and this class needs the total value of the items, should it do the calculation? No, it should ask it for the collection. For the processor class, is doesn't matter if the collection is a set, an array or whatever, it just needs the total value. The changing structure possibility is a bonus you get for doing that, not the primary motivation. – Rafael Piccolo Aug 17 '12 at 13:18
5

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.

Chris Bye
  • 534
  • 2
  • 11
4

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.

Winston Ewert
  • 24,732
  • 12
  • 72
  • 103
3

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.

GSto
  • 8,531
  • 7
  • 39
  • 59
3

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.

K..
  • 606
  • 6
  • 18
2

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

Forty-Two
  • 179
  • 3
1

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.

Chris Cudmore
  • 553
  • 4
  • 13