7

I'm working on a VB.Net WinForms project and found myself writing code like this:

this.Fizz.Enabled = this.Buzz.Enabled = someCondition;

I couldn't decide whether that was bad code or not. Are there any .NET guidelines for when/when-not to do assignment chaining?

Jeff B
  • 838
  • 2
  • 9
  • 14
  • 1
    Related: http://stackoverflow.com/questions/5590392/is-c-sharp-a-b-c-equal-to-b-c-a-c-whereas-c-is-b-c-a-b – Job May 08 '13 at 22:20
  • Specifically in the context of Winforms - if you have just two buttons/controls, then write out two lines. If you have a bunch of them, then perhaps save the logical groups of them in a set, and then apply an attribute change to the whole set with aid of a helper function. Still, as Robert said, try to minimize state! Frankly, try to simplify the UI first. You can also logically separate controls by groupboxes, or give them a specific Tag, and then can change all controls in a given GroupBox with tag = "foo" to have `.Enabled = someCondition`. Speed will matter less than clarity. – Job May 08 '13 at 22:25

3 Answers3

25

I never do it. I always put each assignment on its own line. Clarity is king.

In practice, I seldom use enough state variables to make chaining assignments necessary. If I get to that point, I start looking for ways to trim the amount of state I am using.

Robert Harvey
  • 198,589
  • 55
  • 464
  • 673
  • 2
    +1 having enough state hanging around that you can't spare an extra line in your method is a sign you're juggling too much in a single method. Time to break it into multiple methods. – Jimmy Hoffa May 08 '13 at 22:59
6

It depends on the programming language. If you use a language where variable types are not enforced, then this could happen.

x = 1;
y = 494.0;
z = new Object();
x = y = z = "hello";

Now x,y,z are all a string. This might be confusing if found later on in the code.

The other problem is operator overloads. Some languages allow you to change what happens for the left-hand assignment of a property. So the following could happen.

x = y.something = 0;

Where y.something doesn't return 0. It returns null.

Otherwise, I don't have a problem with it. If it's a strongly typed language and there is no risk in the above happening. Then no problem.

The other problem is in how you read the source code.

 a = b = c = new Object();

Is not the same as.

 a = new Object();
 b = new Object();
 c = new Object();

It's really like this.

 c = new Object();
 b = c;
 a = b;

It might not be clear that all three variables have the same reference.

Robert Harvey
  • 198,589
  • 55
  • 464
  • 673
Reactgular
  • 13,040
  • 4
  • 48
  • 81
  • 8
    So, other than those catastrophic problems... :) – Robert Harvey May 08 '13 at 22:18
  • Two of his mentioned problems only show up in OO languages and only when chaining assignments with objects. One of his mentioned problems only shows up in dynamically typed languages. That doesn't seem catastrophic to me. – Michael Shaw May 09 '13 at 00:02
  • 1
    @MichaelShaw: The problems are so easily avoided, though. – Robert Harvey May 09 '13 at 14:41
  • The question was if it was bad code, and when to do it and not to do it. Answer: it's not *bad* code but there are many cases where you should not do it. I'll try to think up some examples of when it's good to use it, but I've been thinking about that for a while now and can't think of one. lol – Reactgular May 09 '13 at 15:19
  • @RobertHarvey: They're so easily avoided that they don't exist in many languages and you'd have to be doing something goofy in languages where it might cause problems. – Michael Shaw May 09 '13 at 16:03
  • 1
    @MichaelShaw: This is basically in the same category as the "best practice" of always putting braces in `if` `then` statements, even when you don't have to. I have strong feelings that this practice should not be necessary, and yet our shop was bitten by this recently in a very visible way, where a bug crept into a system that would never have occurred had the braces simply been added. Putting each of your declarations on its own line is such a small price to pay to completely eliminate all of the problems described here. – Robert Harvey May 09 '13 at 16:09
  • 1
    @MichaelShaw: Putting multiple declarations on the same line also adds cognitive friction for the person trying to read your code, who now has to evaluate what the line is actually doing, and if it contains any of these problems. It's just not worth it. – Robert Harvey May 09 '13 at 16:12
  • @RobertHarvey not putting braces in source code is the same thing as using bad grammar in an email. It might convey what you mean when a human reads it, but don't be surprised when the computer interprets it differently using proper grammar. – Reactgular May 09 '13 at 16:15
  • @RobertHarvey: How does it add cognitive friction? What do you have to evaluate that you wouldn't have to evaluate if the same thing were written on multiple lines? The only problem that I see that might be hard to spot is the dynamic typing one, and it would be just as bad if split up in individual lines. – Michael Shaw May 09 '13 at 16:50
3

It's not necessarily bad code, but it does make your code significantly less readable. Typically, I would strongly recommend against it, although I suppose there are a few cases where it might be acceptable (or merely less bad). For example, when setting a few local variables to constant value:

// accumulators
int j = 0, k = 0;
for(int i = 0; ...)
{
    ...
    if(reset)
    {
        // oops we have to start from the beginning
        i = j = k = 0;
    }
}

In this case the line, and more importantly the comment that accompanies it, make it clear what the authors intention was when writing it. This code also contains a compound declaration which I also generally discourage, but are more acceptable here for the same reasons.

Still, I would recommend against it in most cases. There is never any harm in writing more explicit code (as long as it's functionally identical).

p.s.w.g
  • 4,135
  • 4
  • 28
  • 40
  • 2
    It seems more readable to me. If you have separate assignments to the same value, verifying that they all get set to that value takes a little extra time compared to chaining them. – Michael Shaw May 08 '13 at 23:58
  • @MichaelShaw Sometimes, that's true--and that is kind of the point I was trying to make: the choice to use chained assignments should be driven first and foremost by readability. – p.s.w.g May 09 '13 at 00:04
  • Having small functions, one could also throw an exception when something bad happens and have the caller decide whether the function should be called again or not - makes it easier to do a limited number of retries. – Job May 09 '13 at 03:12
  • 1
    @Job Perhaps my `somethingBadHappened` was a bit confusing. The code I wrote was just an example of some hypothetical number crunching where this sort of thing would be more acceptable. If this were higher-level code that needed to handle exceptions, I would structure it much differently. – p.s.w.g May 09 '13 at 14:56