14

Context

I recently got interested into producing better formatted code. And by better I mean "following rules endorsed by enough people to consider it a good practice" (since there will never be one unique "best" way to code, of course).

These days, I mostly code in Ruby so I started to use a linter (Rubocop) to provide me some info on the "quality" of my code (this "quality" being defined by the community driven project ruby-style-guide).

Note that I will use "quality" as in "quality of the formating", not so much about the efficiency of the code, even if in some cases, code efficiency actually is affected by how the code is written.

Anyway, doing all that, I realized (or at least, remembered) a few things:

  • Some languages (most notably Python, Ruby and such) allow to make great one-liners of code
  • Following some guidelines for your code can make it significantly shorter and yet still very clear
  • Yet, following these guidelines too strictly can make the code less clear/easy to read
  • The code can respect some guidelines almost perfectly and still be of poor quality
  • Code readability is mostly subjective (as in "what I find clear might be completely obscure to a fellow developer")

Those are just observations, not absolute rules of course. You will also note that code readability and following guidelines might seem unrelated at this point but here the guidelines are a way to narrow down the number of ways to rewrite one chunk of code.

Now, some examples, to make all that more clear.

Examples

Let's take a simple use case: we have an application with a "User" model. A user has optional firstname and surname and a mandatory email address.

I want to write a method "name" which will return then name (firstname + surname) of the user if at least his firstname or surname is present, or its email as a fallback value if not.

I also want this method to take a "use_email" as parameter (boolean), allowing to use the user email as the fallback value. This "use_email" parameter should default (if not passed) as "true".

The most simple way to write that, in Ruby, would be:

def name(use_email = true)
 # If firstname and surname are both blank (empty string or undefined)
 # and we can use the email...
 if (firstname.blank? && surname.blank?) && use_email
  # ... then, return the email
  return email
 else
  # ... else, concatenate the firstname and surname...
  name = "#{firstname} #{surname}"
  # ... and return the result striped from leading and trailing spaces
  return name.strip
 end
end

This code is the most simple and easy to understand way to do it. Even for someone who does not "speak" Ruby.

Now let's try to make that shorter:

def name(use_email = true)
 # 'if' condition is used as a guard clause instead of a conditional block
 return email if (firstname.blank? && surname.blank?) && use_email
 # Use of 'return' makes 'else' useless anyway
 name = "#{firstname} #{surname}"
 return name.strip
end

This is shorter, still easy to understand, if not easier (guard clause is more natural to read than a conditional block). Guard clause also makes it more compliant with the guidelines I'm using, so win-win here. We also reduce the indent level.

Now let's use some Ruby magic to make it even shorter:

def name(use_email = true)
 return email if (firstname.blank? && surname.blank?) && use_email
 # Ruby can return the last called value, making 'return' useless
 # and we can apply strip directly to our string, no need to store it
 "#{firstname} #{surname}".strip
end

Even shorter and following the guidelines perfectly... but a lot less clear since the lack of return statement makes it a bit confusing for those unfamiliar with this practice.

It's here that we can start asking the question: is it really worth it? Should we say "no, make it readable and add 'return'" (knowing this will not respect the guidelines). Or should we say "It's fine, it's the Ruby way, learn the damn language!"?

If we take option B, then why not make it even shorter:

def name(use_email = true)
 (email if (firstname.blank? && surname.blank?) && use_email) || "#{firstname} #{surname}".strip
end

Here it is, the one-liner! Of course it is shorter... here we take advantage of the fact that Ruby will return a value or the other depending of which one is defined (since email will be defined under the same condition as before).

We can also write it:

def name(use_email = true)
 (email if [firstname, surname].all?(&:blank?) && use_email) || "#{firstname} #{surname}".strip
end

It's short, not that hard to read (I mean, we all have seen what an ugly one-liner can look like), good Ruby, it complies with the guideline I use... But still, compared to the first way to write it, it's a lot less easy to read and understand. We can also argue that this line is too long (more than 80 characters).

Question

Some examples of code can show that choosing between a "full-size" code and many of its reduced versions (down to the famous one-liner) can be hard since, as we can see, one-liners can be not that scary but still, nothing will beat the "full-size" code in terms of readability...

So here is the real question: where to stop? When is short, short enough? How to know when the code becomes "too short" and less readable (keeping in mind that it's quite subjective)? And even more: how to always code accordingly and avoid mixing one-liners with "full-size" chunks of code when I just feel like it?

TL;DR

The main question here is: when it comes to choose between a "long but clear, readable and understandable chunk of code" and a "powerful, shorter yet harder to read/understand one-liner", knowing those two are the top and the bottom of a scale and not the two only options: how to define where is the frontier between "clear enough" and "not as clear as it should be" ?

The main question is not the classical "One-liners vs. readability: which one is better?" but "How to find the balance between those two?"

Edit 1

Comments in the code examples are meant to be "ignored", they are here to clarify what's happening, but are not to be taken into account when evaluating the readability of the code.

closed as primarily opinion-based by gnat, Andres F., ChrisF Feb 28 '18 at 22:47

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 7
    Too short for an answer: keep iteratively refactoring until you're not sure it's better than the previous iteration then stop and reverse the last refactorisation. – Dom Feb 28 '18 at 14:20
  • 8
    I'd prefer variant 3 with the return keyword added. Those seven characters add quite a bit of clarity in my eyes. – cmaster Feb 28 '18 at 14:34
  • 2
    If you're feeling really horrible, you can write the whole thing as [firstname,surname,!use_email].all?(&:blank?) ? email : "#{firstname} #{surname}".strip... because false.blank? returns true and the ternary operator saves you a few characters... ¯\_(ツ)_/¯ – DaveMongoose Feb 28 '18 at 16:10
  • 1
    OK, I have to ask: what clarity is the return keyword supposed to add?! It provides no information whatsoever. It’s pure clutter. – Konrad Rudolph Feb 28 '18 at 16:27
  • 2
    The notion that brevity begets clarity not only suffers from the law of diminishing returns, but reverses when pushed to extremes. If you are rewriting to make a short function shorter, you are wasting your time, and the same goes for trying to justify the practice. – sdenham Feb 28 '18 at 16:44
24

No matter what code you write, readable is best. Short is second best. And readable usually means short enough so you can make sense of the code, well named identifiers, and adhering to the common idioms of the language in which the code is written.

If this were language agnostic, I think this would definitely be opinion-based, but within the confines of the Ruby language I think we can answer it.

First, a feature and an idiomatic way of writing Ruby is to omit the return keyword when returning a value, unless returning early from a method.

Another feature and idiom combined is using trailing if statements to increase the readability of the code. One of the driving ideas in Ruby is to write code that reads as natural language. For this, we go to _why's Poignant Guide to Ruby, Chapter 3.

Read the following aloud to yourself.

5.times { print "Odelay!" }

In English sentences, punctuation (such as periods, exclamations, parentheses) are silent. Punctuation adds meaning to words, helps give cues as to what the author intended by a sentence. So let’s read the above as: Five times print “Odelay!”.

Given this, code example #3 is most idiomatic for Ruby:

def name(use_email = true)
  return email if firstname.blank? && surname.blank? && use_email

  "#{firstname} #{surname}".strip
end

Now when we read the code, it says:

Return e-mail if first name is blank, and surname is blank and use e-mail

(return) first name and last name stripped

Which is pretty darn close to the actual Ruby code.

It is only 2 lines of actual code, so it is pretty terse, and it adheres to the idioms of the language.

  • Nice point. It's true that the question was not meant to be Ruby-centric, but I do agree that it is not possible to have a language agnostic answer here. – Sudiukil Feb 28 '18 at 14:28
  • 8
    I find the idea of making code sound like natural language vastly overrated (and at times even problematic). But even without this motivation I arrive at the same conclusion as this answer. – Konrad Rudolph Feb 28 '18 at 16:31
  • 1
    There's one more tweak I would consider doing to the code. That is, to put use_email before the other conditions since it's a variable rather than a function call. But then again the string interpolation swamps the difference anyways. – John Dvorak Feb 28 '18 at 19:44
  • Structuring code following natural language structures can make you fall into the language traps. For example, when you read the next requirements do send an email if A, B, C but no D, following your premise would be natural to type down 2 if/else blocks, when probably would be easier to code if not D, send an email. Be careful at the moment of reading natural language and transform it into code because it can make you write a new version of the "Neverending story" . With classes, methods and variables. Not a big deal after all. – Laiv Mar 2 '18 at 14:33
  • @Laiv: Making code read like natural language doesn't mean literally translating the requirements. It means writing code so that when read out loud it allows the reader to understand the logic without reading every bit of code, character for character, language construct for language construct. If coding it if !D is better, that's fine as lond as D has a meaningful name. And if the ! operator gets lost amongst the other code, then having an identifier called NotD would be appropriate. – Greg Burghardt Mar 2 '18 at 16:05
16

I don't think you will get a better answer than "use your best judgement". In short you should strive for clarity rather than shortness. Often, the shortest code is also the clearest, but if you focus just on achieving shortness clarity may suffer. This is clearly the case in the last two examples, which requires more effort to understand than the previous three examples.

An important consideration is the audience of the code. Readability is of course totally dependent on the person reading. Do the people you expect to read the code (beside yourself) actually know the idioms of the Ruby language? Well this question is not something random people on the internet can answer, this is only your own decision.

  • I do agree with the audience point, but it's part of my struggle: as my software is often open-source, the audience might be composed by beginners as well as "Ruby gods". I could keep it simple to make it accessible to most people, but it kinda feels like a waste of the advantages offered by the language. – Sudiukil Feb 28 '18 at 13:29
  • 1
    As someone that has had to take over, extend and maintain some truly horrible code, clarity has to win. Remember the old adage - Write your code as if the maintainer is a vengeful Hell's Angel who knows where you live and where your kids go to school. – uɐɪ Feb 28 '18 at 14:46
  • 2
    @Sudiukil: That is an important point. I suggest you strive for idiomatic code in that case (i.e. assume good knowledge of the language), since it is unlikely beginners will contribute to open source code anyway. (Or if they do, they will be prepared to put in the effort to learn the language.) – JacquesB Feb 28 '18 at 15:16
7

Part of the problem here is "what is readibility". For me, I look at your first code example:

def name(use_email = true)
 # If firstname and surname are both blank (empty string or undefined)
 # and we can use the email...
 if (firstname.blank? && surname.blank?) && use_email
  # ... then, return the email
  return email
 else
  # ... else, concatenate the firstname and surname...
  name = "#{firstname} #{surname}"
  # ... and return the result striped from leading and trailing spaces
  return name.strip
 end
end

And I find it hard to read as it's full of "noisy" comments that just repeat the code. Strip them out:

def name(use_email = true)
 if (firstname.blank? && surname.blank?) && use_email
  return email
 else
  name = "#{firstname} #{surname}"
  return name.strip
 end
end

and it's now so much more readable. In then reading it, I think "hmm, I wonder if Ruby supports the ternary operator? In C#, I can write it as:

string Name(bool useEmail = true) => 
    firstName.Blank() && surname.Blank() && useEmail 
    ? email 
    : $"{firstname} {surname}".Strip();

Is something like that possible in ruby? Working down through your post, I see there is:

def name(use_email = true)
 (email if (firstname.blank? && surname.blank?) && use_email) || "#{firstname} #{surname}".strip
end

All good stuff. But that's not readable to me; simply because I have to scroll to see the whole line. So let's fix that:

def name(use_email = true)
 (email if (firstname.blank? && surname.blank?) && use_email) 
 || "#{firstname} #{surname}".strip
end

Now I'm happy. I'm not completely sure of how the syntax works, but I can understand what the code does.

But that's just me. Other folk have very different ideas on what makes for a nice to read piece of code. So you need to know your audience when writing code. If you are a teaching absolute beginners, then you'll want to keep it simple and possibly write it like your first example. If you work amongst a set of professional developers with many years of ruby experience, then write code that takes advantage of the language and keep it short. If it's somewhere in between, then aim for somewhere in between.

One thing I would say though: beware "clever code", such as in your last example. Ask yourself, does [firstname, surname].all?(&:blank?) add anything other than making you feel clever because it shows off your skills, even if it's now a bit harder to read? I'd say this example likely does full into that category. If you were comparing five values though, I'd see it as a good code. So again, there's no absolute line here, just be mindful of being too clever.

So in summary: readability requires you know your audience and target your code accordingly and write succinct, but clear code; never write "clever" code. Keep it short, but not too short.

  • 2
    Well, I forgot to mention it, but the comments were meant to be "ignored", they are here only to help those who don't know Ruby well. Valid point though about the audience, I did not thought about that. As for the version that makes you happy: if it is the line length that matters, the third version of my code (the one with only one return statement) kinda does that and is even a bit more understandable, no ? – Sudiukil Feb 28 '18 at 13:18
  • 1
    @Sudiukil, not being a ruby developer, I found that the hardest to read and it didn't fit what I was looking for (from another language's perspective) as the "best" solution. However, for someone familiar with the fact that ruby is one of those languages that returns the value of the last expression, it likely represents the simplest, easiest to read version. Again, it's all about your audience. – David Arno Feb 28 '18 at 13:41
  • Not a Ruby developer but this makes a lot more sense to me than the top-voted answer, which reads like "Here's what I'm going to return [footnote: under a specific long condition]. Also, here's a string that came late to the party." Logic that is essentially just a case statement should be written like a single uniform case statement, not spread across multiple seemingly unrelated statements. – Paul Feb 28 '18 at 17:18
  • Personally I would go with your second code block except I would combine the two statements in your else branch into one: return "#{firstname} #{surname}".strip – Paul Feb 28 '18 at 17:29
2

This is probably a question where it's hard not to give an opinion-based answer, but, here are my two cents.

If you find that making the code shorter doesn't impact readability, or even improves it, go for it. If the code becomes less readable, then you need to consider if there is a fairly good reason to leave it that way. Doing it just because it's shorter, or cool, or just because you can, are examples of bad reasons. You also need to consider if making the code shorter would make it less understandable for other people you work with.

So what would be a good reason? It is a judgement call, really, but an example might be something like a performance optimization (after performance testing, of course, not in advance). Something that gets you some benefit that you are willing to pay for with decreased readability. In that case, you can mitigate the drawback by providing a helpful comment (that explains what the code does, and why it had to be made a bit cryptic). Even better, you can extract that code into a separate function with a meaningful name, so that it's just one line at the call site that explains what is happening (via the name of the function) without going into details (however, people have differing opinions about this, so this is another judgement call you have to make).

1

The answer is a bit subjective, but you have to ask yourself with all the honesty you can muster, if you would be able to understand that code when you come back to it in a month or two.

Each change should improve the average person's ability to understand the code. To make code understandable, it helps to use the following guidelines:

  • Respect the idioms of the language. C#, Java, Ruby, Python all have their preferred ways of doing the same thing. Idiomatic constructs help with understanding code you aren't familiar with.
  • Stop when your code becomes less readable. In the example you provided, that happened when you hit your last pairs of reducing code. You lost the idiomatic advantage of the previous example, and introduced a lot of symbols that require a lot of thinking to truly understand what's going on.
  • Only use comments when you have to justify something unexpected. I know your examples were there to explain constructs to people less familiar with Ruby, and that's OK for a question. I prefer to use comments to explain unexpected business rules, and avoid them if the code can speak for itself.

That said, there are times where expanded code helps with understanding what's going on better. One example with that comes from C# and LINQ. LINQ is a great tool and can enhance readability in some situations, but I've also run into a number of situations where it was a lot more confusing. I've had some feedback in peer review that suggested turning the expression into a loop with appropriate if statements so that others could maintain it better. When I complied, they were right. Technically, LINQ is more idiomatic for C#, but there are cases where it degrades understandability and a more verbose solution improves it.

I say all that to say this:

Improve when you can make your code better (more understandable)

Remember, you or someone like you will have to maintain that code later. The next time you come across it might be months down the line. Do yourself a favor and don't chase reducing line counts at the cost of being able to understand your code.

0

Readability is a property you want to have, having-many-one-liners isn't. So rather than "one-liners vs readability" the question should be:

When do one-liners increase readability, and when do they harm it?

I believe one-liners are good for readability when they fulfill these two conditions:

  1. They are too specific to be extracted to a function.
  2. You don't want to interrupt the "flow" of reading the surrounding code.

For example, let's say name wasn't a good ... name for your method. That combining the first name and surname, or using the email instead of the name, were not natural things to do. So instead of name the best thing you could come up with turned out long and cumbersome:

puts "Name: #{user.email_if_there_is_no_name_otherwise_use_firstname_and_surname(use_email)}"

Such long name indicates that this is very specific - if it was more general you could have found a more general name. So wrapping it in a method does not helps with neither readability (it's too long) nor DRYness (too specific to be used anywhere else), so it's better to just leave the code in there.

Still - why make it a one-liner? They are usually less readable than multiline code. This is where we should check my second condition - the flow of the surrounding code. What if you have something like this:

puts "Group: #{user.group}"
puts "Title: #{user.title}"
if user.firstname.blank? && user.surname.blank?) && use_email
  name = email
else
  name = "#{firstname} #{surname}"
  name.strip
end
puts "Name: #{name}"
puts "Age: #{user.age}"
puts "Address: #{user.address}"

The multiline code itself is readable - but when you try to read the surrounding code (printing the various fields) that multiline construct is interrupting the flow. This is easier to read:

puts "Group: #{user.group}"
puts "Title: #{user.title}"
puts "Name: #{(email if (user.firstname.blank? && user.surname.blank?) && use_email) || "#{user.firstname} #{user.surname}".strip}"
puts "Age: #{user.age}"
puts "Address: #{user.address}"

Your flow does not get interrupted, and you can focus on the specific expression if you need to.

Is this your case? Definitely not!

The first condition is less relevant - you already deemed it to be general enough to deserve a method, and came up with a name for that method that is much more readable than it's implementation. Obviously you wouldn't extract it to a function again.

As for the second condition - does it interrupt the flow of the surrounding code? No! The surrounding code is a method declaration, that picking the name is it's sole purpose. The logic of picking the name is not interrupting the flow of the surrounding code - it is the very purpose of the surrounding code!

Conclusion - don't make the entire function body a one-liner

One-liners are good when you want to do something a little complex without interrupting the flow. A function declaration is already interrupting the flow (so that it won't be interrupted when you call that function), so making the entire function body a one-liner is not helping readability.

Note

I'm referring to "full blown" functions and methods - not inline functions or lambda expressions that are usually part of the surrounding code and need to fit within it's flow.

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