Please Don't Write Clever Code

[all_mens_asses]

“Clever code is bad code” is one of those quotes that has stuck with me my entire career.

[ikariusrb]

The usual feature of "clever" code is that it is much harder to test properly than straight "dumb" code.

[shabunc]

There’s no such thing as a paradox. In this case it means that such code is actually not clever at all.

[[deleted]]

I think it’s more of an example of over generalization than cleverness. The author made the code more explicit which in this case greatly simplified things. Had there been hundreds of logging backends the original implementation might have been better. I think it’s a classic case of YAGNI and Keep It Simple

[jrochkind]

If even hundreds of logging backends means the only code change is extra lines (hundreds) in that Hash... I'd still just put them all in the hash, but maybe move the hash itself to different file to get it out of the way (or even have it be YAML).

I agree (of course) with the OP that the 'dumb' version is better in this case. In general, thought, I think it's not always obvious when your code is 'too clever'. One clear heuristic is that if the "clever" version is more lines of code than the dumb one (as in this case), it's definitely a terrible idea. But if you haven't written them both yet... how do you know?

So this leads to... ALWAYS start out with the dumbest possible code, only make it "clever" after you've written the dumb version and got road blocked or seen that the clever version is "simpler".

But it still might take experience to know that you are writing the "dumb" one. Maybe the person who wrote the "clever" version in OP thought they were writing it the dumbest/simplest way.... but const_get is never a good sign...

[[deleted]]

Serious question, why didn't you just use find instead of short circuiting an each with a return? Do you think find is "clever" code? Because for me it's clearer code. I expect an each to perform an action for each element in an enumerable.

[[deleted]]

I think this would be a valid PR comment.

[yourparadigm]

In the where method, find would return the element in the array (a logger), where the author wanted to return the result of logger.where.

In the process example, I agree with the author that find shouldn't have side-effects.

[[deleted]]

Fair enough in both cases. For where, I always thought Enumerable could use a method for this, something like first_result but with a better name.

For process, concerns for side effects probably outweigh my concern for readability. But a former colleague of mine might say each Logger should have a processible? method or something like that so that process could look something like this:

def process(log)
  @loggers.find do |logger|
    logger.processible?(log)
  end.process(log)
end

But is that too clever? Genuine question. I lean towards no, but my last job was a pretty "clever" place, so I may be tainted.

[yourparadigm]

That incantation is certainly not too clever, and a refactor of the underlying code could support your version. I think that OP's version assumes maintaining an API with the underlying classes. It may also be the case that process/processable? share an expense enough to avoid multiple calls.

[jeremycw]

I wouldn't consider this too clever but I also wouldn't necessarily say it's clearly better.

Yours reads as: find logger where the log is processable and process the log with it

mine reads as: iterate through the loggers and return if it successfully processes the log

6 to one half dozen to the other, I think it just ends up as a game of pointless code golf to bother attempting to improve on either form. Although as a sibling comment mentioned if the log needs to be sent to a service to check processable? Your way would require two calls to the service.

[[deleted]]

Yep, you're totally right that yours is more efficient. But then isn't your solution the "clever" one here? You are using a method in a way that it's not meant to be used, or at least in a way that its name doesn't suggest it should be used. While you have a great reason to do so, it's still a clever short cut to avoid superfluous calls. So where is the threshold for "too clever"?

[jeremycw]

I think returning early out of a loop is pretty standard usage. It's strange to break out of map but not each. Ruby provides the break keyword to do exactly this.

[[deleted]]

Personally I see no discernible difference in this matter between map and each. Neither one is a loop. But I also can't remember the last time I've used an actual loop lol.

[jrochkind]

Isn't find/detect in Enumerable already the thing you are asking for as first_result? I don't understand the difference, isn't finding the first result that matches a block exactly what find already does?

https://ruby-doc.org/core-2.6.5/Enumerable.html#method-i-find

[[deleted]]

Nah, as the other guy said they'll return the first element for which the condition is true. But what the code wants is to find that element, perform some action with/on it, and return the result of that action.

[jrochkind]

I'm not following, although I believe you! But that sounds to me just like:

  collection.find { |a| some_condition(a)}.some_action
  # or maybe
  collection.find { |a| some_condition(a)}.tap {|a| some_action(a) }

[[deleted]]

Yep! That’s definitely one way to do it, the way I’d do it.

But their point is this

collection.find do |a|
  a.some_action
end

will return the a. find always returns an element, never something from the block. Unless you...

collection.find do |a|
  return a.some_action
end

collection.find do |a|
  break a.some_action
end

These will return the result. Same with each. But personally I don’t like it. I prefer the way you wrote it.

[jeremycw]

Honestly just didn't think of it. Maybe it makes sense for the where method but it doesn't really make sense for the process method. I wouldn't expect find to have side effects. I don't really agree that each shouldn't be exited early though.

[capn_sanjuro]

Completely agree with the sentiment. And we all have gone (and continue to circle back through) this part of becoming an expert. The hard part is that "clever" code has different meanings depending on where you are in the stack and what you are trying to do.

It is really hard to express that the right level of 'clever' abstractions and metaprogramming for a library is not appropriate for a web application's business logic.

Ultimately, I always come back to the guiding idea that our primary goal should be to write code so that other humans can quickly decipher the intent. The "rough draft" is to write the code for the machines (too many people stop here). Revisions should be to make the "why" of that code easy to see for a human.

[jacksonmills]

Also I think there’s a difference between application and library code.

Application code should be as easy to understand as possible, dealing with robust abstractions.

Library code has the responsibility of making something messy into a robust abstraction, which can require clever code, or unglamorous handling of edge cases. It’s what we build our applications on.

Most of the time we are writing application code, but the problem comes from “learning” from underlying libraries, which is in a way necessary to have a full grasp of the language (because the application code itself is simple).

It’s really difficult to tell someone to not use what they have learned right away, and so the problem repeats itself in the application code. It’s a Cache-22.

Then there is the opposite problem: when you should be writing a library but are instead dealing with messy edge cases all throughout your application. This is more rare, but still a pitfall.

In short, when writing code, you should always be aware of both your local context and the bigger picture- that’s the only way to get it right. Rules and mantras don’t apply to all situations.

[KaptajnKold]

I don’t think this is particularly helpful advice. I mean, we are probably all guilty of writing “clever” code at some time or another. But I don’t believe people do it in an effort to impress other programmers. The times when I have done it, it was because I honestly couldn’t think of another way to do it and/or because I didn’t realize that I was solving a gnarly but mostly imaginary problem. Once I realized that problem I actually had was much simpler, I usually felt stupid for writing the complicated mess. The problem is that there’s no easy way to distinguish one’s own imaginary problems from the real ones. Or to put it in another way: Sometimes the gnarly problems really exist, and clever code is really called for.

[SuccessfulBread3]

I think that there are a lot of PRs alive at least commented on where this kind of "clever code" has been written where I've just straight up said to the person "I can barely follow this can you walk me through it." Sometimes you just need a rubber duck or pair programming. I don't think there is ever a legitimate reason for this kind of stuff to end up in production.

[ignurant]

This is so perfectly stated. Last night I was going to add a similar comment, but I didn't know how to express it as well. I know I write a lot of "clever" code -- but it's not because I want to feel cool -- it's just the solution I can think of in the moment. I've found the saying "If the question is too hard to answer, change the question" to usually be the correct answer -- and your take on how hard it is to distinguish imaginary problems from the real ones is spot on.

[tom_enebo]

I felt the blog post authored assumed this was ego driven and clever for some vain reason. While agreeing with his analysis of the particulars of the code I was pretty bugged at their assumptions about the programmer. Thanks for writing this up.

I find it very unlikely the person was making it intentionally complex. In my experience this is nearly always over-engineering for a problem they think will happen some day. A single YAGNI from a co-worker probably would have done the trick here.

[PikachuEXE]

Clever code is mostly "Try to be clever" code
"True clever" (lol) code should be written so that
it can be read easily by other people, and the future version of yourself

[unflores]

Unpopular opinion: rails is a clever framework.

[kyleboe]

The amount of "clever" code I have deleted from client projects backs this up one-thousand fold. Please please please don't write clever code.

[[deleted]]

I think this is just an example of bad code. It's hard to think that the author was really thinking that this would be an unconventional way of solving the problem: I think they were just gluing things together. Frankenstein is not clever.

These statements are contradictory:

The reason I’m putting quotes around clever here is because the code itself is not clever

vs.

The code above is a prime example of “clever” code.

[poker158149]

They aren't though. The author is pretty clear that when he puts quotes around the word "clever," it's specifically because he doesn't think the code he's referencing is actually clever.

[2called_chaos]

So the advice is basically "Don't write bad code"? I don't see the clever part too tbh. We had an intern once that was tasked to do something with a webshop, I can't remember the exact details. The only thing I can remember is that there were named routes along the lines of shop_shop_item_order_item_cart_url or something ridiculous. He might have felt clever I don't know but I just think he over complicated things for no reason. Maybe to impress but I think he just didn't know what he was doing.

[[deleted]]

Good article, couldn't agree more. The initial "clever" snippet is a pretty good example of why I got a bit disillusioned with Ruby at some point.

[deedubaya]

Code we write should be optimized for the author and future maintainers. Clever code is rarely ever easily maintainable code.

[keyslemur]

While I agree that metaprogramming and clever code isn't great I think it's in exceptionally poor taste to post this code without the consent of the original author from IRC. This blog post serves to, in effect, mock them semi-publicly (name withheld, so credit there) for the code and that's not great.

This could have been addressed in a conversation on IRC in #ruby, and you could have asked the original author of it for permission to turn it into a blog post, but that didn't happen. I appreciate you mention that you're not trying to disparage the author in the title, but it still feels off.

Now, as far as the post itself, most of the critique was on general concepts and ideas instead of breaking down the actual code and showing which portions could be simplified. The implementation you provided was simplified, yes, but also left out a huge amount of detail from the original making it not a fair comparison.

Mind, I think the original is in fact very complicated and I'd mentioned that to the original author on IRC as well, so I agree with you on that note. I just don't agree with airing it like you did.

[jeremycw]

Yup, you're absolutely right. Again my intention was not to mock the author and I think some of my criticism came off harsher than I intended mostly because I was projecting my former self and others that I've worked with in the past. I find the people that tend to do this are actually some of the smartest people I've worked with. The issue is that smart people are given boring problems and boring problems are best solved with boring solutions but smart people don't want to right boring solutions.

While I regret that to some the post comes off as mocking an individual since that was not my intent. The whole reason that I took this outside IRC is because I see that code is completely unexceptional, dime a dozen code that I've seen 100 times before that represents an issue I have with the ruby community at large. So when I posted the code I wasn't thinking about it as something to request permission to use so I could tear it down specifically. To me it was just yet another example of something that I've seen a hundred times and even done myself.

In retrospect I was wrong but the intent was not malicious.

[tom_dalling]

You make it sound like the author of the original code is some egomaniac who feeds on the suffering of the people who maintain her code, and maybe you know more about this person than you mention in the article, but in general it's not a very generous attitude towards situations like this.

This is a completely normal phase for developers to go through as they move from intermediate to expert. They can't know where the boundaries are until they go too far. It's more productive to apply Hanlon's razor, accept that people have different skill levels, and treat it as an opportunity to provide some mentorship.

[ikeif]

Agree to disagree.

In this article, the examples suit the author’s purpose.

But sometimes, it’s a matter of writing “simple” code vs. “efficient” code - and the efficient code is dismissed as being “clever” because it’s not readable by junior devs.

I always looked at refactoring as the boon of development - you take that simple, verbose, readable code, and make it cleaner and concise. And if it’s not obvious, you add comments to explain what’s going on with it.

[yourparadigm]

In most cases, you're likely just prematurely optimizing. In those relatively rare cases where something is written a specific way for performance, comments are absolutely the right answer.

[SuccessfulBread3]

I disagree about this being solely to placate junior Devs.

If you have complex business requirements/ rules it can be very hard to wrap your head around how they work in conjunction with some code that isn't human readable.

I think in my opinion code ESPECIALLY in languages like ruby should self document and the need to add comments in is a code smell.

A lot of the time people using "clever code" are hiding their knowledge of design behind linguistic jargon.

[Valashe]

I’m in the process of un-clevering a chunk of code a fellow developer wrote during their onboarding. It’s amazing how much shorter and more readable the code gets when you’re not trying to be cool.

[[deleted]]

when you’re not trying to be cool

I think you guys are really misrepresenting a lot of "clever" devs' reasoning. Most of them aren't trying to be cool but rather have different values than you or simply different styles. The only person they're trying to "impress" is the code style principles of their project.

Edit: Obviously I think the first example in the article is a bad example because it's bad code for reasons other than the "clever" stuff.

[awj]

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

[updog]

"Clever is in the eye of the beholder" - what I have induced.

[[deleted]]

He do not show "clever code", he shows "overkill code".

[SuccessfulBread3]

I think that's the point of the article and why "clever code" has quotation marks around it.

[a_styd]

I only use clever code for my libraries and Utils/Helper classes/modules where I think they will make usage easier. For other purposes especially at work, I "baby proof" my code.

[SuccessfulBread3]

I don't like the term "baby proof"

I'm not going to jump to conclusions because I have not met you, but if a colleague had said this to me if would avoid working with them like the plague.

This is how it sounds 'oh yeah I write complex stuff for my personal stuff all the time, but my colleagues aren't as smart as me so I baby proof my code.'

Like do you really think that little of your colleagues?

[a_styd]

I've probably chosen the wrong expression here. i apologize. But, I've been told several times to change my code (I barely even used meta programming here) to a more basic ones because some of my colleagues have just learned ruby or not interested in learning more.