Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is a ternary expression ok in this case:

cobBereich.DropDownStyle = (cobBereich.DropDownStyle == ComboBoxStyle.DropDownList ? ComboBoxStyle.DropDown : ComboBoxStyle.DropDownList);

Is it overkill?

Would an if-else be easier to understand or to maintain for other coders?

share|improve this question
3  
The better question is why is your code using logic that says: If style is drop down list then set it equal to drop down otherwise set it equal to drop down list. –  Ryan Gates Jan 29 '13 at 17:25
    
It´s something like a professional-mode at a form... if you are a normal user you should just select one of the given 'strings', and if you are a experienced user you can "unlock" that 'drop down' so you can select one of those or enter a own 'string'. –  Vloxxity Jan 30 '13 at 7:40

4 Answers 4

up vote 17 down vote accepted

It's a question of personal preference. I would personally prefer the following formatting:

cobBereich.DropDownStyle = (cobBereich.DropDownStyle == ComboBoxStyle.DropDownList) 
    ? ComboBoxStyle.DropDown 
    : ComboBoxStyle.DropDownList;
share|improve this answer
5  
for humans, ternary ops are great (unless nested too much), but long lines are awful. –  Michael Paulukonis Jan 29 '13 at 17:12

From you logic I am assuming you have two possible values and you want to switch the current value to the other one?

If so, I must admit that it is not clear immediately this is what you are trying to do. For this reason, I would personally prefer to use an if else statement which would be easier to read/follow:

//switch the value
if(cobBereich.DropDownStyle == ComboBoxStyle.DropDownList)
    cobBereich.DropDownStyle = ComboBoxStyle.DropDown;
else
    cobBereich.DropDownStyle = ComboBoxStyle.DropDownList;

Of course, the code you have already is perfectly valid. So I would say go with what you find easier to read.

share|improve this answer
3  
eeek! no curly-braces! Yes, they are not required for single lines, but THINK OF THE CHILDREN! More literally, think of future maintenance when one other condition is added. (for that reason alone, the ternary op is a better solution). –  Michael Paulukonis Jan 29 '13 at 17:14
    
@MichaelPaulukonis: To be honest, I would normally use the curlies anyway. It just seemed more clear to exclude them in this example. Normally I only ever exclude them when I use a single if, and it is a short bit of code, like a return condition at the top of a function –  musefan Jan 29 '13 at 17:18

Expanding on @almaz's answer, I have started formatting my code as the following:

cobBereich.DropDownStyle = 
    (cobBereich.DropDownStyle == ComboBoxStyle.DropDownList)
        ? ComboBoxStyle.DropDown 
        : ComboBoxStyle.DropDownList;

I find that putting the conditional expression on a new line helps keep it from being swallowed up in a single long line, and keeps the position consistent, independent of variable name lengths.

share|improve this answer

No, this is nowhere near overkill. This is exactly the case that the conditional operator was designed for. Looks A-OK.

EDIT: now that I see almaz's answer, I agree wholeheartedly that formatting is key to making the conditional operator maintainable.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.