C# performance and readability with buttons enabling/disabling

Question

I've often found myself in front of that kind of code :

if(Something > 0)
{
    btnOne.Enabled = true;
    btnTwo.Enabled = true;
    btnThree.Enabled = false:
}
else
{
    btnOne.Enabled = false;
    btnTwo.Enabled = false;
    btnThree.Enabled = true:
}

And I've always wondered if it was better to let it like that, or put it like this :

bool ButtonEnabled = (Something > 0);

btnOne.Enabled = ButtonEnabled;
btnTwo.Enabled = ButtonEnabled;
btnThree.Enabled = !ButtonEnabled;

Realizing the question is a bit argumentative, let's put aside the "readability" factor and concentrate on the performance factor... What would be best ? One more assignation or a condition ?

Thanks in advance for your advices (or a even better way to write it) !

Edit : Corrected an error in my second snippet. Edit : The two initial examples weren't equivalent...

Since it is not the answer you want, I am writing in the comment. Don't bother about performance when it comes to code like this. Most of the time it doesn't matter (unless you are doing that in a loop which is highly unlikely). Go for readability! — Hemant, Oct 27 '10 at 06:33
Your two snippets are not equivalent. In the first `btnThree.Enabled` and `btnFour.Enabled` only get set when `Something <= 0` (and vice versa for the other two buttons), whereas in the second all buttons get set. — Matt Ellen, Oct 27 '10 at 07:12

score 5 · Accepted Answer · answered Oct 27 '10 at 06:33

That depends on the properties being called. As you know, a property can do any amount if things. In Windows Forms or WPF, I wouldn't worry about it. I'd argue for the latter style for correctness and readability. If you set all necessary variables every time, there is less chance of missing something and leaving one button in an invalid state.

I'd do something like

bool ButtonEnabled = (Something > 0);
btnOne.Enabled = ButtonEnabled;
btnTwo.Enabled = ButtonEnabled;
btnThree.Enabled = !ButtonEnabled;
btnFour.Enabled = !ButtonEnabled;

+1: I had exact same thoughts when I saw the code. I have seen countless incidents when developer forgot to disable a control when he should have or worse still, forgot to enable it back in certain case. — Hemant, Oct 27 '10 at 07:11

score 1 · Answer 2 · answered Oct 27 '10 at 06:31

1

Whatever performance difference you may see between the two will most likely be insignificant in this case, so I would pick the one that is most readable.

answered Oct 27 '10 at 06:31

Brian Rasmussen

114,645
34
221
317

Guffa · Answer 3 · 2010-10-27T07:02:22.903

You can't compare the two pieces of code, neither on readability nor on performance, as they give different results.

The version of the first code that is equivalent to the second would be:

if(Something > 0)
{
    btnOne.Enabled = true;
    btnTwo.Enabled = true;
    btnThree.Enabled = false;
    btnFour.Enabled = false;
}
else
{
    btnOne.Enabled = false;
    btnTwo.Enabled = false;
    btnThree.Enabled = true;
    btnFour.Enabled = true;
}

The version of the second code that is equivalent to the first would be:

bool ButtonEnabled = (Something > 0);

btnOne.Enabled = ButtonEnabled ? true : btnOne.Enabled;
btnTwo.Enabled = ButtonEnabled ? true : btnTwo.Enabled;
btnThree.Enabled = !ButtonEnabled ? false : btnThree.Enabled;
btnFour.Enabled = !ButtonEnabled ? false : btnFour.Enabled;

So, the first piece of code is clearly more efficient and readable than it's equivalent alternative, and the second piece of code is shorter and somewhat easier to maintain than it's equivalent alternative.

So in the second way, buttons will always stay in the same state, cool :p — Aurelien Ribon, Oct 27 '10 at 07:03

score 1 · Answer 4 · answered Oct 27 '10 at 06:58

Yep, unlike your application has one hundred thousand buttons displayed at the same time, concentrate HEAVILY on readability, not on micro-optimization ! The time it will take the UI layer to update the visual of your control will be 10.000 times longer than your "Enabled" assignments anyway !

Solution 2 is actually nearly what you will want to do when using data-binding (you were very near :p). Actually, you would code something more like:

public class MyClass {
    public bool IsSomethingTrue { get; set; } // with notification on property changed
    public bool IsSomethingFalse { get { return !IsSomethingTrue; } }

    private AMethod() {
        ...
        IsSomethingTrue = Something > 0;
        ...
    }

And your UI would be something like (WPF flavored):

<Button IsEnabled={Binding IsSomethingTrue} /> <!-- btn 1 -->
<Button IsEnabled={Binding IsSomethingTrue} /> <!-- btn 2 -->
<Button IsEnabled={Binding IsSomethingFalse} /> <!-- btn 3 -->
<Button IsEnabled={Binding IsSomethingFalse} /> <!-- btn 4 -->
<!-- Want a 5th button ? just add it without changing your code-behind ! -->

This pattern allows you to add as many buttons as you want without changing your methods each time. This is especially helpful when methods tends to be quite complicated, and it improves the readability.

It works for WPF, Qt, Java, and I think Winforms should provide some data-binding capability.

C# performance and readability with buttons enabling/disabling

4 Answers4