Coding Efficiency vs Readability in C#

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?

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

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

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

Best Answer

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

Stop!

You have a bigger problem than the readability. It looks like you don't understand how object initializers work.

Let's create a disposable class which traces its execution:

public class Demo : IDisposable
{
    private string hello;

    public Demo()
    {
        Debug.WriteLine("The parameterless constructor was called.");
    }

    public Demo(string hello)
        : this()
    {
        Debug.WriteLine("The constructor with parameters was called.");
        this.Hello = hello;
    }

    public string Hello
    {
        get
        {
            Debug.WriteLine("The getter was called.");
            return this.hello;
        }

        set
        {
            Debug.WriteLine("The setter was called.");
            this.hello = value;
            throw new NotImplementedException();
        }
    }

    public void Dispose()
    {
        Debug.WriteLine("The disposer was called.");
    }
}

Now, let's write an app which initializes this class in three different ways (sorry, it's too big; I'll explain it more in details later):

public static class Program
{
    // --- Interesting stuff ---
    private static void WithPropertiesInitialization()
    {
        using (var demo = new Demo())
        {
            demo.Hello = "Hello World!";
            // Do nothing.
        }
    }

    private static void WithObjectInitializer()
    {
        using (var demo = new Demo { Hello = "Hello World!" })
        {
            // Do nothing.
        }
    }

    private static void WithConstructor()
    {
        using (var demo = new Demo("Hello World!"))
        {
            // Do nothing.
        }
    }

    // --- Not so interesting stuff ---
    public static void Main()
    {
        Debug.Listeners.Add(new TextWriterTraceListener(Console.Out));

        Console.WriteLine("With constructor:");

        try
        {
            WithConstructor();
        }
        catch (NotImplementedException)
        {
        }

        Console.WriteLine();
        Console.WriteLine("With object initializer:");

        try
        {
            WithObjectInitializer();
        }
        catch (NotImplementedException)
        {
        }

        Console.WriteLine();
        Console.WriteLine("With properties initialization:");

        try
        {
            WithPropertiesInitialization();
        }
        catch (NotImplementedException)
        {
        }

        Console.WriteLine();
        Console.WriteLine("Press any key to continue...");
        Console.ReadKey(true);
    }
}

There are three types of initialization here:

Through the constructor, the value to set to the property being passed to the constructor,
With the usage of an object initializer,
With a parameterless constructor, the property being assigned later.

Before continuing to read the answer, ask yourself two questions:

Do those initialization techniques result in the same console output?
Is there a bug?

Those techniques are totally different, and there is effectively two bugs: only one of them is correct.

1. Constructor

using (var demo = new Demo("Hello World!"))
{
    // Do nothing.
}

has nothing wrong, but it's the actual implementation of the constructor which is wrong. Constructors shouldn't throw exceptions, and one of the reasons is that it may cause inconsistent state when the object is used inside a using block: it's not fully initialized yet, so it will not be disposed, causing disaster if the unfinished initialization allocated some resources which should have been disposed.

using (var a = new A())
{
    DoWork(a);
}

is basically the same as:

var a = new A();
try
{
    DoWork(a);
}
finally
{
    a.Dispose();
}

As you can see, there is no try around the constructor, so an exception within it would terminate the program before reaching a.Dispose(): in all cases, we can't invoke a method of an object which is not initialized yet.

The setter of Hello throws an exception; the constructor should have been using the backing field directly, instead of the property.

2. Object initializer

    using (var demo = new Demo { Hello = "Hello World!" })
    {
        // Do nothing.
    }

is itself incorrect, and if you have used Code Analysis even at Minimum Recommended Rules level, you would have seen the warning CA2000, telling that:

object '<>g__initLocal0' is not disposed along all exception paths.

What it that?!

That is the indicating that somewhere, an object is created and not disposed. To avoid concurrency issues, when you use an object initializer, a temporary instance of created:

this.Pet = new Gremlin
{
    Title = "Mogwai",
    Age = 14,
}

is transformed by the compiler into:

Gremlin someCrypticName = new Gremlin();
someCrypticName.Title = "Mogwai";
someCrypticName.Age = 14;
this.Pet = someCrypticName;

Indeed, when you use an object initializer inside a using (...):

The temporary object is created,
The properties are assigned,
The temporary object is assigned to the one which is used inside a using,
The using block runs.

Any exception thrown when assigning properties will prevent disposing the object, since the assignment happens before the try/finally.

3. Late assignment

using (var demo = new Demo())
{
    demo.Hello = "Hello World!";
    // Do nothing.
}

is correct. Since the constructor is not expected to throw an exception (and if it will, it would be the fault of the person who implemented the constructor, not the person who is using it within the using), all exceptions will occur within the try block, resulting in the demo object being properly disposed.

Conclusion:

using should contain either:

A parameterless constructor,
A constructor which takes parameters,

given that constructors are not expected to throw exceptions.

using should not contain an object initializer: properties should be initialized within the using { ... }, in order to guarantee the proper disposal of objects.

Readability

Now that you've understood that the two pieces of code in your question are totally difference, since one is correct, and another one is buggy, let's talk readability.

Until C# 4.0, object initialization was slightly more readable than a similar constructor:

var product = new Product("PowerEdge R620 rack server", 1, "Intel® Xeon® E5-2620", 16, 1929, true);

Here, I have no idea what is "1" or "16" or "1929". A person who is not at all familiar with hardware wouldn't know what "Intel® Xeon® E5-2620" is. No one would find what is true.

Instead:

var product = new Product
{
    Title = "PowerEdge R620 rack server",
    RackUnits = 1,
    ProcessorName = "Intel® Xeon® E5-2620",
    MemoryInGB = 16,
    PriceInDollars = 1929,
    IsAvailable = true,
}

is much more readable.

.NET Framework introduced named arguments, and since then, a constructor can be as readable as an object initializer, while being more compact. For example, title is explicit enough, and we would consider that anyone will understand the names of processors, so we can write:

var product = new Product(
    "PowerEdge R620 rack server",
    "Intel® Xeon® E5-2620",
    rackUnits: 1,
    memoryInGB: 16,
    priceInDollars: 1929,
    isAvailable: true);

I have an office mate that told me that object initialization is much readable and faster than constructor.

We have seen that with named arguments, a constructor can be as readable as an object initializer. What about performance?

Above, I've explained that an object initializer this.a = new A { B = b } is translated by the compiler into:

A temp = new A();
temp.B = b;
this.a = temp;

If the only thing the constructor is doing is to assign fields or properties from arguments, then the performance would be approximately the same. Unless your office mate can give precise profiling results and benchmarks proving his point, his statement about the performance should be considered wrong.

Best Answer

Related Solutions

C# Design – Coding to Interfaces

C# ASP.NET – Object Initializer Problem with Readability

Stop!

Readability

Related Topic