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?

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();
}

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

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

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.

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.

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