219

I have read a book called Clean Code by Robert C. Martin. In this book I've seen many methods to clean up code like writing small functions, choosing names carefully, etc. It seems by far the most interesting book about clean code I've read. However, today my boss didn't like the way I wrote code after reading this book.

His arguments were

  • Writing small functions is a pain because it forces you to move into each small function to see what the code is doing.
  • Put everything in a main big loop even if the main loop is more than 300 lines, it is faster to read.
  • Only write small functions if you have to duplicate code.
  • Don't write a function with the name of the comment, put your complex line of code (3-4 lines) with a comment above; similarly you can modify the failing code directly

This is against everything I've read. How do you usually write code? One main big loop, no small functions?

The language I use is mainly Javascript. I really have difficulties reading now since I've deleted all my small clearly named functions and put everything in a big loop. However, my boss likes it this way.

One example was:

// The way I would write it
if (isApplicationInProduction(headers)) {
  phoneNumber = headers.resourceId;
} else {
  phoneNumber = DEV_PHONE_NUMBER;
}

function isApplicationInProduction(headers) {
   return _.has(headers, 'resourceId');
}

// The way he would write it
// Take the right resourceId if application is in production
phoneNumber = headers.resourceId ? headers.resourceId : DEV_PHONE_NUMBER;

In the book I've read for example comments are considered as failure to write clean code because they are obsolete if you write small functions and often leads to non-updated comments (you modify your code and not the comment). However what I do is delete the comment and write a function with the name of the comment.

Well, I would like some advice, which way/practice is better to write clean code?

25
  • 22
    Just so you know, John Carmack would probably agree with your boss.
    – walen
    Commented Nov 14, 2016 at 8:29
  • 5
    phoneNumber = headers.resourceId ?: DEV_PHONE_NUMBER;
    – Joshua
    Commented Nov 14, 2016 at 19:35
  • 11
    Validate, that you want to work in place, where management told you HOW to do your work, instead of WHAT need to be solved. Commented Nov 16, 2016 at 13:56
  • 8
    @rjmunro Unless you really like your job, keep in mind there are less developers than jobs. So to quote Martin Fowler: "If you can't change your organization, change your organization!" and Bosses telling me how to code is something I advice you should want to change. Commented Nov 18, 2016 at 12:43
  • 23
    DO NOT EVER have an isApplicationInProduction() function! You must have tests, and tests are useless if your code behaves anything different from when it is production. It's like intentionally having untested/uncovered code in production: it doesn't make sense. Commented Nov 19, 2016 at 23:52

16 Answers 16

227

Taking the code examples first. You favour:

if (isApplicationInProduction(headers)) {
  phoneNumber = headers.resourceId;
} else {
  phoneNumber = DEV_PHONE_NUMBER;
}

function isApplicationInProduction(headers) {
   return _.has(headers, 'resourceId');
}

And your boss would write it as:

// Take the right resourceId if application is in production
phoneNumber = headers.resourceId ? headers.resourceId : DEV_PHONE_NUMBER;

In my view, both have problems. As I read your code, my immediate thought was "you can replace that if with a ternary expression". Then I read your boss' code and thought "why's he replaced your function with a comment?".

I'd suggest the optimal code is between the two:

phoneNumber = isApplicationInProduction(headers) ? headers.resourceId : DEV_PHONE_NUMBER;

function isApplicationInProduction(headers) {
   return _.has(headers, 'resourceId');
}

That gives you the best of both worlds: a simplified test expression and the comment is replaced with testable code.

Regarding your boss' views on code design though:

Writing small functions is a pain because it forces you to move into each small functions to see what the code is doing.

If the function is well-named, this isn't the case. isApplicationInProduction is self-evident and it should not be necessary to examine the code to see what it does. In fact the opposite is true: examining the code reveals less as to the intention than the function name does (which is why your boss has to resort to comments).

Put everything in a main big loop even if the main loop is more than 300 lines, it is faster to read

It may be faster to scan through, but to truly "read" the code, you need to be able to effectively execute it in your head. That's easy with small functions and is really, really hard with methods that are 100's of lines long.

Write only small functions if you have to duplicate code

I disagree. As your code example shows, small, well-named functions improve readability of code and should be used whenever eg you aren't interested in the "how", only the "what" of a piece of functionality.

Don't write a function with the name of the comment, put your complex line of code (3-4 lines) with a comment above. Like this you can modify the failing code directly

I really can't understand the reasoning behind this one, assuming it really is serious. It's the sort of thing I'd expect to see written in parody by The Expert Beginner twitter account. Comments have a fundamental flaw: they aren't compiled/interpreted and so can't be unit tested. The code gets modified and the comment gets left alone and you end up not knowing which is right.

Writing self-documenting code is hard, and supplementary docs (even in the form of comments) are sometimes needed. But "Uncle Bob"'s view that comments are a coding failure holds true all too often.

Get your boss to read the Clean Code book and try to resist making your code less readable just to satisfy him. Ultimately though, if you can't persuade him to change, you have to either fall in line or find a new boss that can code better.

32
  • 35
    Small fucntions are easilly unit tested
    – Mawg
    Commented Nov 14, 2016 at 14:35
  • 13
    Quoth @ExpertBeginner1: “I’m tired of seeing tons of little methods all over the place in our code, so from here forward, there's a 15 LOC minimum on all methods.”
    – Greg Bacon
    Commented Nov 14, 2016 at 17:00
  • 44
    "Comments have a fundamental flaw: they aren't compiled/interpreted and so can't be unit tested" Playing the devil's advocate here, this is true also if you replace "comments" with "function names".
    – seldon
    Commented Nov 14, 2016 at 17:01
  • 13
    @mattecapu, I'll take your advocacy and double it right back at you. Any old rubbish developer can waffle on in a comment trying to describe what a piece of code does. Succinctly describing that piece of code with a good function name takes a skilled communicator. The best developers are skilled communicators as writing code is primarily concerned with communicating with other devs, and with the compiler as a secondary concern. Ergo, a good developer will use well named functions and no comments; poor developers hide their poor skills behind excuses for using comments.
    – David Arno
    Commented Nov 15, 2016 at 15:24
  • 5
    @DavidArno All functions have pre- and postconditions, the question is whether you document them or not. If my function takes a parameter, which is a distance in measured feet, you must supply it in feet, not kilometers. This is a precondition. Commented Nov 17, 2016 at 12:38
232

There are other problems

Neither code is good, because both basically bloat the code with a debug test case. What if you want to test more things for whatever reason?

phoneNumber = DEV_PHONE_NUMBER_WHICH_CAUSED_PROBLEMS_FOR_CUSTOMERS;

or

phoneNumber = DEV_PHONE_NUMBER_FROM_OTHER_COUNTRY;

Do you want to add even more branches?

The significant problem is that you basically duplicate part of your code, and thus you aren't actually testing the real code. You write debug code to test the debug code, but not the production code. It's like partially creating a parallel codebase.

You are arguing with your boss about how to write bad code more cleverly. Instead, you should fix the inherent problem of the code itself.

Dependency injection

This is how your code should look:

phoneNumber = headers.resourceId;

There's no branching here, because the logic here doesn't have any branching. The program should pull the phone number from the header. Period.

If you want to have DEV_PHONE_NUMBER_FROM_OTHER_COUNTRY as a result, you should put it into headers.resourceId. One way of doing it is to simply inject a different headers object for test cases (sorry if this is not proper code, my JavaScript skills are a bit rusty):

function foo(headers){
    phoneNumber = headers.resourceId;
}

// Creating the test case
foo({resourceId: DEV_PHONE_NUMBER_FROM_OTHER_COUNTRY});

Assuming that headers is part of a response you receive from a server: Ideally you have a whole test server that delivers headers of various kinds for testing purposes. This way you test the actual production code as-is and not some half duplicated code that may or may not work like the production code.

8
  • 12
    I did consider addressing this very topic in my own answer, but felt it was already long enough. So +1 to you for doing so :)
    – David Arno
    Commented Nov 10, 2016 at 20:35
  • 5
    @DavidArno I was about to add it as a comment to your answer, because the question was still locked when I first read it, found to my surprise that it was open again and so added this as an answer. Maybe it should be added that there are dozens of frameworks/tools for doing automated testing. Especially in JS there seems to be a new one coming out every day. Might be hard to sell that to the boss though.
    – null
    Commented Nov 10, 2016 at 20:59
  • 65
    @DavidArno Maybe you should have broken up your answer into smaller answers. ;)
    – krillgar
    Commented Nov 11, 2016 at 11:05
  • 2
    @user949300 Using a bitwise OR would not be wise ;) Commented Nov 12, 2016 at 12:57
  • 1
    @curiousdannii Yeah, noticed that too late to edit...
    – user949300
    Commented Nov 12, 2016 at 15:29
59

There is no "right" or "wrong" answer to this. However, I will offer my opinion based on 36 years of professional experience designing and developing software systems ...

  1. There is no such thing as "self-documenting code." Why? Because that claim in completely subjective.
  2. Comments are never failures. What is a failure is code that cannot be understood at all without comments.
  3. 300 uninterrupted lines of code in one code block is a maintenance nightmare and highly prone to errors. Such a block is strongly indicative of bad design and planning.

Speaking directly to the example you provided ... Placing isApplicationInProduction() into its own routine is the smart(er) thing to do. Today that test is simply a check of "headers" and can be handled in a ternary (?:) operator. Tomorrow, the test may be far more complex. Additionally, "headers.resourceId" has no clear relation to the application's "in production status;" I would argue that a test for such status needs to be decoupled from the underlying data; a subroutine will do this and a ternary will not. Additionally, a helpful comment would by why resourceId is a test for "in production."

Be careful not to go overboard with "small clearly named functions." A routine should encapsulate an idea more so than "just code." I support amon's suggestion of phoneNumber = getPhoneNumber(headers) and add that getPhoneNumber() should do the "production status" test with isApplicationInProduction()

14
  • 28
    There is such a thing as good comments which are not a failure. However, comments that are nearly verbatim the code they supposedly explain or are just empty comment blocks preceding a method/class/etc. are definitely a failure.
    – jpmc26
    Commented Nov 10, 2016 at 20:55
  • 4
    It's possible to have code which is smaller and easier to read than any English-language description of what it does and the corner cases it does and does not handle. Further, if a function is pulled out into its own method, someone reading the function won't know what corner cases are or are not handled by its callers, and unless the function's name is very verbose someone examining the callers may not know what corner cases are handled by the function.
    – supercat
    Commented Nov 10, 2016 at 22:19
  • 8
    Comments are never intrinsically failures. Comments can be failures, and are so when they are inaccurate. Wrong code can be detected at multiple levels, including by wrong behavior in black box mode. Wrong comments can only be detected by human comprehension in white box mode, through recognition that two models are described and that one of them is incorrect. Commented Nov 11, 2016 at 0:33
  • 8
    @Timbo You mean, "... at least one of them is incorrect." ;)
    – jpmc26
    Commented Nov 11, 2016 at 2:41
  • 6
    @immibis If you can't understand what the code does without comments, then the code probably isn't clear enough. The main purpose for comments is clarifying why the code is doing what it's doing. It's the coder explaining his design to future maintainers. The code can never provide that kind of explanation, so comments fill in those gaps in understanding.
    – Graham
    Commented Nov 11, 2016 at 11:45
50

“Entities must not be multiplied beyond necessity.”

— Occam's Razor

Code has to be as simple as possible. Bugs like to hide in between complexity, because they are difficult to spot there. So what makes code simple?

Small units (files, functions, classes) are a good idea. Small units are easy to understand because there's fewer stuff you have to understand at once. Normal humans can only juggle ~7 concepts at a time. But size isn't just measured in lines of code. I can write as little code as possible by “golfing” the code (choosing short variable names, taking “clever” shortcuts, smashing as much code as possible onto a single line), but the end result is not simple. Trying to understand such code is more like reverse engineering than reading.

One way to shorten a function is to extract various helper functions. That can be a good idea when it extracts a self-contained piece of complexity. In isolation, that piece of complexity is much simpler to manage (and test!) than when embedded in an unrelated problem.

But every function call has a cognitive overhead: I don't just have to understand the code within my current piece of code, I also have to understand how it interacts with code on the outside. I think it's fair to say that the function you extracted introduces more complexity into the function than it extracts. That's what your boss meant by “small functions [are] a pain because it forces you to move into each small function to see what the code is doing.

Sometimes, long boring functions can be incredibly simple to understand, even when they are hundreds of lines long. This tends to happen in initialization and configuration code, e.g. when creating a GUI by hand without a drag-and-drop editor. There's no self-contained piece of complexity you could reasonably extract. But if the formatting is readable and there are some comments, it's really not difficult to follow what is happening.

There are many other complexity metrics: The number of variables in a scope should be as small as possible. That doesn't mean we should avoid variables. It means that we should restrict each variable to the smallest possible scope where it is needed. Variables also become simpler if we never change the value they contain.

A very important metric is cyclomatic complexity (McCabe complexity). It measures the number of independent paths through a piece of code. This number grows exponentially with each conditional. Each conditional or loop doubles the number of paths. There's evidence to suggest a score of more than 10 points is too complex. This means a very long function that maybe has a score of 5 is perhaps better than a very short and dense function with a score of 25. We can reduce the complexity by extracting control flow into separate functions.

Your conditional is an example of a piece of complexity that could be extracted entirely:

function bigFatFunction(...) {
  ...
  phoneNumber = getPhoneNumber(headers);
  ...
}

...

function getPhoneNumber(headers) {
  return headers.resourceId ? headers.resourceId : DEV_PHONE_NUMBER;
}

This is still very on the edge of being useful. I'm not sure whether that substantially decreases complexity because this conditional isn't very conditional. In production, it will always take the same path.


Complexity can never disappear. It can only be shuffled around. Are many small things simpler than few big things? That depends very much on the circumstance. Usually, there's some combination that feels just right. Finding that compromise between different complexity factors takes intuition and experience, and a bit of luck.

Knowing how to write very small functions and very simple functions is a useful skill, because you can't make a choice without knowing the alternatives. Blindly following rules or best practices without thinking about how they apply to the current situation leads to average results at best, cargo-cult programming at worst.

That's where I disagree with your boss. His arguments are not invalid, but neither is the Clean Code book wrong. It's probably better to follow your boss's guidelines, but the very fact that you are thinking about these problems, trying to find a better way, is very promising. As you gain experience, you'll find it easier to find a good factoring for your code.

(Note: this answer is based in parts on thoughts from the Reasonable Code blog post on The Whiteboard by Jimmy Hoffa, which provides a high-level view on what makes code simple.)

1
  • I'm General I liked your response. I do, however take issue with mcabes cyclomatic complexity measure. From what I have seen of it, it does not present a true measure of complexity. Commented Jul 12, 2017 at 14:24
28

Robert Martin's programming style is polarizing. You will find many programmers, even experienced ones, who find a lot of excuses why splitting up "that much" is too much, and why keeping functions a little bit bigger is "the better way". However, most of these "arguments" are often a expression of unwillingness to change old habits, and to learn something new.

Don't listen to them!

Whenever you can save a comment by refactoring a piece of code to a separate function with an expressive name, do it - it will most probably improve your code. You don't have go so far as Bob Martin does it in his clean code book, but the vast majority of code I have seen in the past which caused maintenance problems contained too large functions, not too small ones. So trying to write smaller functions with self-describing names is what you should try.

Automatic refactoring tools make it easy, simple and safe to extract methods. And please, do not take people serious who recommend writing functions with >300 lines - such people are definitely not qualified to tell you how you should code.

14
  • 57
    "Don't listen to them!": given the fact that the OP is asked by his boss to stop splitting code, the OP should probably avoid your advice. Even if the boss is unwilling to change his old habits. Also note that as highlighted by previous answers, both the code of the OP and his boss' code are badly written, and you (intentionally or not) don't mention that in your answer. Commented Nov 10, 2016 at 21:49
  • 10
    @ArseniMourzenko: not everyone of us has to buckle before his boss. I hope the OP is old enough to know when he has to do the right thing, or when he has to do what his boss says. And yes, I was not going into the details of the example intentionally, there are enough other answers already discussing those details.
    – Doc Brown
    Commented Nov 10, 2016 at 22:03
  • 8
    @DocBrown Agreed. 300 lines is questionable for a whole class. A 300 line function is obscene.
    – JimmyJames
    Commented Nov 10, 2016 at 22:27
  • 32
    I've seen many classes that are more than 300 lines long that are perfectly good classes. Java is so verbose that you almost can't do anything meaningful in a class without that much code. So "number of lines of code in a class," all by itself, is not a meaningful metric, anymore than we would consider SLOC a meaningful metric for programmer productivity. Commented Nov 10, 2016 at 23:36
  • 10
    Also, I've seen Uncle Bob's sage advice misinterpreted and abused so much that I have my doubts that it is useful to anyone but experienced programmers. Commented Nov 10, 2016 at 23:38
24

In your case: You want a phone number. Either it is obvious how you would get a phone number, then you write the obvious code. Or it is not obvious how you would get a phone number, then you write a method for it.

In your case, it's not obvious how to get the phone number, so you write a method for it. The implementation is not obvious, but that's why you are putting it into a separate method so you have to deal with it only once. A comment would be useful because the implementation isn't obvious.

The "isApplicationInProduction" method is quite pointless. Calling it from your getPhonenumber method doesn't make the implementation any more obvious, and just makes it harder to figure out what's going on.

Don't write small functions. Write functions that have a well-defined purpose and meet that well-defined purpose.

PS. I don't like the implementation at all. It assumes that absence of phone number means it is a dev version. So if the phone number is absent in production you not only don't handle it, but substitute a random phone number. Imagine you have 10,000 customers, and 17 don't have a phone number, and you're in trouble in production. Whether you are in production or development should be checked directly, not derived from something else.

1
  • 1
    "Don't write small functions. Write functions that have a well-defined purpose and meet that well-defined purpose." that is the correct criterion for splitting code. if a function does too many (like more than one) disparate functions, then split it up. Single Responsibility Principle is the guiding principle. Commented Nov 20, 2016 at 5:45
17

It seems like what you actually want is this:

phoneNumber = headers.resourceId || DEV_PHONE_NUMBER

This should be self-explanatory to anyone who reads it: set phoneNumber to the resourceId if it's available, or default to DEV_PHONE_NUMBER if it's not.

If you truly want to set that variable only in production, you should have some other, more canonical app-wide utility method (that doesn't require parameters) to determine where you're running from. Reading the headers for that information doesn't make sense.

1
  • 1
    It's self explanatory what it does (with a bit of guessing what language you are using), but it's not at all obvious what is going on. Apparently the developer assumes that the phoneNumber is stored under "resourceId' in the production version, and that resourceId is not present in the development version, and he wants to use DEV_PHONE_NUMBER in the development version. Which means the phone number is stored under a weird title, and it means that things will go badly wrong if a phone number is missing in the production version.
    – gnasher729
    Commented Nov 12, 2016 at 23:41
16

Even ignoring the fact that neither implementation is all that good, I would note that this is essentially a question of taste at least at the level of abstracting out single use trivial functions.

Number of lines is not a useful metric in most cases.

300 (or even 3000) lines of utterly trivial purely sequential code (Setup, or something like that) is seldom an issue (But might be better auto generated or as a data table or something), 100 lines of nested loops with lots of complicated exit conditions and maths as you might find in Gaussian Elimination or matrix inversion or such may be way too much to follow easily.

For me, I would not write a single use function unless the amount of code required to declare the thing was much smaller then the amount of code forming the implementation (Unless I had reason like say wanting to be able to easily do fault injection). A single conditional seldom fits this bill.

Now I come from a small core embedded world, where we also have to consider things like stack depth and call/return overheads (Which again argues against the sorts of tiny functions that seem to be advocated here), and this might be biasing my design decisions, but if I saw that original function in a code review it would get a old style usenet flame in response.

Taste is design is difficult to teach and only really comes with experience, I am not sure it can be reduced to rules about function lengths, and even cyclomatic complexity has its limits as a metric (Sometimes things are just complicated however you tackle them).
This is not to say that clean code does not discuss some good stuff, it does, and these things should be thought about but local custom and what the existing code base does should be given weight as well.

This particular example seems to me to be picking at trivial detail, I would be more concerned by much higher level stuff as that matters far more to the ability to understand and debug a system easily.

1
  • 1
    I strongly agree - it would take a very complex one-liner for me to consider wrapping it in a function... I certainly wouldn't wrap a ternary/default value line. I have wrapped one liners, but usually that's which shell scripts where it's ten pipes to parse something and the code is incomprehensible without running it. Commented Nov 12, 2016 at 0:11
15

Let me be blunt: it seems to me that your environment (language/framework/class design etc.) is not really suited for "clean" code. You are mixing all possible kinds of things up in a few lines of code that should not really be close together. What business does a single function have with knowing that resourceId==undef means that you are not in production, that you are using a default phone number in non-production systems, that the resourceId is saved in some "headers" and so on? I assume headers are HTTP headers, so you even leave the decision about which environment you are in to the end user?

Factoring single pieces of that out into functions will not help you much with that underlying problem.

Some keywords to look for:

  • decoupling
  • cohesion
  • dependency injection

You can achieve what you want (in other contexts) with zero lines of code, by shifting the responsibilities of the code around, and using modern frameworks (which may or may not exist for your environment/programming language).

From your description ("300 lines of code in a 'main' function"), even the word "function" (instead of method) makes me assume that there is no point in what you are trying to achieve. In that old-school programming environment (i.e., basic imperative programming with little structure, certainly no meaningful classes, no class framework pattern like MVC or somesuch), there is really not much point in doing anything. You will never get out of the sump without fundamental changes. At least your boss seems to allow you to create functions to avoid code duplication, that is a good first step!

I know both the type of code as well as the type of programmer you are describing quite well. Frankly, if it were a co-worker, my advice would be different. But as it is your boss, it is useless for you to go fighting about this. Not only that your boss can overrule you, but your code additions will indeed lead to worse code if only you do "your thing" partially, and your boss (and probably other people) keep on like before. It may just be better for the end result if you adapt to their style of programming (only while working on this particular codebase, of course), and try, in this context, to make the best out of it.

2
  • 1
    I agree 100% that there are implicit components here that should be separated, but without knowing more about the language/framework, it's hard to know whether an OO approach makes sense. Decoupling and the Single Responsibility Principle are important in any language, from pure functional (e.g. Haskell) to pure imperative (e.g. C.) My first step—if the boss would allow it—would be to convert the main function into a dispatcher function (like an outline or Table of Contents) which would read in a declarative style (describing policies, not algorithms) and farm the work to other functions. Commented Nov 11, 2016 at 18:44
  • JavaScript is prototypal, with first-class functions. It's inherently OO, but not in the classic sense, so your assumptions might not be correct. Cue hours of watching Crockford vids on YouTube ... Commented Nov 15, 2016 at 19:59
15

Don't put everything in one big loop, but don't do this too often either:

function isApplicationInProduction(headers) {
   return _.has(headers, 'resourceId');
}

The problem with the big loop is that its really hard to see its overall structure when it spans many screens. So try to take out large chunks, ideally chunks that have a single responsibility and are re-usable.

The problem with the tiny function above, is that while atomicity and modularity are generally good, that can be taken too far. If you're not going to reuse the above function it detracts from code readability and maintainability. To drill down into the detail you have to jump to the function instead of being able to read the detail inline, and the function call takes up hardly any less space than the detail itself would.

Clearly there is a balance to be found between methods that do too much and methods that do too little. I would never break out a tiny function as above unless it was going to be called from more than one place, and even then I would think twice about it, because the function just isn't that substantial in terms of introducing new logic and as such barely warrants having it's own existence.

1
  • 2
    I understand that a single boolean one liner is easy to read but that alone really only explains "What" is happening. I still write functions that wrap simple ternary expressions because the name of the function helps explain the reason "Why" I'm performing this condition check. This is especially helpful when someone new (or yourself in 6 months) needs to understand the business logic.
    – AJ X.
    Commented Nov 21, 2016 at 13:48
14

"Clean" is one goal in writing code. It's not the only goal. Another worthy goal is colocality. Put informally, colocality means that people trying to understand your code don't need to jump all over the place to see what you're doing. Using a well-named function instead of a ternary expression may seem like a good thing, but depending on how many such functions you have and where they're located, this practice can turn into a nuisance. I can't tell you whether you've crossed that line, except to say that if people are complaining, you ought to listen, particularly if those people have a say about your employment status.

2
  • 2
    "...except to say that if people are complaining, you ought to listen, particularly if those people have a say about your employment status". IMO this is really bad advice. Unless you are a seriously poor developer who needs to appreciate any job you can get, then always apply the "if you can't change your job, change your job" principle. Never feel beholden to a company; they need you more than you need them, so walk away to a better place if they don't offer what you want.
    – David Arno
    Commented Nov 11, 2016 at 19:07
  • 4
    I've moved around a bit during my career. I don't think I've ever had a job where I saw 100% eye-to-eye with my boss about how to code. We're human beings with our own backgrounds and philosophies. So I personally wouldn't leave a job just because there were a few coding standards I didn't like. (The finger-bending naming conventions managers seem to like are particularly contrary to the way I'd code if left to my own devices.) But you're right, a decent programmer shouldn't need to worry too much about simply staying employed. Commented Nov 11, 2016 at 20:59
6

Using small functions is, in general, a good practise. But ideally, I believe that the introduction of a function should either separate large logical chunks or reduce the overall size of the code by DRYing it out. The example you gave both makes the code longer and requires more time for a developer to read, while the short alternative doesn't explain that the "resourceId" value is only present in production. Something simple like that is both easy to forget and confusing when trying to work with it, particularly if you're still new to the codebase.

I won't say that you should absolutely use a ternary, some people I've worked with prefer the slightly longer if () {...} else {...}, it's mostly a personal choice. I tend to prefer a "one line does one thing approach", but I'd basically stick to whatever the codebase normally uses.

When using ternary, if the logical check makes the line too long or complicated, then consider creating a well named variable/s to hold the value.

// NOTE "resourceId" not present in dev build, use test data
let isProduction = 'resourceId' in headers;
let phoneNumber = isProduction ? headers.resourceId : DEV_PHONE_NUMBER;

I would also like to say that if the codebase is stretching towards 300 line functions, then it does need some subdivision. But I'd advise the use of slightly broader strokes.

5

The code example you gave, your boss IS CORRECT. A single clear line is better in that case.

In general, breaking complex logic into smaller pieces is better for legibility, code maintenance and the possibility that subclasses would have different behavior (even if only slightly).

Don't ignore the disadvantages: function overhead, obscuration (function does not do what comments and function name imply), complex spaghetti logic, the potential for dead functions (at one time were created for a purpose that no longer is called).

2
  • 1
    "function overhead": that's up to the compiler. "obscuration": OP has not indicated whether it's the only or the best way to check that property; you can't know for sure either. "complex spaghetti logic": where? "the potential for dead functions": that kind of dead code analysis is low-hanging fruit, and development toolchains that lack it are immature.
    – Rhymoid
    Commented Nov 12, 2016 at 12:26
  • The answers were more focused on the advantages, I only wanted to point out disadvantages too. Calling a function like sum(a, b) is always going to be more expensive than "a + b" (unless the function is inlined by the compiler). The rest of the disadvantages demonstrate that over-complexity can lead to its own set of problems. Bad code is bad code, and just because its broken into smaller bytes (or kept in a 300 line loop) doesn't mean its easier to swallow.
    – Phil M
    Commented Nov 14, 2016 at 21:38
2

I can think of at least two arguments in favor of long functions:

  • It means you have a lot of the context around each line. A way of formalizing this: draw the control flow graph of your code. At a vertex (~= line) between function entry and function exit, you know all of the incoming edges. The longer the function, there more such vertices there are.

  • Many small functions means there's a larger and more complex call graph. Pick a random line in a random function, and answer the question "in which context(s) is this line executed?" This becomes harder the bigger and more complex the call graph is, because you have to look at more vertices in that graph.

There are also arguments against long functions—unit-testability springs to mind. Use t̶h̶e̶ ̶f̶o̶r̶c̶e̶ your experience when choosing between one and the other.

Note: I'm not saying your boss is right, only that his perspective may not be completely devoid of value.


I think my view is that the good optimization parameter is not function length. I think a desiderata more useful to think in terms of is the following: all else being equal, it is preferable to be able to read out of the code a high-level description of both the business logic and the implementation. (The low-level implementation details can always be read if you can find the relevant bit of code.)


Commenting on David Arno's answer:

Writing small functions is a pain because it forces you to move into each small functions to see what the code is doing.

If the function is well-named, this isn't the case. isApplicationInProduction is self-evident and it should not be necessary to examine the code to see what it does. In fact the opposite is true: examining the code reveals less as to the intention than the function name does (which is why your boss has to resort to comments).

The name makes evident what the return value means, but it says nothing about the effects of executing the code (= what the code does). Names (only) convey information about intent, code conveys information about behavior (from which parts of the intent can sometimes be inferred).

Sometimes you want one, sometimes the other, so this observation doesn't create a one-sided universally valid decision rule.

Put everything in a main big loop even if the main loop is more than 300 lines, it is faster to read

It may be faster to scan through, but to truly "read" the code, you need to be able to effectively execute it in your head. That's easy with small functions and is really, really hard with methods that are 100's of lines long.

I agree that you have to execute it in your head. If you have 500 lines of functionality in one big function vs. in many small functions, it's not clear to me why this gets easier.

Suppose the extreme case of 500 lines of straight-line highly side-effecting code, and you want to know if effect A happens before or after effect B. In the big function case, use Page Up/Down to locate two lines and then compare line numbers. In the many-small-functions case, you have to remember where in the call tree the effects happen, and if you forgot you have to spend a non-trivial amount of time rediscovering the structure of this tree.

When traversing the call tree of supporting functions, you're also faced with the challenge of determining when you've gone from business logic to implementation details. I claim without evidence* that the simpler the call graph, the easier it is to make this distinction.

(*) At least I'm honest about it ;-)

Once again, I think both approaches have strengths and weaknesses.

Write only small functions if you have to duplicate code

I disagree. As your code example shows, small, well-named functions improve readability of code and should be used whenever [e.g.] you aren't interested in the "how", only the "what" of a piece of functionality.

Whether you are interested in the "how" or "what" is a function of the purpose for which you are reading the code (e.g. getting a general idea vs. tracking down a bug). The purpose for which you are reading the code is not available while writing the program, and you will most likely read the code for different purposes; different decisions will optimize for different purposes.

That said, this is the part of the boss's view I probably disagree with the most.

Don't write a function with the name of the comment, put your complex line of code (3-4 lines) with a comment above. Like this you can modify the failing code directly

I really can't understand the reasoning behind this one, assuming it really is serious. [...] Comments have a fundamental flaw: they aren't compiled/interpreted and so can't be unit tested. The code gets modified and the comment gets left alone and you end up not knowing which is right.

Compilers only compare names for equality, they never give you a MisleadingNameError. Also, because several call sites may invoke a given function by name, it is sometimes more arduous and error-prone to change a name. Comments don't have this problem. However, this is somewhat speculative; to really settle this, one would probably need data about whether programmers are more likely to update misleading comments vs. misleading names, and I don't have that.

1

Your boss apparently has never heard of Unit Tests. You can write Unit Tests for small functions and thus test the correctness of every function separately.You cannot do that with a big loop that does lots of things, as all you will end up is the information, that the end result was wrong but not where in the loop the mistake is. If you test all functions separately, you will find this one function that does it wrong.

If the functions are really small, you can even theoretically proof their correctness but that will become close to impossible with big loops as the complexity soon makes this approach impractical.

Writing small functions is a pain because it forces you to move into each small function to see what the code is doing.

If the name is chosen correctly, the name alone should tell him what the function does. countNumberOfCustomers() will certainly get the current market price for bananas, taking a wild guess, I'd say "it counts the number of customers".

For even more details, add documentary comments. Every decent modern IDE will display the documentary comments somewhere (pop-up, certain editor area) when you highlight the function call without having to open up the function. This way you also get explanations what the parameters mean.

Put everything in a main big loop even if the main loop is more than 300 lines, it is faster to read.

CustomerList clist = fetchCustomerList(MainServer);

Is one line of code, it is easy to read and it is easy to see what it does. fetchCustomerList() however could be 20 lines of code. So pulling this code there means I replace one line with 20 lines. The same thing would happen to many other functions. How is that faster to read? How are 20 lines faster to read than a single one?

Only write small functions if you have to duplicate code.

This is an instruction, not an argument, no reasoning is given here.

Don't write a function with the name of the comment, put your complex line of code (3-4 lines) with a comment above; similarly you can modify the failing code directly

Code should always speak for itself. Comments should only explain what code cannot explain. If your boss uses useless names for functions, no wonder he never knows what functions do and always has to look up their code. So he's trying to solve a problem by forcing a code style on you that only exists because of that code style.

Since you meI would recommend that you let your boss watch this (I bookmarked where the important part starts):

https://youtu.be/7EmboKQH8lM?t=2435

Keep watching till you reach 55:25. What your boss is requesting of you is "rude". Writing code as you suggest is "polite" as it allows the reader to exit early. And this saves the reader a lot of time and makes it much easier to understand your code. Maybe he will understand it once he has seen that talk or at least the 15 minutes I picked out.

4
  • Testing too fine-grained is wasted effort, and doesn't test what matters, making any change, however beneficial, much more difficult. Or are you saying all those micro-functions should be essential parts of the interface anyway, instead of implementation details? Commented May 2, 2020 at 11:13
  • @Deduplicator Creating an own function for every line of code isn't meaningful either, never said the opposite. Swapping a line of code with a function call won't make things much easier to read in general. Generally a function call should hide multiple lines of code that perform a meaningful operation which can be seen as "an atomic" operation. I was only opposing that putting everything into one big loop is more readable and it's hardly testable. And in some cases putting just a single line of code into an own function is justifiable. Still looking for a good sample to improve my answer.
    – Mecki
    Commented May 2, 2020 at 11:41
  • @Deduplicator I would also suggest you to watch the video I've now linked to my reply. Note that this is part 1 and the whole talk has 5 parts. I think in part 3 he talks about testing. And as long as a function can be meaningfully split into two functions, then a function is not doing one thing, yet a function should always only do one thing and its test should always only test one thing. This whole talk is a must see for every decent developer out there, trust me, it will change your point of view on everything you've ever done in that field.
    – Mecki
    Commented Jul 8, 2020 at 13:33
  • Seems I was a bit over-critical. Yes, functions should only be big enough to do their one thing, and no bigger. Commented Jul 8, 2020 at 14:07
0

Ironically, not only are both approaches problematic, but they share a reason for being so. They both could do with the advice: write code more declaratively and less imperatively.

For example, ternary expressions are declarative, because they just say phoneNumber = a ternary expression, whereas an if/else that assigns to phoneNumber in each branch is imperative about how we branch. Yes, the two approaches are logically equivalent, but the advantage of declarative code is chunking, which humans reading, writing and editing code need dearly. Chunking is when people define a concept in terms of others so they can reduce the number of concepts they must simultaneously consider, which improves what we can do with 7 plus-or-minus 2 concepts.

But ternary expressions aren't the only way to wrap up imperative logic declaratively. One of the reasons functions are so useful in programming is that invoking functions allows an arbitrary amount of imperative logic to be summarized with a single declarative line. In fact, ternary expressions are invoking a "function", just a function split across two operators. In an alternate history or another language, the syntax would be the function-like ifthenelse(headers.resourceId, headers.resourceId, DEV_PHONE_NUMBER). Why did language designers ever give us either that or an operator-based alternative? Because it's a useful bit of declaration. Call it syntactic sugar if you like, but its motives are deeper than that.

But the advantage to making the boolean you check a function call is that you can declare isApplicationInProduction(headers) rather than imperatively writing either headers.resourceId, or _.has(headers, 'resourceId'), or whatever implementation detail gets the answer. You don't care how it's done; you only care that, if the application is in production, you use the resource ID, otherwise you use the developer's phone number. This suggests a compromise between your and your boss's code,

function isApplicationInProduction(headers) {
   return headers.resourceId;
}

phoneNumber = isApplicationInProduction(headers) ? headers.resourceId : DEV_PHONE_NUMBER;

Depending on the language, we can do it all nicely in one line with no function at all, using a null-coalescing operator:

phoneNumber = headers.resourceId ?? DEV_PHONE_NUMBER;

That's a nice-looking operator. Why was it invented? Well, it's another example of chunking: a ?? b declares an outcome achieved imperatively by checking whether a is null, returning a if it isn't, but otherwise returning b. Notice in these examples why I said to write more declaratively and less imperatively, rather than declaratively and not imperatively: the distinction is not only a spectrum, but a nested one.

A 300-line function is a really bad idea from this perspective. What does such a function do? Well, this and that and that and that and that. OK, name those activities. No doubt in such functions you've seen "paragraphs", headed by comments and separated by blank lines. Those paragraphs deserve to be functions; and if you make them functions, each replacement of one paragraph with one function call replaces imperative guts with declarative description.

This idea that declaring something that does imperative work behind the scenes goes much, much further than examples like this. It's what happen every time you import from a library, let a language hand work to another, etc. It's also the reason for various OOP principles like encapsulation and the law of Demeter. We ask that code calling instances of classes only have access to what the instance class guarantees in its contract, not because we're worried that a.b.c being legal is less secure, or anything like that, but because classes readily change how they get their job done. Going back to the code I suggested without a null-coalescing operator, it allows how we check whether the application's in production to be uncoupled from the fact that we're checking. Sure, all one approach does compared with another is rearrange the code, putting the logic on one line instead of another. But that's refactoring for you.

You'd be amazed how much of clean code boils down to making code more declarative - in practice, recursively so. Once you have an eye for it, you'll see it everywhere. True, you'll see imperative code wherever there's a for loop; but you'll also see declarative code in alternatives to it. Quite apart from the examples above (functions, and ternary and null-coalescing operators), if you've ever used SIMD calculations or a map or a filter or a Python comprehension or LINQ in C#, you've seen declarative code in action.

Not the answer you're looking for? Browse other questions tagged or ask your own question.