4

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.

  • 1
    Possible duplicate of One-line functions that are called only once – gnat Apr 19 '17 at 13:58
  • see also: Style for control flow with validation checks – gnat Apr 19 '17 at 13:58
  • 6
    The code, as given, has completely sacrificed thread safety by adding statefulness where it is not clearly not needed. There are reasons to encapsulate an action as an object, such as the command pattern, but it should be explicit in the design that the object is a one-shot usage - for instance, setting the A and B parameters in the constructor would achieve that. – Mr Cochese Apr 19 '17 at 14:40
  • Another great example of cargo cult design. What I don't understand is why he did not create several more classes; one can always add more classes to make it more "clean". – Frank Hileman Apr 23 '17 at 22:52
  • @FrankHileman the actual code will probably detract the point of my question. It came with a SomethingProcess interface with only process() method define. It's only used in the class, and everywhere the class is used he put three lines: 1) comment for what it does 2) new Processor() and 3) calling process() on the object. These three lines were repeated at least three times and he said nothing was wrong with that and we could replace it when we start using DI * facepalm * – imel96 Apr 25 '17 at 9:11
12

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.

  • 11
    "Probably the developer have seen this design in a context where it makes sense"...or is just unthinkingly applying a rule that he learned in a book. There is a lot of that going on. – user82096 Apr 19 '17 at 15:01
  • My issue is how to answer the "clean code" argument. So, if it's over-complicated and YAGNI and not KISS then it's not "clean code"? Or should "clean code" comes after the other principles? – imel96 Apr 20 '17 at 16:11
  • @imel96: "Clean code" just means good, maintainable code. So if code is overly complicated then it is not clean. – JacquesB Apr 21 '17 at 15:37
  • @JacquesB thanks for clarifying that. Also just read someone said "Uncle Bob wrote thousands of pages on clean code. Kent Beck wrote for lines (rules)." Glad it's just common sense not some intricate conventions. – imel96 Apr 25 '17 at 9:16
  • Dependency Injection again rears its ugly head. – Frank Hileman Apr 25 '17 at 13:35
5

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();
  • 1
    I see, you're saying that it'd make more sense if isVerified() called isAVerified() it it only test a's status – imel96 Apr 19 '17 at 20:27
  • 1
    @imel96: Yes, at least it is not obscuring, but still no improvement over the original this.a.isVerified(). Therefore I would not define a function in this case. – Frank Puffer Apr 20 '17 at 6:55
4

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.

2

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.

2

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?

  • Yes, I put the Teamwork tag because it may be that he's on the right track and the team needs to adjust. So, first I need to know if it's really Clean Code and whatever the answer is put it up in retrospective without being an endless debate. The current code base works fine and the focus is on new code. The thing is even in a 10 lines function, we get comment saying that simple if statement (1 or 2 expressions) should be put in it's own function, potentially turning it into 3 function, and that put people off. – imel96 Apr 20 '17 at 15:42
2

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.

  • I get the idea and you pick a good example how it may blow up in code review. Let say Bob wrote that in * in and Alice suggested a square() function, Bob would say, I won't need square() in the future, if someone needs it they can use the standard pow() function and if square() is a good idea, it'd be exist in standard library. Then Alice would say it'd be cleaner as square() and refers to Clean Code. I would personally use something that's close to standard math notation. What's the Clean Code way of doing it and should it override all other arguments? – imel96 Apr 20 '17 at 16:04
  • 1
    @imel96, I think people don't pay enough attention to abstractions when they write functions. They pay more attention to abstractions when creating classes. IMO, the abstraction of a square is more important than the fact that you can compute it by multiplying a number with itself. The argument that if square() is a good idea, then it would exist in the standard library is a poor one. Not all useful functions are in the standard libaray. As usual, YMMV. – R Sahu Apr 20 '17 at 16:11
  • It gets more interesting (if you don't think operator as abstraction) when it's implemented in Lisp, e.g. (* in in). It probably would confuse people who used to the idioms then suddenly see (square in). – imel96 Apr 20 '17 at 16:22
  • Also wonder why do you think standard library argument is poor, in the case of simple functions as square(), not complex ones which would be in specialise library? – imel96 Apr 20 '17 at 16:31
  • @imel96, I do see operators as abstractions but I know that not everybody does. Regarding why I think "not in the standard library" is a poor one, standard libraries are constantly evolving, at least in C/C++ land. Maybe square will be added to it at some point in the future. – R Sahu Apr 20 '17 at 16:48
1

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.

  • 7
    Oh, but you're going to regret all of that hard-coding if the fundamental laws of mathematics ever change! – user82096 Apr 19 '17 at 15:03
  • 1
    I agree that this add function is useless (unless a pointer to it for dynamically changing behavior is necessary) but nowadays it would have zero performance impact with any decent compiler (and linker) – Daniel Jour Apr 19 '17 at 18:24
  • Just curious if you only did some loop unrolling if that would have been enough to gain performance without getting rid of the function. – JeffO Apr 19 '17 at 19:00
  • 1
    Agree. What I really want to know is how did you and the initial programmer come to a resolution? Why was the code accepted at the first place? Did others think that add(a, b) was more readable than a + b? – imel96 Apr 19 '17 at 20:31
  • 1
    @imel96 external contractor, no code reviews, guy was a theorist and university teacher in software engineering. His work had been accepted as excellent based on his credentials alone. This wasn't the only piece of theoretically beautiful software that performed horribly that we had to take under hand to improve performance to where it worked as required. – jwenting Apr 20 '17 at 6:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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