I must set a bool to be passed down from a controller to a view in my MVC solution as well as do something (as part of an if). I thought, in order to keep the code concise, I could kill two birds with one stone here:
if (model.MyBoolean = ([Some condition])) { // Do something }
The language permits it, but is this in any way bad practice? Does it compromise readability?
There is nothing wrong with using = symbol for both assignment and as an operator (that returns the value of the assignment), and it is certainly legal, and concise. The question is whether it is clear, and that will depend on who is reading it.
In my experience, most developers avoid single = in an if statement because it is way to easy to make a mistake, and the mistake is often hard to detect and has a very negative effect. In fact Visual Studio will warn you just because it is such a common problem. I think that is a pretty strong sign you should avoid it.
What's really bad is when the operator is used further along in a short-circuited expression, like this:
if (condition1 | model.MyBoolean = condition2) {}
The above will work, but this will not:
if (condition1 || model.MyBoolean = condition2) {}
If condition1 is false, the assignment won't happen!!! This might not be obvious to someone who is editing the code, and can easily introduce catastrophic logic failure.
On the other hand, using single = like this is not uncommon in certain other constructs, such as in a while loop:
while (bytesRead = streamReader.Read(buffer, x, y) > 0)
{
//Do something with the buffer
}
The above loop will read bytes from a stream and record the length of the bytes read, and will exit the loop if bytesRead is zero.
I have seen this construct all over the place. Because I am familiar with it, it is clear enough to me, and so far hasn't caused me any confusion when I come across it.
You have to decide for yourself whether your code construct is clear. In my opinion (and Microsoft's), it is not, and I wouldn't do it, but perhaps your team is different. Then again it is probably best to err on the side of caution.
I get that you want to be concise. I think most of the posters on this forum think conciseness is far, far less important than clarity. Also, here is a blog post on writing "Really Obvious Code" and why it is a good idea. Your code isn't really obvious.
While the language allows it, that isn't something that (at least in my experience) gets used a lot. I doubt too many people would get thrown by it, but it will make them have to stop and read more carefully. And in my opinion, I should be able to read the code without having to spend so much mental effort on things like that.
On top of that, if statements aren't a usual place for assignments, so people won't be looking for that to happen there. And for the keen observer, they may think it was a mistake and try to "correct" what is probably a bug.
I would go for something like this:
model.MyBoolean = [condition];
if (model.MyBoolean) { ... }
Because it's very easy to misread = for ==, especially if you're expecting the latter. It's the kind of thing that I'd read, then some time later realize something is amiss, go back to the if and go "Oh, right, that's where it was set".
Also, if you ever need to modify this condition later to something slightly more complex, it's a grand source of bugs - assignment in conditions doesn't play very well with short-circuiting operators!
It looks like you are testing the result of assigning a value to model.MyBoolean. I find this a bit weird to read. Myself, I prefer to read code such as
model.MyBoolean = ([Some Condition]) ;
if (model.MyBoolean)
{
/* do stuff */
}
It might not read as "clever", but I think it's clearer.
It is far more important that code is clear than that it is concise.
I don't see it as clear.
model.MyBoolean = ([some condition]);
if (model.MyBoolean) { // do something}
is clear. It is far less a risk that someone reads it as == in stead of =.
Secondly you have to rely on an assignment always return the assigned value, not only in the current language but also if you port to another language. You might have a language where an assignment always returns true.
I always turn on all reasonable warnings in a compiler, and turn warnings into errors, so my code will be free of warnings.
"All reasonable warnings" with one compiler that I use will warn for "if (x = y)" because it might be an unwanted assignment instead of a comparison. It will not warn for "if ((x = y))" - the unusual extra parentheses tell the compiler that this code isn't written that way by accident but intentional, so no warning.
On the other hand, if you want to write this in one line, you can always write "if ((x = y) != 0)" or "if ((x = y) == true)".
if (model.MyBoolean == [some condition]) { }if (12 == variable)to avoid the headaches this style will cause.