1
\$\begingroup\$

I created this short method:

private Item createItemType(final Integer type) {
    if(type == null) {
        return null;
    }else if (type == 0) {
        return Item.PLACE;
    } else if (type == 7) {
        return Item.ADDRESS;
    }
    return null;
}

Logically it is correct, but I want to know if there is a better way/elegant way to write this piece of code ?

\$\endgroup\$
3
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Jul 14, 2021 at 20:20
  • 1
    \$\begingroup\$ Do you really need the Integer type? Do you think that int would work? If this is the case, it will remove the validation of the null... because int can't be null \$\endgroup\$ Jul 15, 2021 at 12:17
  • \$\begingroup\$ this question is very opinion based - you will not get useful input (beside the most obvious switch-case) to your question if you do not provide more content. --> What is type? why is type an Integer? how many types do you have? two (0,7)? eight (0..7)? whats the expected behavior in case you don't find a type? \$\endgroup\$ Jul 16, 2021 at 9:57

4 Answers 4

7
\$\begingroup\$

In this case, i would use the inline if. This would take less line and would be cleaner overall.

private Item createItemType(final Integer type) {
    return type == null ? null : 
        type == 0 ? Item.PLACE : 
        type == 7 ? Item.ADDRESS :
        null;
}
\$\endgroup\$
3
  • 3
    \$\begingroup\$ Bad Idea: you moved the null check to the end (or rather removed it), which will lead to a null pointer exception when auto-unboxing the Integer for int comparison. \$\endgroup\$
    – mtj
    Jul 15, 2021 at 5:13
  • \$\begingroup\$ No difference. in fact see the two nulls use a switch instead... \$\endgroup\$
    – D. Sikilai
    Jul 16, 2021 at 16:20
  • 1
    \$\begingroup\$ Nesting/chaining ternary operators is, in my opinion, ugly and a potential maintenance headache. In this case, it's not totally abhorrent, but if anyone in my team asked me to approve this code I'd refuse and ask for it to be recoded. \$\endgroup\$ Jul 20, 2021 at 12:07
11
\$\begingroup\$

switch

I would find a switch cleaner.

private static Item createItemType(final Integer type) {
    if (type != null) {
        switch (type) {
            case 0:
                return Item.PLACE;
            case 7:
                return Item.ADDRESS;
        }
    }

    return null;
}

If you really want to reduce the number of lines, you can combine the return lines with the case labels. But I find that to reduce both readability and editability.

This is also better than the inline if in the face of future changes, e.g. if you need to check something else as well.

As stands, that method doesn't use any object state, so it can be static.

Ternary conditional operator

Another alternative is nesting the ternary conditional operator.

private static Item createItemType(final Integer type) {
    return (type == null) ? null
         : (type == 0) ? Item.PLACE
         : (type == 7) ? Item.ADDRESS
         : null;
}

This is even shorter than the inline if much less your original if/else tree. It only has one return keyword as compared to four in your original.

Personally, I find this at least as readable as the inline if statements if formatted like this. It still suffers from difficulty of adding side effects, but it is more obvious about why that won't work.

Map

private final static Map<Integer, Item> ITEMS = new HashMap<>();

static {
    ITEMS.put(0, Item.PLACE);
    ITEMS.put(7, Item.ADDRESS);
}

private static Item createItemType(final Integer type) {
    return ITEMS.get(type);
}

This is the shortest function, but you have to build the map separately.

It is possible to build the map automatically. Something like

foreach (Item item : Item.values()) {
    ITEMS.put(item.getIntValue(), Item.PLACE);
}

But that requires Item to implement getIntValue (or similar).

\$\endgroup\$
1
\$\begingroup\$

The sample code is too small, and lacks enough context, to allow a detailed analysis.

Initial points though are:

  1. Your method name is poorly chosen. It doesn't create anything, it looks something up.
  2. Your if statements immediately return, so there's no need for "else". In the IDE I use (eclipse) I'd get warnings about that!
  3. The suggestion to go without curly braces should be ignored. It's poor practice, as I know from > 20 years of writing Java.

I don't much like the use of "if" or "switch" for this sort of thing. There are approaches which are less fragile and easier to extend.

In my opinion, the best approach is to find a good way to represent the data you're working with - if you can do that, you can often avoid "if" or "switch"...

For this sort of lookup, I'm fond of using Java enums, so here's an example doing just that:

public class TypeGetter {

  static private enum Item {
    PLACE(0), ADDRESS(7);

    private int type;

    Item(int type) {
      this.type = type;
    }

    public static Item getByType(int requiredType) {
      for (Item candidate : Item.values()) {
        if (candidate.type == requiredType) {
          return candidate;
        }
      }
      return null;
    }

  }

  public static void main(String[] args) {
    TypeGetter typeGetter = new TypeGetter();
    for (int requiredType : new int[]{0, 7, 42}) {
      System.out.format("type %d gives item %s%n", requiredType, typeGetter.lookupItemByType(requiredType));
    }

  }

  private Item lookupItemByType(final int requiredType) {
    return Item.getByType(requiredType);
  }

}

The Item.getByType(int) method is a pattern I use quite a lot with enums. In this case, as the list is small, I've simply done a linear scan of the Items.

For more than (say) 6 or so Items, I'd probably use a Map, populated in a static initialiser. Here's the Item enum rewritten to use that approach:

  static private enum Item {
    PLACE(0), ADDRESS(7);

    private static Map<Integer, Item> byType = new HashMap<>();

    private int type;

    Item(int type) {
      this.type = type;
    }

    static {
      for (Item item : Item.values()) {
        byType.put(item.type, item); // relies on autoboxing
      }
    }

    public static Item getByType(int requiredType) {
      return byType.get(requiredType); // relies on autoboxing
    }

  }
\$\endgroup\$
0
\$\begingroup\$

In the latter java versions its even much easier. I would recommend the following:-

private static Item createItemType(final Integer type) {
    if (type != null) 
        return switch (type) {
            case 0 -> Item.PLACE;
            case 7 -> Item.ADDRESS;
            default -> return null;
        }
    return null;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Part of a code review is not only to give an alternative implementation, but also to explain why you believe your solution is better. One problem with your code, is that the if statement is lacking braces, which most style guides suggest that they are never left out. \$\endgroup\$
    – RoToRa
    Jul 16, 2021 at 21:31

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.