Using a switch statement in place of multiple ORs in an if statement

Question

I was doing a code review today and came across a change that, while it works, "smells" to me.

Original code:

if(itemStatus.equals(ItemStatus.Preparing)){
    orderStatus = OrderStatus.NotReady;
}

and the modified code looks like this:

switch (itemStatus)
{
    case ItemStatus.Preparing:
    case ItemStatus.Ordered:
    case ItemStatus.Voiding:
    case ItemStatus.Delayed:
    case ItemStatus.OutOfStock:
    orderStatus = OrderStatus.NotReady;
}

Which is being used in place of:

if(itemStatus.equals(ItemStatus.Preparing)
   || itemStatus.equals(ItemStatus.Ordered)
   || itemStatus.equals(ItemStatus.Voiding)
   || itemStatus.equals(ItemStatus.Delayed)
   || itemStatus.equals(ItemStatus.OutOfStock)){
    orderStatus = OrderStatus.NotReady;
}

The code in question is in java but this could apply to several languages. I'm fairly new to Java after 15 years as a C# developer, but I've never seen this used before. Is this normal in Java? If not, is it bad practice?

Answer 1

Fall-through in switch statements is common feature of C-family languages. I'm not sure you'll get a general consensus on whether it's a good feature. The problem I find with it is that it tends to be easy to forget a break statement when coding and not notice it is missing when reading code. I think it can be a little hard to follow code like this:

switch (cond)
{
    case a:
       stepX();
    case b:
       stepY();
       stepZ();
       break;
    case c:
       stepP();
    default:
       stepR();
}

It's easy to miss the break and then you might wonder if the fall-through was intentional or the break was omitted inadvertently. I think it's valid to forbid all fall-through logic in switch statements as part of coding standard for this reason.

On the other hand, the particular case you show, IMO, is probably the least egregious way to use this feature. There's no real confusion and if someone doesn't understand it, they'll have learned something once they do the research and/or ask about it. But I think it's totally reasonable to avoid subjectivity in your standards and say no.

I think it comes down to how your team dynamic works. If you have people who want clear-cur rules, you might want to just say fall-through is either fine to use or not. If there's tolerance for more nuance in code reviews, maybe you judge each usage independently. If everyone else on the team is comfortable with fall-through and like it, maybe you bless any use of it. The worst situation will be if fall-through is only used occasionally and you have team members who don't grok it.

On a side note, there's another way to accomplish this logic that might be worth considering. Something along these lines:

Set<ItemStatus> notReadyStatuses = EnumSet.of(
   ItemStatus.Preparing,
   ItemStatus.Ordered,
   ItemStatus.Voiding,
   ItemStatus.Delayed,
   ItemStatus.OutOfStock);

Then the condition becomes:

if (notReadyStatuses.contains(itemStatus)) {
    orderStatus = OrderStatus.NotReady;
}

Thanks to Vincent Savard for the recommendation on using EnumSet.

Answer 2

bool orderReadyStatus = true;

for (Item item: items) {
    orderReadyStatus &= item.orderStatus();
}

This works just fine. It doesn’t force you to create a place in the code that knows every kind of item or all reasons order status takes on one state or another.

Sure, there’s a bit of work putting the logic in the different item classes but now each item type can be hidden away where you don’t have to keep thinking about them. That’s a good thing.

Answer 3

Judging from the code this looks like ItemStatus is not an enum, since typically in java you would then be able to leave out the enum class and just write case Preparing: and so on.

Using an enum with a switch is good, since most IDEs can give you a warning if you don't actually cover all enum values, which helps if you want to avoid bugs coming from adding a state but forgetting to handle it everywhere. So this pattern is great for that.

Also, even absent enums, I think this pattern (of using switch-case is superior to repeated || for readability, which I consider a paramount concern.

I also note that || does not actually add anything else either in terms of performance or succinctness.

Consider why java added switching over strings: it was precisely to avoid the equals chains and allow strings to be more cleanly compared using switch-case. So this commonly recognized as better than equality.

Answer 4

First, in Java, it is recommended to use == for enum comparsion.

if (itemStatus == ItemStatus.Preparing
        || itemStatus == ItemStatus.Ordered
        || itemStatus == ItemStatus.Delayed
        || itemStatus == ItemStatus.Voiding
        || itemStatus == ItemStatus.OutOfStock){
    orderStatus = OrderStatus.NotReady;
    }

Second, I believe in this case the choice depends more on the context. If that code can grow in the future where other cases can be handled differently then the switch is the right choice. If it is only for this case, the if statement is preferable.

However, if efficiency is a concern here, the switch code is a bit more efficient since the switch uses a lookup table.