1

I was faced with this question by my team leader.

Statement 1 (written by me):

lnkbtn1.visible = lnkbtn2.visible = lnkbtn.visible = false; 

Statement 2 (written by my team leader):

lnkbtn1.visible = false;
lnkbtn2.visible = false;
lnkbtn3.visible = false;

Which statement is better and why?

  • 1
    Well, there are ways to find out whether or not one is faster. You could profile them (in which case I'm 100% sure you won't be able to find a difference), or you could examine the IL. – user16764 Feb 23 '14 at 15:34
  • 6
    Its interesting that statement 1 is probably faulty, but you missed that due to worse readabilty (if you had written lnkbtn instead of lnkbtn3 in statement 2, too, everyone would have noticed immediately). – Doc Brown Feb 23 '14 at 22:17
  • 3
    Do not worry about performance in code that isn't in a loop that will cause it to be repeated many, many times. – Loren Pechtel Feb 24 '14 at 3:06
  • I think your team leader is insane. He sees identical code but expects different results. – Reactgular Mar 16 '14 at 17:30
12

The performance difference will be so incredibly minor that you should never even think about it.

The only thing that matters here is what one you would rather be using.

The first option might be in one line but oneliners aren't necessarily better. It is harder to read and your approach also means that if you want to have false-true-false, you would have to retype everything.

The second option is very clear and each statement is self contained, I would go with this.

  • 11
    Any compiler with an optimizer worth its salt should generate equally-efficient code for either. – Blrfl Feb 23 '14 at 15:35
7

There's no difference in performance at all. They will compile to virtually identical IL1.

However, there is a difference in readability and code style. The first option may be more compact, but the second is clear and conforms to most .NET coding guidelines. I personally prefer the second.

1. I ran a simple test in LINQPad. The only difference between the two options was in how the false value was loaded on the stack between calls to set_Visible. The first option produced one ldc.i4.0 and two dup's while the second produced three ldc.i4.0's.

  • Yes, the first statement might not sit so comfortably for those C# programmers from a VB background... – Robbie Dee Feb 23 '14 at 15:44
  • 5
    Nothing to do with a VB background. I have a C background and I don't like it. – MetalMikester Feb 23 '14 at 15:58
  • @MetalMikester What are your views on the tertiary statement? – Robbie Dee Feb 23 '14 at 16:43
  • +1 Even if the first does perform better, I totally agree that the preferred style should be used. Such differences are negligible these days. – Robbie Dee Feb 23 '14 at 17:06
4

What about a more DRY version, where even the code visible =is not repeated over and over again?

 foreach(var btn in new[]{lnkbtn1, lnkbtn2, lnkbtn3})
     btn.visible=false;

IMHO that will scale better when the number of buttons increase, or the number of properties of each button which has to be initialized increases.

  • 1
    +1 Excellent answer. Then each button simply gets added without the rest of the cruft. – Robbie Dee Feb 24 '14 at 9:12
  • 1
    Maybe it's possible to turn the three buttons into an array in the first place. Or use data binding to set the property. – Sebastian Redl Mar 16 '14 at 16:12
0

I have to say I prefer the first option. It is crystal clear that all values are being set to false.

With the second option I'm having to read every line and wonder why it isn't all on one line following DRY principles due the redundant literal.

I'd find it hard to justify the second case on the grounds that the values might change at some point in the future. So much code cruft comes about because of something the programmer thought might happen but never did. This code then has to be tested and maintained. Not to mention being read by potentially dozens of programmers over the course of its life.

  • 7
    I don't think DRY should be taken this literally. If you did, then repeating = or .visible should also be considered a bad thing. – svick Feb 23 '14 at 16:04
  • Consider the case where 3 integer variables are all being set to the same large value. You wouldn't dream of writing statement 2 then. So why prefer this style for booleans? – Robbie Dee Feb 23 '14 at 16:39
  • 2
    Putting them as part of the same assignment adds a conceptual overhead of "these are the same thing". This might be justifiable if you are doing "left and right margins are the same". However, the visibility of three buttons (unless they all change together) isn't the same... just happens to be at the start. – user40980 Feb 23 '14 at 16:44
  • Are you saying this sort of thing should be reserved for objects rather than properties? – Robbie Dee Feb 23 '14 at 16:49
  • 1
    @svick: see my answer, repeating of = and visible can be easily avoided, so why not take DRY seriously? – Doc Brown Feb 23 '14 at 22:19
0

Just because somebody's a "team leader" doesn't mean much.

If .visible is nothing more than a simple boolean variable, the difference in cost is either zero or basically nothing.

However, if .visible is a property, all bets are off. Assigning to a property is one method call, and retrieving the value is another. In that case, the separate assignments might be faster, but watch out for another problem. Properties have some nice properties (!), but I've seen them treated as if they were free, performance-wise, when in fact they can be major sink-holes of CPU time if they are implemented recursively.

In terms of style, I take a dim view of giving this more than a nanosecond of thought. Nearly all serious software has far bigger issues than this.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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