Is it OK to use assignments in expressions?

Question

I came across this code and wanted others to provide their point of view... is it good or bad ? ;)

Class ReportClass
{
 public string ReportName {get; set;}
}

Then it was used as follows in code:

displayReport(ReportClass.ReportName = cmbReportName.SelectedValue.ToString())

That is about the simplest form example I can give you. Quetion is... why can't I find examples ? What would this be called? Is this just asking for trouble?

EDIT: I'm referring to the inplace assignment. Which I wasn't aware of until today

Answer 1

Seems fine to me. It is probably compiled for .NET 3.0 and that allows C# automatic properties.

Answer 2

What you are seeing is the new Automatic Properties in .Net.

See this post for some information:

Personally, I really like to use them.

Answer 3

Did you mean the automatic properties or the in-place assignment?

Depends on your team IMO. If your team were comprised of C-style devs. Then I think it is ok.

But if this code is gonna be maintained further by, say, VB developers, then you'd need to make this more readable.

Examples? The assignment operator (=) in C langauges also return the values it's assigned.

var a = 0;
var b = 0;

// This makes a *and* b equals to 1
a = b = 1; 

// This line prints 3 and a is now equals to 3
Console.WriteLine(a = 3);

// This line prints 7 and a and b is now equals to 7
Console.WriteLine(a = b = 7);

I think it is rather natural for this kind of assignment to happen. Because the C-languages seem to encourage shorthand notations IMO.

If you need more readability and less trouble, then I'd say a newline is a nice add.

displayReport(
    ReportClass.ReportName = cmbReportName.SelectedValue.ToString());

Make your intentions clearer when each compounded statements are on different lines. (but still a compound statement)

Or, extract the right part out into its own variable first, e.g.

var reportName = cmbReportName.SelectedValue.ToString();

displayReport(ReportClass.ReportName = reportName);

I use it all the time, and I havn't seen anyone confused upon reading it yet.

Answer 4

I think this is harder to maintain/harder to debug/harder to understand code. I would do the assignment on a line prior to calling that method.

ReportClass.ReportName = cmbReportName.SelectedValue.ToString();
displayReport(ReportClass.ReportName);

I've never really been a fan of compound statements.

Answer 5

It is totally fine. Both of them.

• Automatic properties (the {get; set;} thing): Introduced in C#. Very useful feature.

• Assignment in the parameters: Rarely used in C# but totally legal. Basically it assigns the value of the expression to the right of the assignment operator (=) to the property to the porperty on the left, and then passes this value on to the method.

  This is more common in C, but I see no reason why shouldn't it be allowed in C# as well. Sometimes it might help readability, sometimes it makes it much worse. Use common sense to decide which applies when.

Answer 6

Is that really valid code? Seems like a static class to me.

I would have guessed you used it like this:

displayReport(new ReportClass { ReportName = cmbReportName.SelectedValue.ToString() } )

Answer 7

I tend to avoid in-place assignment - or indeed any side effects like this - except for one common idiom:

string line;
while ((line = reader.ReadLine()) != null)
{
    // Do something with line    
}

(And variants for reading streams etc.)

I'm also okay with using object initializers in parameter calls, e.g.

Foo(new Bar { X=1, Y=2 });

but assigning to a property in an existing object... well, it's all subjective but it's not my cup of tea.

Answer 8

The Microsoft Framework Design Guidelines discourage the placement of multiple statements on one line, except where there is direct language support, such as in the for statement. Since there is explicit language support for multiple assignment,

int a, b, c;
a = b = c = 0;

is permitted. On the other hand, code of the form used in your example is discouraged.

How about this one, people?

while ((packetPos = Packet.FindStart(buffer, nextUnconsideredPos)) > -1)

To avoid the inline assignment, you would have to factor redundantly, like this:

packetPosition = Packet.FindStart(buffer, nextUnconsideredPosition);
while (packetPosition > -1)
{
  ...
  packetPosition = Packet.FindStart(buffer, nextUnconsideredPosition);
}

Answer 9

Personally I find the assignment as parameter not very readable. The overall flow of execution is just skewed by having an operation actually happening inside the parameter list.

I like to think that my code should express intent, not save whitespace. And my intent in this example would be exactly what EvilSyn suggests, first assign value, then pass it to the method.

Answer 10

As far as naming the property goes you have a ReportClass, I would change that to Report and the property on it changed from ReportName to just Name. Saves you on typing (although yes intellisense is available). Looks better when coding up as Report.Name.

I know its a little off topic but hey ho