views:

529

answers:

9

I have a simple little code fragment that is frustrating me:

HashSet<long> groupUIDs = new HashSet<long>();
groupUIDs.Add(uid)? unique++ : dupes++;

At compile time, it generates the error:

Only assignment, call, increment, decrement, and new object expressions can be used as a statement

HashSet.Add is documented to return a bool, so the ternary (?) operator should work, and this looks like a completely legitimate way to track the number of unique and duplicate items I add to a hash-set.

When I reformat it as a if-then-else, it works fine.

Can anyone explain the error, and if there is a way to do this as a simple ternary operator?

+4  A: 

The compiler isn't complaining about Add it is complaining about the fact that your conditional expression is not a complete statement.

Some languages (like JavaScript) allow you to use a conditional expression to branch logic like you have done here but C# requires that you assign the result of the conditional expression to a variable. Once you assign the result of the expression, you have made a complete statement and the compiler is happy.

Andrew Hare
Actually, it's complaining about the ternary operator.
Mehrdad Afshari
@Andrew Wouldn't those be considered an increment though, which is mentioned in the error message as a valid statement?
AaronLS
@Merhdad - Yes, you are right - I am fixing it now.
Andrew Hare
@abelenky: There's nothing wrong with using the `++` operator here. The problem is that the ternary expression itself isn't a statement: Something like `groupUIDs.Add(uid) ? 0 : 1;` won't work either.
LukeH
+6  A: 

You not setting the value result of the ternary to anything that's why.

HashSet<long> groupUIDs = new HashSet<long>();
int count = groupUIDs.Add(uid)? unique++ : dupes++;
gmcalab
+5  A: 

The ternary operator is not a statement. So, it cannot be used alone in an instruction - it's the equivalent of writing

"something that is not a statement";

To clarify, you should take out the ternary operator and use an if.

ANeves
In C and C++ at least, that is a perfectly valid (although without side-effects) statement. Apparently this isn't true in C#. Turns out the right answer is to have a dummy-assignment as @sth suggested.
abelenky
This is a great explanation, because if you think about how a ternary works, it evaluates to a single value, the value which is chosen from the two options. So once evaluated it is just like writing the statement that is the value of `unique;` (after being incremented), or `dupes;` which would be something like `12345;` which wouldn't be a valid statement since it is just a lone integer.
AaronLS
@AaronLS: First, it would be the value of unique PRIOR to incrementing, not after. Secondly, in many languages, having a lone-integer is perfectly valid. C# is different in this regard.
abelenky
By the way, I had absolutely no idea lone-integers could be statements in other languages - and I know a few. One keeps learning. :)
ANeves
@abelenky Good call on the order of operation, I thought about the postfix but was really focusing on the fact that it evaluates to an integer. We could explore all the different ways different languages would handle it, for example "12345" is not a complete statement in English, but since the question is tagged as C# I think it would be productive to focus on that ;)
AaronLS
+16  A: 

According to the error message the ternary operator cannot be used as a statement. You would need to do something like this to turn it into an assignment:

int dummy = groupUIDs.Add(uid)? unique++ : dupes++;

That being said, I'd recommend to just use if-then-else. It's less confusing because it doesn't involve the creation of "magic" dummy variables...

sth
This seems to fix it. In C, there is nothing wrong with a standalone ternary that does nothing. Apparently this is not true in C#. Thanks.
abelenky
Or use an if instead of the ternary operator, instead of assigning a value to a useless variable for the sake of code coolness and detriment of readability.
ANeves
a fully formatted if-statement takes 8 lines with an additional indentation level. That seems like a huge waste for such a simple operation. I'm extremely comfortable with ternary as a C/C++ programmer, and don't see any cool-factor or that it hurts readability.
abelenky
I prefer opening brackets in the same line, but you're absolutely right about line count. By all means use a ternary op and a dummy, if that's your preference - as long as it's a choice out of knowledge, it's a fine choice. :)
ANeves
@abelenky - I definitely see why you didn't assign it to anything, but in C# you have to, I wish you didn't though, kinda of a waste. I had this same issue a few years ago and it really confused me... I wouldn't consider it a dummy mistake or anything.
gmcalab
`if(groupsUIDs.Add(uid)) { unique++; } else { dupes++; }` is only 15 more characters.
AaronLS
A: 

If this is not acceptable, why would your line be? Just use an if statement :-)

        bool b = false;
        b?callB():callA();
Dested
+1  A: 

gmcalab and sr pt are right; the ternary operator is meant to give you a result, just like 1 + 1 gives you 2. You could not just write:

1 + 1;

The confusion here (I think) is that you're thinking of the ternary operator like it's a function.

Dan Tao
+2  A: 

You need to use the value from the ternary operator for something...

HashSet<long> groupUIDs = new HashSet<long>();
int newCount = groupUIDs.Add(uid)? unique++ : dupes++;

or - use an if

HashSet<long> groupUIDs = new HashSet<long>();
if (groupUIDs.Add(uid))
   unique++;
else
   dupes++;
daddyman
+1  A: 

The description of the ternary operator in the language reference says that

If condition is true, first expression is evaluated and becomes the result; if false, the second expression is evaluated and becomes the result.

It looks like the ternary can only be used in the context of assignment, although the language reference doesn't state it explicity. You're not doing an assignment on the result.

In my opinion, the re-writing as an if/else would be clearer.

Andy Johnson
It doesn't have to be an assignment - you could pass it to another method, for example.
Jon Skeet
You're right - I was in a hurry and my wording was too loose.
Andy Johnson
+12  A: 

As others have pointed out, the conditional operator is not a legal statement expression. (The legal statement expressions are assignments, calls, increments, decrements and constructions.)

However, there's a stylistic problem here as well. In my opinion, expressions should be useful for their values, and statements should be useful for their side effects. What you are running into is that you have an expression that is only useful for its side effect, and that is a bad code smell.

You have a side effect, so use a conditional statement rather than a conditional expression.

Eric Lippert
Out of respect to @EricLippert, I'll change it to a conditional statement, if-else. Another step up the learning curve from C to C#.
abelenky
Aren't assignment and compound assignment operators also statement expressions?
Gabe
@abelenky: I am flattered, but please, don't do it for my sake. Do it for the sake of the future people who must maintain your code. :-)
Eric Lippert