returning an assignment expression

Question

While I was working on some of older code in C#, I encountered a code which irked me.

Without much ado, it goes something like this:

private string foo(string _text)
{
   /* some manipulation on _text */
   return _text = Server.HtmlDecode(_text);
}

It's the last line which irks me; I'm from C background, and I can understand that the code is in effect trying to return a decoded _text variable. Also the value of an assignment operator is the left operand, so I can see it.

Yet I still find it irksome.

Is it a ordinate practice in C# that I need to get accustomed to?

To me the last line should simply be

return Server.HtmlDecode(_text);

and not be an assignment expression. Is there a deeper C# feature which I'm not aware of?

Answer 1

While I was working on some of older code in C#, I encountered a code which irked me.

There are numerous irksome problems here. Let's list them all.

private string foo(string _text)
{
   /* some manipulation on _text */
   return _text = Server.HtmlDecode(_text);
}

It's the last line which irks me

The comment is also irksome. Local variables are cheap. There is no need to erase the original value of _text. Instead, make a new local variable and manipulate that. That way, when you are debugging at any point in the method you can know what the original argument was. (Remember, the original argument might be eligible for garbage collection the moment the variable is overwritten, and therefore could be lost forever.)

Do not write over a formal parameter without a very good reason. It makes it harder to debug.

the value of an assignment operator is the left operand, so I can see it.

That is correct in this case but subtly wrong in general; the value of an assignment operator in C# is the value of the right operand after being converted to the type associated with the left hand side. Remember, the left hand side might not have a value; it could be a write-only property.

Is it a ordinate practice in C# that I need to get accustomed to?

There is a standard practice here, yes. What is bizarre about this usage is (1) that the variable chosen is a formal, and (2) that the assignment is combined with the return.

It would be a standard practice in C# to say:

string decoded = Server.HtmlDecode(_text);
return decoded;

Now you might wonder what the compelling benefit of this is over what you suggest:

return Server.HtmlDecode(_text);

The answer is: before Visual Studio 2013, there was no facility in the debugger to examine the returned value of a method call! Therefore if you wanted to see what the value returned by HtmlDecode was while debugging you had the following choices:

• Debug at the assembly level and look at the contents of EAX
• Step into HtmlDecode and examine its state
• Step out of the current method and examine whatever the return value was assigned to
• Assign the result to an otherwise useless local variable and then examine the local in the debugger

Since the first three are horrid and the last one is easy, that's what many C# programmers got in the habit of doing.

If you do this and then do not use the resulting local, the C# compiler knows that this is a common practice and deliberately suppresses the "you wrote to a local you then never read from" warning. It only gives that warning if the local had a constant written to it, in which case you already knew what it was at compile time and don't typically need to examine it in the debugger.

Hopefully now that VS2013 finally supports this frequently-requested feature, this sort of pattern will gradually disappear.

Answer 2

This statement is redundant and not a C# practice.

Doing so while ReSharper is active will also give the warning

Value assigned is not used in any execution path

As you mentioned, this code would indeed be the best practice

return Server.HtmlDecode(_text);

Also, since the decode is part of the manipulations on _text, it would also be valid to separate the assignation and return statement, to keep the logic in the same block :

/* Other manipulations on _text */
_text = Server.HtmlDecode(_text);

return _text;