2

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?

7
  • 2
    Never mind code aesthetics - the change changes semantics drastically. Was that intended? If not, this is much worse than a "smell". Aug 13, 2021 at 17:20
  • @KilianFoth I take 'it works' to mean it is intentional but it would be preferable to see equivalent conditions in both forms. My guess is that the required change was to add more status codes to the condition.
    – JimmyJames
    Aug 13, 2021 at 17:47
  • @KilianFoth Check my edit
    – Kevin
    Aug 13, 2021 at 17:58
  • 2
    after 15 years as a C# developer, but I've never seen this used before What? Really? I’ve seen this construct many times
    – John Wu
    Aug 14, 2021 at 2:00
  • 1
    See this question for a similar discussion. Aug 14, 2021 at 3:08

4 Answers 4

8

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.

12
  • 4
    On top of what you said, one benefit of switch blocks is that you can enable compiler warnings for non-exhaustive cases, which you can't easily do with if blocks. This can be helpful to ensure that new values added to an Enum are handled. Also, boring technical detail, but Java has the EnumSet type which you can use for this, i.e. Set<ItemStatus> notReadyStatuses = EnumSet.of(ItemStatus.Preparing, ...). Aug 13, 2021 at 18:53
  • 1
    See also CWE repository, which flags this practice as a weakness for C, C++, Java, C# and PHP. Several secure coding standard forbid this (e.g. CEI ), and static analyzers therefore flag this as a code smell.
    – Christophe
    Aug 13, 2021 at 21:41
  • 1
    The original question has a switch with five fall throughs.
    – gnasher729
    Aug 14, 2021 at 12:57
  • 3
    "I think it's valid to forbid all fall-through logic in switch statements as part of coding standard for this reason." I would suggest that you'd only forbid fall-through after a non-empty case body. With an empty case body, it's very apparent that case Foo: case Bar: Console.WriteLine("Baz"); share the same execution. However, for case Foo: Console.WriteLine("Baz"); case Bar: Console.WriteLine("Bat");, it becomes harder to catch, which is where I agree with you.
    – Flater
    Aug 17, 2021 at 14:05
  • 1
    @Christophe not only modern languages. Ada‘s ‚case‘-statement does the job since 1983. Aug 17, 2021 at 18:15
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.

4
  • Don't think that would work. If you've got two items in an order and the first is out of stock and the second is available, you're going to end up with the wrong order statues
    – Kevin
    Aug 17, 2021 at 12:41
  • @kevin This is the first I’m hearing of two items. How exactly does this code of yours deal with two items: switch (itemStatus)? Aug 17, 2021 at 12:46
  • This is code is called for each item in the order
    – Kevin
    Aug 17, 2021 at 12:50
  • @kevin then I'm guessing you want something like an accumulating bool that will allow any one item to put an order into a NotReady status. See edit. Aug 17, 2021 at 14:01
0

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.

-1

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.

4
  • Those are actually strings, hence the .equals
    – Kevin
    Aug 13, 2021 at 19:50
  • @Kevin That might be something to give feedback on. From what I understand Enums in Java are quite a bit different than in C#. They are typesafe, for one. I think they can be overused but this looks like a situation where they make sense.
    – JimmyJames
    Aug 13, 2021 at 20:16
  • Yeah. I agree. Unfortunately, it being a string is pretty deeply rooted in the code from long before this change (and before my time at this company) so refactoring it would be a pretty large undertaking
    – Kevin
    Aug 13, 2021 at 21:01
  • @Kevin And that's the problem with 'primitive obsession', isn't it. It's hard to refactor your way out of that tarpit.
    – JimmyJames
    Aug 17, 2021 at 14:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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