Additional line in block vs additional parameter in Clean Code

Question

Context

In Clean Code, page 35, it says

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.

I completely concur, that makes a lot of sense.

Later on, on page 40, it says about function arguments

The ideal number of arguments for a function is zero (niladic). Next comes one (monadic), followed closely by two (dyadic). Three arguments (triadic) should be avoided where possible. More than three (polyadic) requires very special justification—and then shouldn’t be used anyway. Arguments are hard. They take a lot of conceptual power.

I completely concur, that makes a lot of sense.

Issue

However, rather often I find myself creating a list from another list and I will have to live with one of two evils.

Either I use two lines in the block, one for creating the thing, one for adding it to the result:

 public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
 {
 List<Flurp> flurps = new List<Flurp>();
 foreach (BadaBoom badaBoom in badaBooms)
 {
 Flurp flurp = CreateFlurp(badaBoom);
 flurps.Add(flurp);
 }
 return flurps;
 }

Or I add an argument to the function for the list where the thing will be added to, making it "one argument worse".

 public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
 {
 List<Flurp> flurps = new List<Flurp>();
 foreach (BadaBoom badaBoom in badaBooms)
 {
 CreateFlurpInList(badaBoom, flurps);
 }
 return flurps;
 }

Question

Are there (dis-)advantages I am not seeing, which make one of them preferable in general? Or are there such advantages in certain situations; in that case, what should I look for when making a decision?

• cmaster - reinstate monica: What's wrong with flurps.Add(CreateFlurp(badaBoom));?
• cmaster - reinstate monica: Nope, it's just a single statement. It's just a trivially nested expression (a single nested level). And if a simple f(g(x)) is against your style-guide, well, I can't fix your style-guide. I mean, you don't split sqrt(x*x + y*y) into four lines either, do you? And that's three(!) nested subexpressions on two(!) inner nesting levels (gasp!). Your goal should be readability, not single operator statements. If you want the later, well, I have the perfect language for you: Assembler.
• Luaan: @cmaster Even x86 assembly doesn't strictly have single-operator statements. The memory addressing modes include many complicated operations and can be used for arithmetic - in fact, you can make a Turing-complete computer using only x86 mov instructions and a single jmp toStart at the end. Someone actually made a compiler that does exactly that :D
• cmaster - reinstate monica: @Luaan Not to speak of the infamous rlwimi instruction on the PPC. (That stands for Rotate Left Word Immediate Mask Insert.) This command took no less than five operands (two registers, and three immediate values), and it performed the following operations: One register contents was rotated by an immediate shift, a mask was created with a single run of 1 bits which was controlled by the two other immediate operands, and the bits that corresponded to 1 bits in that mask in the other register operand were replaced with the corresponding bits of the rotated register. Very cool instruction :-)
• psmears: @R.Schmitz "Im doing general purpose programming"--actually no, you're not, you're doing programming for a specific purpose (I don't know what purpose, but I'm assuming you do ;-). There are literally thousands of purposes for programming, and the optimal coding styles for them vary--so what is appropriate for you may not be suitable for others, and vice versa: Often advice here is absolute ("always do X; Y is bad" etc) ignoring that in some domains it's utterly impractical to stick to. That's why advice in books like Clean Code should always be taken with a pinch of (practical) salt:)

Answer 1

These guidelines are a compass, not a map. They point you in a sensible direction. But they can't really tell you in absolute terms which solution is "best". At some point, you need to stop walking into the direction your compass is pointing, because you have arrived at your destination.

Clean Code encourages you to divide your code into very small, obvious blocks. That is a generally good direction. But when taken to the extreme (as a literal interpretation of the quoted advice suggests), then you will have subdivided your code into uselessly small pieces. Nothing really does anything, everything just delegates. This is essentially another kind of code obfuscation.

It is your job to balance "smaller is better" against "too small is useless". Ask yourself which solution is simpler. For me, that is clearly the first solution as it obviously assembles a list. This is a well-understood idiom. It is possible to understand that code without having to look at yet another function.

If it's possible to do better, it's by noting that "transform all elements from a list to another list" is a common pattern that can often be abstracted away, by using a functional map() operation. In C#, I think it's called Select. Something like this:

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
{
 return badaBooms.Select(BadaBoom => CreateFlurp(badaBoom)).ToList();
}

• Ben Voigt: The code is still wrong, and it pointlessly reinvents the wheel. Why call CreateFlurps(someList) when the BCL already provides someList.ConvertAll(CreateFlurp) ?
• amon: @BenVoigt This is a design-level question. I'm unconcerned with exact syntax, especially since a whiteboard doesn't have a compiler (and I have last written C# in '09). My point is not "I've shown the best possible code" but "as an aside, this is a common pattern that's already solved". Linq is one way to do it, the ConvertAll you mention another. Thank you for suggesting that alternative.
• jpmc26: Your answer is sensible, but the fact that LINQ abstracts the logic away and reduces the statement to one line after all seems to contradict your advice. As a side note, BadaBoom => CreateFlurp(badaBoom) is redundant; you can pass CreateFlurp as the function directly (Select(CreateFlurp)). (As far as I know, this has always been the case.)
• JounceCracklePop: Note that this removes the need for the method entirely. The name CreateFlurps is actually more misleading and harder to understand than just seeing badaBooms.Select(CreateFlurp). The latter is completely declarative -- there's no problem to decompose and therefore no need for a method at all.
• JounceCracklePop: @R.Schmitz It's not hard to understand, but it's less easy to understand than badaBooms.Select(CreateFlurp). You create a method so that its name (high level) stands in for its implementation (low level). In this case they're at the same level, so to find out exactly what's happening I have to just go look at the method (instead of seeing it inline). CreateFlurps(badaBooms) could hold surprises, but badaBooms.Select(CreateFlurp) cannot. It's also misleading because it's erroneously asking for a List instead of an IEnumerable.

Answer 2

The ideal number of arguments for a function is zero (niladic)

No! The ideal number of arguments for a function is one. If it's zero, then you are guaranteeing that the function has to access external information to be able to perform an action. "Uncle" Bob got this one very wrong.

Regarding your code, your first example only has two lines in the block because you are creating a local variable on the first line. Remove that assignment, and you are complying with these clean code guidelines:

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
{
 List<Flurp> flurps = new List<Flurp>();
 foreach (BadaBoom badaBoom in badaBooms)
 {
 flurps.Add(CreateFlurp(badaBoom));
 }
 return flurps;
}

But that's very long winded (C#) code. Just do it as:

IEnumerable<Flurp> CreateFlurps(IEnumerable<BadaBoom> badaBooms) =>
 from badaBoom in babaBooms select CreateFlurp(badaBoom);

• Ryathal: A function with zero arguments is meant to imply the object encapsulates the required data not that things are existing in a global state outside an object.
• David Arno: @Ryathal, two points: (1) if you are talking methods, then for most (all?) OO languages, that object is inferred (or explicitly stated, in the case of Python) as the first parameter. In Java, C# etc, all methods are functions with at least one parameter. The compiler just hides that detail from you. (2) I never mentioned "global". The object state is external to a method for example.
• Doc Brown: Im am pretty sure, when Uncle Bob wrote "zero" he meant "zero (not counting this)".
• David Arno: @DocBrown, probably as he's a big fan of mixing state and functionality in objects, so by "function" he likely refers specifically to methods. And I still disagree with him. It's far better to only give a method what it needs, rather than leave it rummage through the object to obtain what it wants (ie, it's classic "tell, don't ask" in action).
• David Arno: @AlessandroTeruzzi, The ideal is one parameter. Zero is too few. This is why, for example, functional languages adopt one as the number of parameters for currying purposes (in fact in some functional languages, all functions have exactly one parameter: no more; no less). Currying with zero parameters would be nonsensical. Stating that "the ideal is as few as possible, ergo zero is best" is an example of reductio ad absurdum.

Answer 3

The 'Clean Code' Advice is completely wrong.

Use two or more lines in your loop. Hiding the same two lines in a function makes sense when they are some random maths which needs a description but it does nothing when the lines are already descriptive. 'Create' and 'Add'

The second method you mention in doesn't really make any sense, as you are not forced to add a second argument in order to avoid the two lines.

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
 {
 List<Flurp> flurps = new List<Flurp>();
 foreach (BadaBoom badaBoom in badaBooms)
 {
 flurps.Add(badaBoom .CreateFlurp());
 //or
 badaBoom.AddToListAsFlurp(flurps);
 //or
 flurps.Add(new Flurp(badaBoom));
 //or
 //make flurps a member of the class
 //use linq.Select()
 //etc
 }
 return flurps;
 }

or

foreach(var flurp in ConvertToFlurps(badaBooms))...

As noted by others, the advice that the best function is one with no arguments is skewed to OOP at best and plain bad advice at worst

• R. Schmitz: Maybe you want to edit this answer to make it more clear? My question was if one thing outweighs another under Clean Code. You say that the whole thing is wrong and then go on to describe one of the options I gave. At the moment this answer looks like you are following an anti-Clean Code agenda instead of actually trying to answer the question.
• Ewan: sorry i interpreted your question as suggesting the first was the 'normal' way, but you were being pushed into the second. I'm not anti clean-code in general, but this quote is obviously wrong
• cmaster - reinstate monica: @R.Schmitz I have read "Clean Code" myself, and I follow most of what that book says. However, concerning the perfect function size being pretty much a single statement, it's just plain wrong. The only effect is, that it turns spaghetti code into rice code. The reader is lost in the multitude of trivial functions that only produce sensible meaning when seen together. Humans have a limited working memory capacity, and you can either overload that with statements, or with functions. You must strike a balance between the two if you want to be readable. Avoid the extremes!
• R. Schmitz: @cmaster The answer was only the first 2 paragraphs when i wrote that comment. It's a way better answer by now.
• Ewan: frankly I preferred my shorter answer. Theres too much diplomatic talk in most of these answers. The quoted advice is plain wrong, no need to speculate on 'what it really means' or twist around to try and find a good interpretation.

Answer 4

Second is definitely worse, as CreateFlurpInList accepts list and modifies that list, making the function not pure and harder to reason about. Nothing in the method name suggests the method only adds to the list.

And I offer third, best, option:

public List<Flurp> CreateFlurps(List<BadaBoom> badaBooms)
{
 return badaBooms.Select(CreateFlurp).ToList();
}

And hell, you can inline that method immediately if there is only one place where it is used, as the one-liner is clear by itself, so it doesn't need to be encapsulated by method to give it meaning.

• gnasher729: I wouldm't complain so much about that method "not being pure and harder to reason about" (although true), but about it being a completely unnecessary method for handling a special case. What if I want to create a standalone Flurp, a Flurp added to an array, to a dictionary, a Flurp that then gets looked up in a dictionary and the matching Flurp removed etc. ? With the same argument, the Flurp code would need all these methods as well.

Answer 5

The one argument version is better, but not primarily because of the number of arguments.

The most important reason it is better is that it has lower coupling, which makes it more useful, easier to reason about, easier to test, and less likely to turn into copy+pasted clones.

If you provide me with a CreateFlurp(BadaBoom), I can use that with any type of collection container: Simple Flurp[], List<Flurp>, LinkedList<Flurp>, Dictionary<Key, Flurp>, and so on. But with a CreateFlurpInList(BadaBoom, List<Flurp>), I'm coming back to you tomorrow asking for CreateFlurpInBindingList(BadaBoom, BindingList<Flurp>) so that my viewmodel can get the notification that the list changed. Yuck!

As an additional benefit, the simpler signature is more likely to fit with existing APIs. You say you have a recurring problem

rather often I find myself creating a list from another list

It's just a matter of using the tools available. The shortest, most efficient, and best version is:

var Flurps = badaBooms.ConvertAll(CreateFlurp);

Not only is this less code for you to write and test, it's also faster, because List<T>.ConvertAll() is smart enough to know that the result will have the same number of items as the input, and preallocate the result list to the correct size. While your code (both versions) required growing the list.

• JounceCracklePop: Don't use List.ConvertAll. The idiomatic way to map an enumerable of objects to different objects in C# is called Select. The only reason ConvertAll is even available here is because the OP is erroneously asking for a List in the method -- it should be an IEnumerable.

Answer 6

Keep the overall goal in mind: making the code easy to read and maintain.

Often, it will be possible to group multiple lines into a single, meaningful function. Do so in these cases. Occasionally, you'll need to reconsider your general approach.

For example, in your case, replacing the whole implementation with

var flups = badaBooms.Select(bb => new Flurp(bb));

might be a possibility. Or you might do something like

flups.Add(new Flurp(badaBoom))

Sometimes, the cleanest and most readable solution will simply not fit in one line. So you'll have two lines. Don't make the code harder to understand, just to fulfill some arbitrary rule.

Your second example is (in my opinion) considerably harder to understand than the first. It's not just that you have a second parameter, it's that the parameter is modified by the function. Look up what Clean Code has to say about that. (Don't have the book at hand right now, but I'm pretty sure it's basically "don't do that if you can avoid it").