When is code "too clever" / how do you think about readability/cognitive load?

Usually, it's quite clear to me whether code I'm writing is clean and easy to understand, but lately, I find myself doing the following a lot in JavaScript:

When a function needs to do something only when a certain option is passed in, I do this:

function foo(shouldBar){
    //...

    shouldBar && bar();
}

I find it quite clear, but I'm probably going to try to kick this habit because

if(shouldBar){
    bar();
}

• is more explicit,
• is easier to understand or devs with less JavaScript experience.

How do you decide whether a particular line of code is "too clever"?

Comments

[Donald Merand]

The language that you're using can play into this as well. In Ruby, I'd have no problem with:

def foo(should_bar)
  bar if should_bar
end

In Ruby, such shortenings are idiomatic (and easy to understand). In JavaScript I'd find the equivalent (your first example) to be too potentially confusing to leave in. I'd be thinking of future-me, who might be tired, or future-other-dev, who might not know the trick with sequential execution using &&. I'd want that person to be clear that I was making a conditional statement, without having to guess at my intent.

As for the general question of what makes a line of code "too clever", I like to stick to the old adage:

Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?
The Elements of Programming Style, 2nd edition, chapter 2

[Josh Cheek]

I prefer the short-circuiting approach. The second one is okay if you remove the block.

[Ben Sinclair]

I'm the opposite. I'll use the short-circuit in a shell script, where it's common practice, but in JS I'll write it all out the long way, and I'll always include the block.

[Josh Cheek]

My preference for the short-circuit is b/c I think it's important to understand short-circuiting (as opposed to some things, which could be called "trivia"), and the short-circuit has the best signal to noise ratio.

My preference for removing the block is mostly because of better signal to noise ratio (ie less spammy), but also because I think it facilitates syntactic misunderstanding to use the block.

[Adrian B.G.]

Sounds better indeed, is not about the person or its capabilities, it's actually about how fast you can load that piece of code in your head.

This problem is usually a sign that another problem on the list is present, usually what is hard to understand quickly is "smelly code", other devs had issue understanding it so the risk of having a bug is greater.

Every second that is added to this process is multiplied by the number of reads, for each team member.

[Weston Wedding]

While I get your point; Linus writes bad code like anyone else.

[Jochem Stoel]

I frequently choose to write code that is 'too clever' intentionally so that it is harder to understand.

[Michael Larson]

I totally identify with this. When I started I felt the same. Worse was the code examples for alogrothims at school. Just happy that I finally realize what the real issue was.

[Adrian B.G.]

Hopefully I don't develop in python, otherwise this would be considered "normal" :))

On a serious note, I usually follow this guidelines, I refactor when:

• the code would not be understood by a junior, this is a good threshold overall
• it cannot be tested
• it will be impossible to find the error source in production (one-liners are the best example, they throw an error in the middle of the line, but there is no way to find out which operation in the pipeline failed)
• "compiler wannabe optimization" some devs like to pre-optimize and do the same (or worst) than the compiler/runtime already does, sacrificing readability with no real gain
• when the IDE/linter throws a warning, usually suggesting something better, usually he found an anti-pattern.

[Kushan Joshi]

I feel a very important point in a project is figuring out the convention being followed.

Convention

If you find the code using

function foo(shouldBar){
    shouldBar && bar();
}

^{^} everywhere, then nobody cares whether it is too clever! you should stick to using the style for any commits you make. In my experience, most of these tricks are reminiscent of past, when a certain pattern was the new cool.

To give you an example of past, let's look at an example which I have seen frequently used in old projects but not used that frequently in new projects. The following code is taken from an old code base:

  var base = this.base(),
            i, j, k, id;

In my opinion, folks of today would rather use a new line for each definition than jamming all of them in one line.

Anti-Pattern

In some places, you will identify a clever trick which is an anti-pattern in disguise. Now, this is a tricky one, you should voice it with the maintainers of the project about the feasibility of ditching the anti-pattern in favour of something else. Now it's up to the maintainers to decide whether time invested in porting the entire code to remove the clever trick is worthwhile or not. As the common saying goes don't fix if it ain't broke

Mix of too clever and readable code

If you are in this situation, I have bad news as this is the worst case for anyone to be in. As I said consistency is pretty important for collaborating and overall health of a project. If you find the project using a mix of clever code at some places and a more readable code at some places, it becomes difficult to reason about with code. You should definitely voice it out and try to get rid of too clever code as soon as possible.

You are starting a new project

Note that the line between clever code and readable code keeps changing year after year. What you think is readable today might not be readable tomorrow for the new kid, so decide to go for whatever you and your team find readable today and please stick with it.

Refactoring is a costly affair! pick your coding style and keep it consistent and predictable.

[Alain Van Hout]

The principle of least surprise is typically a good guideline. As such, the explicit if statement will usually be the obvious choice. Unless of course the situation is atypical and both are equally unsurprising e.g. when you are the only one who will ever work on the code (say, a hobby project) or there are clear conventions in your team, and the && conditional made it on that list.

[Nested Software]

I think this question can't really be answered without knowing who will be working on the code.

Let's take something really basic like loops. Depending on the case, we could replace loops with, say, recursion, higher order functions (map, fold), or list comprehensions. I would say that I usually try to replace loops in my own code with something like the above if it is reasonable to do so. I consider the resulting code more expressive, and there is usually less boilerplate.

However, all of these constructs require a more complicated mental model of what's going on than just following a loop. If the people maintaining the code aren't familiar with these constructs, they may find that the resulting code is actually harder to understand and harder to debug.

If I'm just writing the code for myself, I can do whatever feels right to me. If I'm writing code as part of a team, we need to build up a set of practices together that reflect the capabilities and proclivities of the team.

In general terms, I think it's good to always ask oneself if one's code is more complicated than necessary, and to avoid adding logic only because "maybe it will be needed in the future." It doesn't take very many steps to go from code that is quick to write and easy to understand, to something that's all of a sudden rather complicated. I think the latter can happen when one tries to optimize prematurely for performance or because one takes into consideration the most general case possible when that may not actually be required.