4

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;
6
  • 1
    That depends on your project. Some require quick development, others require efficient code, and others require long-term maintainability. No one here can tell you what's best for your project.
    – Doval
    May 15, 2014 at 21:50
  • 6
    Conciseness of expression does not imply efficiency of execution. Code for clarity of expression and then use actual measurements to meet performance constraints. The interaction between conciseness and clarity is complex and somewhat subjective which is probably why this question is getting down votes. May 15, 2014 at 22:20
  • C is easy enough to understand. All other things being equal, less code is better than more. May 15, 2014 at 22:31
  • 1
    One off-topic note: it is considered bad practice to assign null to variables unless the null really has a meaning and is not chosen because "we don't know what to assign" or "there is no meaningfull value to assign". You may instead want to either not even declare the variable at all (and of course don't use it anywhere after) or throw an exception if you don't know how to handle the case in this scope of code.
    – valenterry
    May 15, 2014 at 22:36

5 Answers 5

17

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".

6
  • Very good. The code is done when there is nothing left to remove.
    – david.pfx
    May 16, 2014 at 14:47
  • Your line is certainly very readable, but isn't creating two helper methods just for this going too far? (Assuming those methods won't be reused elsewhere.)
    – svick
    May 16, 2014 at 17:39
  • @svick: Going too far into what? What downside are you envisioning?
    – pdr
    May 16, 2014 at 18:05
  • 1
    @svick: I would argue that for pretty much any change I can think of here, you're maintaining less code. You're either going to change one of the methods or the calling code; rarely more than one of them, certainly not all three. And you're going to find it easier to find the bit you're looking for, when you're not figuring out which bracket goes with which in (Regex.Split(unitsParam.Last(), ", ")[1]).
    – pdr
    May 16, 2014 at 18:17
  • 2
    I hope the SecondToken function is a joke. May 16, 2014 at 19:01
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;
1

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.

3
  • Your first line has one major issue: bugs are proportionally related to lines of code. I generally agree with your tone, though.
    – Magus
    May 16, 2014 at 14:23
  • Agreed. Bugs and time to read have a first order correlation with size, second order correlation with complexity.
    – david.pfx
    May 16, 2014 at 14:44
  • @Magus: So I can reduce the number of bugs by removing all line breaks in my code? Don't think so.
    – gnasher729
    Oct 28, 2016 at 21:22
0

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)

0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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