Coding: conciseness/efficiency vs readability

Question

I am fairly new to c# and trying to learn best practices. I've been faced with many situations over the last week in which I need to make a choice between longer+simpler code, or shorter code that combines several actions into a single statement. What are some standards you veteran coders use when trying to write clear, concise code? Here is an example of code I'm writing with two options. Which is preferable?

A)

    if (ncPointType.StartsWith("A"))//analog points
    {
        string[] precisionString = Regex.Split(unitsParam.Last(), ", ");
        precision = int.Parse(precisionString[1]);
    }
    else
    {
        precision = null;
    }

B)

    if (ncPointType.StartsWith("A"))//analog points
        precision = int.Parse(Regex.Split(unitsParam.Last(), ", ")[1]);
    else
        precision = null;

C)

    precision = ncPointType.StartsWith("A") == true ? 
            int.Parse(Regex.Split(unitsParam.Last(), ", ")[1]) : 
            null;

Answer 1

It's just like writing in English. Concise is rarely a bad thing, as long as the reader can see all the information they need to understand the intent.

The reason option C isn't perfectly readable is not because it's TOO concise, it's because it's not concise enough. Look at it again:

precision = ncPointType.StartsWith("A") == true ? 
            int.Parse(Regex.Split(unitsParam.Last(), ", ")[1]) : 
            null;

What if it said this?

precision = ncPointType.StartsWith("A") ? SecondToken(unitsParam.Last()) : null

That seems pretty readable to me. It's very clear that if ncPointType starts with an A, we'll set the precision to the second token in the last parameter of unitsParam. Otherwise we'll set it to null.

The details of tokenization is irrelevant to the person reading this line of code. But if they want to know, they can look at the method, which will say.

public int SecondToken(string param)
{
    return int.Parse(GetTokens(param)[1]);
}

Makes sense. Very clear. And then ...

public string[] GetTokens(string param)
{
    return Regex.Split(param, ", ");
}

Now you're going to say that this is a lot more words than any of your options. And it is. But think of each method as part of a glossary of terms. You're creating a language as you go.

Not doing this is like trying to write a story about your pets, having removed the words dog and cat from the dictionary.

There are a lot of words in the definition of dog; even more in the dictionary as a whole. But not having that definition for dog means you have to say "domesticated carnivorous mammal" instead and your story becomes less readable.

That said, as I alluded at the beginning, there is a point at which you lose information. For example:

precision = ncPointType.IsOk() ? GetIt() : Dont();

This is a step too far. To beat the last piece of usefulness out of an analogy: It's like writing a story about your pets, referring to them always as "it".

Answer 2

From Clean Code:

Each line represents an expression or a clause, and each group of lines represents a complete thought. Those thoughts should be separated from each other with blank lines.

I also find that ternary operators can be unreadable if the lines are too long; so I would go with B.

---

However, also from Clean Code, I would split out the conditional into it's own method.

It only takes a few seconds of thought to explain most of your intent in code. In many cases it's simply a matter of creating a function that says the same thing as the comment you want to write.

Consider, which is more readable, this:

if (ncPointType.StartsWith("A"))//analog points
    precision = int.Parse(Regex.Split(unitsParam.Last(), ", ")[1]);
else
    precision = null;

or this:

if (isAnalog(ncPointType))
    precision = int.Parse(Regex.Split(unitsParam.Last(), ", ")[1]);
else
    precision = null;

Answer 3

More code. More lines of code does not translate into more instructions executed.

In all of your examples I would expect the compiler to produce identical code (barring some variation in the line numbers mappings for use in diagnostics). I would also expect any benchmark on the three variations to produce identical results.

Compilers are very clever pieces of software written by a large number of very clever people over several years.

Your program will be maintained by one person (probably yourself) who may not be that clever and may only have a few minutes to read your code and resolve a problem.

So your priority should be the poor sucker who has read your code, not the super clever optimizing compiler which will render any old garbage into reasonably efficient machine code.

The key here is readability and comprehension. Whatever makes your intention clearer is the better code. You can do this by splitting dense lines of code, or, conversely you can do this by reducing many lines of code to a single terse statement -- its all a matter of judgement.

Answer 4

Not being in your aware of the rest of your code, I clearly prefer option A.

A) In case you want to debug, you'll have an easy access to precisionString

B) Not so good, the return value of the regexp parsing will not be easily accessed in debugging. Also the absence of the {} is dangerous: adding one statement (eg: a debug line) will break the logic

C) Is actually too verbose it would also be accessible to have:

precision = ncPointType.StartsWith("A") /* Analog unit */ ? 
        int.Parse(Regex.Split(unitsParam.Last(), ", ")[1]) : 
        null;

(without the easy access on the regexp split return value)

Answer 5

If you can have both readability AND conciseness AND efficiency, that would be ideal. However, I agree that sometimes having too much conciseness can make code unreadable. For example, I often break my code into multiple lines and give each intermediate part of my calculations a name, rather than mashing complex calculations into one line of code.

Personally, I prefer if-else statements over the notation in C. It's easier to do breakpoints if you need to test a specific spot. If you mash code into too few lines, it's harder to test. Also, since the if-else statement is well-indented, a reader can see the structure of the code without even reading anything.

I suppose my personal rule is this: "If I were to explain my block of code to someone in the simplest English possible, would the code reflect my words?" If I find that I'd have to break the code down and explain each chunk of it, then the code itself needs to be broken down.

And as James Anderson mentioned, it most likely won't make much a of a difference in terms of performance. Usually, if I'm forced to choose, I'd choose readability over conciseness.

I'd recommend considering the 90/10 rule. 90% of your slowdown is usually concentrated in 10% of your code. Some people favor efficiency to the point where code is unreadable. Then, the next developer has to spend so much time deciphering the code that he has no time to optimize the 10% of the code that's actually causing efficiency problems. Make it easier for the next person to skim your code, so that he has more time optimizing what actually NEEDS to be optimized.