Skip to main content
added 586 characters in body
Source Link
John Wu
  • 26.9k
  • 10
  • 68
  • 92

Technically it's OK

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.

Why it is avoided

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.

This is even worse

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.

What might be OK

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.

The bottom lineI have seen this construct all over the place. Because I am familiar with it, it is clear enough to me, youand so far hasn't caused me any confusion when I come across it.

Bottom line

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.

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.

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.

The bottom line is, 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.

Technically it's OK

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.

Why it is avoided

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.

This is even worse

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.

What might be OK

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.

Bottom line

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.

Source Link
John Wu
  • 26.9k
  • 10
  • 68
  • 92

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.

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.

The bottom line is, 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.