current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 4 down vote favorite

I have two List<T>s of the same T and a T object. Based on a condition, I want to add the object to one of the two lists. So I did:

(condition ? listOne : listTwo).add(item);

To me, this is great, because it cuts a 5-line snippet:

if (condition) {
    listOne.add(item);
} else {
    listTwo.add(item);
}

...or a (less readable, IMO) 2-line snippet:

if (condition) listOne.add(item);
else listTwo.add(item);

...to one line. But is it bad practice?

share|improve this question
add comment

3 Answers

up vote 2 down vote accepted

It's not bad practice. It's all about readability. Personally, as an advanced Java developer, ternary operators are part of the fluency of the language, and I find your first version much easier to read and absorb at a glance. But for those who are beginner or intermediate developers, it may be much more cryptic.

I don't usually recommend dumbing things down if it's actually done right since it will force people to learn and become better programmers, but in this case you might want to consider your "target audience" (your other developers who will have to maintain the code, if any). If they're beginners, option 2 is the best. For intermediate, option 3 is fine. For advanced, option 1 is great.

share|improve this answer
add comment
up vote 7 down vote

I think 5 line version is the best option. It is most readable of these three and it is very easy to add third list as oppose to the ternary version. These 3 versions will most likely produce same byte code, so you should go with version which is nice to read and quick to understand for your fellow programmers.

share|improve this answer
add comment
up vote 3 down vote

Your first two examples (1-line, 5-lines) are perfectly Ok. The 5-line solution is easy to read, and immediately obvious.

The problem with the 1-line solution is that this usage of the conditional is absolutely unexpected. At first glance, it seems that you are using the ternary on a statement level, which can't be recommended. I feel that the only good place for that operator is on the RHS of an assignment (control-flow vs. data-flow). This would open up another good possibility:

List<T> interestingList = condition ? listOne : listTwo;
interestingList.add(item);

This is less cryptic than your two short solutions, and has significantly less repeated code than the if/else-solutions.

share|improve this answer
add comment

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.