How can wrapping an expression as a function be Clean Code?

Question

Another programmer just started working in our team and submitted a patch. What required was to have something that compares and check a couple of conditions and set a property based on the outcome. The patch, in essence, is a class that implements the requirement as something like this:

class Processor {
    A a;
    B b;

    public function process(A a, B b) {
        this.a = a;
        this.b = b;

        if (false == this.isVerified() && this.equalNames()) {
            this.setVerified();
        }
    }

    private function equalNames(): bool {
        return this.a.name() == this.b.name();
    }

    private function isVerified(): bool {
        return this.a.isVerified();
    }

    private function setVerified() {
        this.a.setVerified(true);
    }
}

The actual code probably had slightly more detail but the pseudo code is detail enough, I think. Basically, what happen when we reviewed the code we were dumbstruck. To make matter worse, he couldn't give the explanation beyond that his code is "Clean Code" and telling us to "Look how clean it is!" and read the book. Actually, someone pointed out that to utilize his code, one would need to instantiate that class first, then call process() to which his reply was "What's wrong with that?"

Since we needed to get the job done, one of us proposed to replace that class with a method within AService class:

public function verifyA(A a, B b) {
    if (!a.isVerified() && a.name() == b.name()) {
        a.setVerified(true);
    }
}

That is what finally got merged. Now, I feel that I want to know if there any truth behind wrapping a single logical expression in a function because I don't want him to feel being ignored (in his code review he always put comment on everyone's if statements to put them in function, and so far no one oblige). Also to make him or we understand how to explain whatever the right way is.

I haven't read the Clean Code book. I do read Uncle Bob's blog and understand putting business logic in functions and. My question is more specific to putting a single logical expression in function and reference to the book if any. The secondary question is how to handle debating this issue as it has come up a few times and affecting the team's dynamics.

Answer 1

What constitutes "clean code" always depends on context. The exact same lines of code could be either perfectly clean or hideously over-complicated depending on the requirements of the application.

The design in question allows you to use dependency injection to substitute implementations of the process without affecting clients. So this is clean and SOLID code if you have this requirement. If you don't have such a requirement, then it is over-complicated and violating the YAGNI and KISS principles.

Since neither you or the developer could explain the reasoning for the design I think the second option is the most likely. Probably the developer have seen this design in a context where it makes sense, but applied it in a context where it didn't make sense.

Answer 2

It's all about naming.

If you find a function name that makes it clearer what the result of the comparison means, then wrap it into a function.

In your example this is not the case. For example, isVerified() actually obsures what exactly is being verified, in this case

return this.a.isVerified();

but it could also be

return this.b.isVerified();

or

return this.a.isVerified() && this.b.isVerified();

Answer 3

You should also, and perhaps firstly, look at the abstraction provided to client programmer's who use the classes, then secondly as you are doing, look at the implementation for clean-ness, etc..

If the client's usage looks like this:

new Process().process(a,b);

The Process instance not even captured for future reference. I'd say this class is overkill and does not provide a useful abstraction (over, say, a.verify(b);)

Another potential usage is to capture and hold a Process instance, and use it's process method on different a's and b's:

Process p = new Process();
...
p.process(a,b);
...
p.process(aa,bb);
...

I can't say that this feels like a great abstraction; but I've seen worse. (It wouldn't be half bad if for some reason the Process instance were to take some configuration.)

However, regarding the implementation, the fields A a; and B b; should be removed in favor of passing parameters to the private methods to make the class appropriate for this usage (and thread safe as @MrCochese rightly points out).

Further, in both the approaches you show, an a is verified against a b, and then the a is generally marked as verified even though it was only verified against a very specific b.

Here's an approach I would consider, which I show not from implementation but from client programmer's usage, as I consider the abstraction being provided as the foremost consideration over the internal implementation details:

Pair p = new Pair(a,b);
...
p.verify();
...

Now we have a decent abstraction that merits a class. It is clear that the pair can be verified, and that once verified we know what that means: that a was verified against that b. I would also capture the verification state in the Pair class rather than in the A class.

Answer 4

I'm guessing you changed the name of the class to Processor for the sake of the example. But I think the name is actually key here.

If the developer could come up with a name for the class that maps to a defined business concept, his approach may make sense, although I have misgivings that the class is stateful (especially if the name ends with er). If it maps to a defined business concept, the class can serve as an extension point and may improve maintainability.

If on the other hand it may have a silly name: a name that is concatenation of operations (e.g. CheckVerifiedAndUpdateProcessor), or a name that is an arbtrary neologism (e.g. CleanNameProcessor). If the former, I would ask if the developer plans to add a new class anywhere a compound expression is used. If the latter, I would ask if he expects everyone on the team maintaining the code to learn his private little terminology. Both of those are terrible ideas.

Answer 5

Is your question really about a one liner or the way your programmer works? You've set a tag "Teamwork" so I think this may be more what you're looking for, drawing from experience.

The context that you presented is too small that we can make something of it. He may be right if the entire code is moving toward (or is) a big ball of mud. In that case, I'd support him because he's trying to help and get the application on the right track. In fact, we could say he's really putting himself into the project and wants it to succeed! And he's constantly pointing out flaws in other's code. He really wants to get things going on the right track.

You've given us a bit and asking us if this bit is right or wrong. Without knowing anything about the entire context, architecture, business process, infrastructure, limitations etc., we can debate endlessly and it's mainly going to be about someone's preferences.

On the other hand if you have a strong and good architecture that works and is proven throughout many years and extensions and further business requirements that are put on software and infrastructure and is cost effective, then he's wrong because he's not working as the team does irrelevant if his code is clean or not, including the one liner. But this is also subjective, because most people perceive their code as the best. He maybe sees something that you don't see. Maybe you should let him speak on the subject to the whole team as a debate?

Answer 6

Some times a function name provides better abstraction than what a simple expression does. I often find myself using:

double square(double in)
{
   return in*in;
}

Use of

double length_squared = square(a) + square(b) + square(c);

reads better than use of

double length_squared = a*a + b*b + c*c;

Use of square makes more sense if a, b, and c are obtained from other function calls.

double length_squared = square(aFun(x)) + square(bFun(x)) + square(cFun(x));

is definitely better than:

double a = aFun(x);
double b = bFun(x);
double c = cFun(x);
double length_squared = a*a + b*b + c*c;

You definitely don't want to use:

double length_squared = aFun(x)*aFun(x) + bFun(x)*bFun(x) + cFun(x)*cFun(x);

if aFun, etc., have any significant overhead.

Answer 7

There is a school of thought that thinks like this. There are many other schools that think it's excessive. Personally I think for simple comparisons like this it's utter BS and actually harmful (especially it's a serious performance hit if the comparison gets done frequently).

The "logic" goes something like "if it may be called more than once, make it a function". And a logical comparison of course gets called more than once in a piece of code.

Worst I've seen of that was a piece of C code where the initial programmer had made a function

int add(int a, int b) {
  return a+b;
}

and that function was called millions of times in the execution of the program, causing it to slow to a crawl because of the massive amount of function stack creation and unwrapping that happened as a result.

The program, which was pretty simple, as a result took 36 hours to complete its work, when required to run once every 24 hours. By removing those silly functions and some loop unrolling, runtime was brought back to 16 hours at which point the customer was happy. We could have brought it back even more but that was considered unneeded expense.