Thread: Should functions be _this_ small?

  1. #1
    Set Apart -- jrahhali's Avatar
    Join Date
    Nov 2002
    Posts
    256

    Should functions be _this_ small?

    I am reading through the book Clean Code by Robert C. Martin. It is a book of advice on how to write more readable, and consequentially, more maintainable code. There is a chapter on writing good functions. His section on how small functions should be seems pretty extreme, and I wanted to hear the communities opinion on what he says. The following are some brief excerpts, followed by a couple examples.

    Quote Originally Posted by pg. 34
    Small!
    The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that....
    How short should a function be? In 1999 I went to visit Kent Beck at his home in Oregon...At one point he showed me a cute little Java/Swing program that he called Sparkle...Every function in this program was just two, or three, or four lines long. Each was transparently obvious. Each told a story. And each led you to the next in a compelling order. That's how short your functions should be!
    Quote Originally Posted by pg. 35
    Blocks and Indenting
    This implies that the blocks within if statements, else statements, while statements, and so on should be one line long. Probably that line should be a function call. Not only does this keep the enclosing function small, but it also adds documentary value because the function called within the block can have a nicely descriptive name.

    This also implies that functions should not be large enough to hold nested structures. Therefore, the indent level and a function should not be greater than one or two. This, of course, makes the functions easier to read and understand.
    Quote Originally Posted by pg. 302
    G30: Functions Should Do One Thing
    ...For example:
    Code:
    public void pay() {
        for (Employee e: employees) {
            if (e.isPayday()) {
                Money pay = e.calculatePay();
                e.deliverPay(pay);
            }
        }
    }
    ...This code would be better written as:
    Code:
    public void pay() {
        for (Employee e: employees)
            payIfNecessary(e);
    }
    
    private void payIfNecessary(Employee e) {
        if (e.isPayday())
            calculateAndDeliverPay(e);
    }
    
    private void calculateAndDeliverPay(Employee e) {
        Money pay = e.calculatePay();
        e.deliverPay(pay);
    }
    There is another example found at this link of a team who tried implementing the small function advice in their java code base.

    I have not made up my mind what I think about breaking functions apart like this. What do you guys think?
    Clear the mines from our Shazbot!
    Get the enemy Shazbot!

  2. #2
    Registered User
    Join Date
    Jun 2013
    Posts
    66
    Uncle Bob is a pretty extreme Agile methodology guy, so keep that in mind when reading his advice.

  3. #3
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    I think that functions should do one thing and do it well. A long function is less likely to be doing that than a short function. On the other hand, entering a function implies some mental context switch. This is good when it means that you can ignore whatever is not needed in the context, but if the context actually is the same and you are merely creating a name for a partial block of code because of a perceived best practice of having functions that are "just two, or three, or four lines long", then you incur the cost of the mental context switch without any real benefit.

    In the example that you quoted, I would argue that payIfNecessary is good: it handles the payment for a single Employee, thus in its context you only need to think about a single Employee. However, I would argue that calculateAndDeliverPay is quite unnecessary, even bad: its name describes something that is already obvious from the names of the two functions that it calls.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  4. #4
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    Code:
     private int compareThisTo(final ExampleClass _otherExampleClass) 
     { 
       if (equals(_otherExampleClass)) 
       {
         return 0;
       }
    return compareCreatedDates(_otherExampleClass);
    }
    
    private int compareCreatedDates(final ExampleClass _otherExampleClass)
    {
     if (isThisAndOthersCreatedDateNull(_otherExampleClass)) 
     {
       return 0;
     }
     if (isThisCreatedDateNull()) 
     {
       return -1;
      }
      if (isOthersCreatedDateNull(_otherExampleClass)) 
     {
       return 1;
     }
      return compareCreatedDatesThatAreNotNull(_otherExampleClass);
    }
    
    private boolean isThisAndOthersCreatedDateNull(final ExampleClass _otherExampleClass) 
     {
        return isThisCreatedDateNull() &&isOthersCreatedDateNull(_otherExampleClass);
    }
    private boolean isThisCreatedDateNull() 
    {
      return isOthersCreatedDateNull(this);
    }
    private boolean isOthersCreatedDateNull(final ExampleClass _otherExampleClass) 
    {
       return _otherExampleClass.getCreatedDate() == null;
    }
    private int compareCreatedDatesThatAreNotNull(final ExampleClass _otherExampleClass) 
    {
     return _otherExampleClass.getCreatedDate().compareTo(getCreatedDate());
    }
    O_o

    Thanks for posting this link jrahhali; I don't often see such "Enterprise" level code.

    It gave me a tickle. ^_^

    Soma
    Last edited by phantomotap; 06-29-2013 at 01:21 PM.
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  5. #5
    Set Apart -- jrahhali's Avatar
    Join Date
    Nov 2002
    Posts
    256
    @phantomotap: O_o is right!

    Quote Originally Posted by laserlight
    I would argue that calculateAndDeliverPay is quite unnecessary, even bad: its name describes something that is already obvious from the names of the two functions that it calls.
    I think you're correct here. payIfNecessary() already means calculateAndDeliverPay() and shouldn't have been split into another function. Martin might have been doing this to abide by his rule of thumb of separating out the body of an if statement into it's own function.
    Clear the mines from our Shazbot!
    Get the enemy Shazbot!

  6. #6
    Make Fortran great again
    Join Date
    Sep 2009
    Posts
    1,413
    Apparently I'm a bad programmer, because it's very much like me to do something like the example in the original post. It looks simple and straightforward to me, no reason to split it up into 3 functions. What Soma posted gave me cancer, I just went terminal.

    I think that functions should be on the smaller side, but it's more about what is simple, straightforward, and followable. Comments/documentation rule all; with good notes it should be easier to follow code, especially your own.

  7. #7
    Registered User
    Join Date
    Jun 2013
    Posts
    66
    Apparently I'm a bad programmer, because it's very much like me to do something like the example in the original post.
    There is far too much variance in purposeful code to define strict rules such as the length of functions. That's why programming is as much an art form as it is a science; at a certain point code must simply feel right. You can't teach that, it's all about experience from trial and error.

  8. #8
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    Apparently I'm a bad programmer, because it's very much like me to do something like the example in the original post.
    O_o

    Well, I would split the iteration from the operation (two functions), but yeah, I don't see a reason for `calculateAndDeliverPay' to be a thing that exists.

    What Soma posted gave me cancer, I just went terminal.
    ;_;

    Sorry, I should have put a warning label on it.

    Soma
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  9. #9
    Registered User
    Join Date
    Mar 2011
    Posts
    596
    G30: Functions Should Do One Thing

    In post #1, don't the two different examples of the function pay() do exactly the same thing?
    Not in the same manner, but to the same exact end? Aren't they both doing the same "one thing"?

    I find nesting easier to follow when it's all in one place, all in one function. Tracing
    through nested function calls is what I find confusing, when it's broken down into
    such small pieces.

    It almost looks like functions are being used to simply add extra levels of labelling
    things.

  10. #10
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Quote Originally Posted by megafiddle
    In post #1, don't the two different examples of the function pay() do exactly the same thing?
    Not in the same manner, but to the same exact end? Aren't they both doing the same "one thing"?
    In the net effect, yes, and indeed that must be so otherwise the revised version has a bug. The "one thing" -- the overall responsibility of the function -- is payment of all employees who need to be paid. However, the difference is that the revised version does it better by delegating the payment of each employee to a function whose sole responsibility is the payment of an employee. Thus, in a sense, the original version does two things: payment of all employees, and the payment for each employee.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  11. #11
    Registered User
    Join Date
    Mar 2011
    Posts
    596
    I guess I am counting everything that happens from when the function is called,
    to when it returns, as the "one thing" that it does.

    Maybe a better question, is why separate that particular part into a separate function?
    It seems to be a integral part of what pay() does. Also, if I saw the function pay() called
    from within main(), I would not know what it did, regardless of whether pay() was broken
    down into smaller functions. It wouldn't help at all at the higher lever. A more descriptive
    name would help, but that would have nothing to do with whether the function was broken
    down or not.

  12. #12
    Master Apprentice phantomotap's Avatar
    Join Date
    Jan 2008
    Posts
    5,108
    I guess I am counting everything that happens from when the function is called, to when it returns, as the "one thing" that it does.
    O_o

    If that is true, how do you design anything without overwhelming every single function?

    I mean, `main' is a function that is called and also returns. With that same view, does `main' do "one thing"?

    I don't know, maybe I'm reading too much into the comment, or maybe you aren't parsing "one thing" as laserlight intends.

    Look at the "one thing" concept from the perspective of implementation instead of interface.

    Code:
    class SVector
    {
        // ...
        void push_back
        (
            int fData
        )
        {
            if(mSize == mCapacity)
            {
                // resize storage
            }
            // copy `fData' into storage
        }
        // ...
    };
    Code:
    class SVector
    {
        // ...
        void push_back
        (
            int fData
        )
        {
            if(isFull())
            {
                expand();
            }
            insert(mLast, fData);
        }
        // ...
    };
    The difference isn't that `push_back' does actually something different but for what it is responsible.

    The design:

    `isFull' considers the state of storage.
    `expand' increases the storage.
    `insert' moves the data into storage.

    `push_back' isn't responsible for all of these things.

    Soma
    “Salem Was Wrong!” -- Pedant Necromancer
    “Four isn't random!” -- Gibbering Mouther

  13. #13
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    28,413
    Quote Originally Posted by megafiddle
    Maybe a better question, is why separate that particular part into a separate function?
    I have explained that in post #3 and again from a different perspective in post #10.

    Quote Originally Posted by megafiddle
    Also, if I saw the function pay() called
    from within main(), I would not know what it did, regardless of whether pay() was broken
    down into smaller functions. It wouldn't help at all at the higher lever. A more descriptive
    name would help, but that would have nothing to do with whether the function was broken
    down or not.
    Given the public and private modifiers used in the code snippet, this is presumably a member function/method of a class, maybe in Java. The class is likely to model say, a list of employees. As such, it would be obvious from the context of the caller and the class that that method is called to pay the employees in that list.
    Last edited by laserlight; 07-01-2013 at 01:24 PM.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  14. #14
    Officially An Architect brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,396
    In an example such as this, I would make a quick application of the DRY (Don't Repeat Yourself) principal, make a decision, and then move on.

    EDIT: I guess for code, the principle is Once And Only Once, not DRY, but OAOO isn't a nice to say or write..
    Last edited by brewbuck; 07-01-2013 at 05:30 PM.
    Code:
    //try
    //{
    	if (a) do { f( b); } while(1);
    	else   do { f(!b); } while(1);
    //}

  15. #15
    Registered User
    Join Date
    Mar 2011
    Posts
    596
    Quote Originally Posted by phantomotap View Post
    O_o

    If that is true, how do you design anything without overwhelming every single function?

    I mean, `main' is a function that is called and also returns. With that same view, does `main' do "one thing"?

    I don't know, maybe I'm reading too much into the comment, or maybe you aren't parsing "one thing" as laserlight intends.

    Soma
    Some of it does have to do with my interpreting "one thing" differently, I think.
    My comment describes what I think the beginning and end points are, for whatever
    the function does, ie, the net effect. So I would be including every function called
    within the function (and every function they called, etc).
    Certainly a function can do many things, even completely unrelated things. So I
    didn't mean that it was necessarily doing only one thing, only that if it was, that is
    what I what include in that process.

    My interpretation of "one thing" can certainly be wrong. But can't everything be broken
    down into smaller "one things"? Is it doing one thing only because you can't see what's
    involved in doing it?

    Also there is something else that I am not quite understanding (see below).

    Quote Originally Posted by laserlight View Post
    I have explained that in post #3 and again from a different perspective in post #10.
    Yes, I am still not clear though.

    You can pay a single employee alone. But you cannot pay all employees alone. That requires
    paying individual employees (hence the inclusion of the code for that or the function call).
    So paying all employees in the first example of pay() is doing one thing and paying each
    employee is also doing one thing. Apparently it is doing two things, but you have to allow
    one to be a subset of the other.
    In the second example though, how can you count paying all employees as doing one thing
    since it cannot do it by itself?
    Isn't it incomplete, if you don't count the function call for paying a single employee? And if
    you do count the function call for paying a single employee, aren't you right back to doing
    two things again?
    Last edited by megafiddle; 07-01-2013 at 06:05 PM.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 1
    Last Post: 08-28-2012, 06:11 AM
  2. A Small Problem Using fgets() and functions
    By Shamshir in forum C Programming
    Replies: 7
    Last Post: 04-07-2011, 10:13 PM
  3. Replies: 3
    Last Post: 03-20-2011, 01:39 PM
  4. Reasons for keeping functions small
    By maththeorylvr in forum C Programming
    Replies: 7
    Last Post: 03-26-2006, 12:38 AM
  5. A small problem with a small program
    By Wetling in forum C Programming
    Replies: 7
    Last Post: 03-25-2002, 09:45 PM