Can I make this switch statement any smaller? Preferably with a for loop

Question

Is there a way to make this smaller, I tried using a for loop but couldn't make a random instance for each of the possible Types.

Random randFireworkEffect = new Random(5);
switch(randFireworkEffect.nextInt()) {
    case 0:
        e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.BALL).trail(true).build();
        break;
    case 1:
        e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.BALL_LARGE).trail(true).build();
        break;
    case 2:
         e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.BURST).trail(true).build();
        break;
    case 3:
         e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.CREEPER).trail(true).build();
        break;
    case 4:
         e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.STAR).trail(true).build();
        break;
}

yes, a lot shorter, but here we're talking about code review — Stultuske, Jul 06 '21 at 07:28
You have a lot of code duplication. The only thing that is different between all of those cases is the `Type` enum you pick. You should put that into a variable and then do the rest of the code after the `switch`. So `Type type = switch(...) { case 0 -> Type.BALL; case 1 -> ... };` and then `...with(type)...`. — Zabuzard, Jul 06 '21 at 07:33
You are just trying to convert an `int` to a `FireworkEffect.Type` enum, so [this](https://stackoverflow.com/questions/5878952/cast-int-to-enum-in-java) should help. There is no need for a switch statement, or a loop, at all. — Sweeper, Jul 06 '21 at 07:33
I’m voting to close this question because it belongs on [codereview.se]. — Ken Y-N, Jul 06 '21 at 07:34

Ofek · Accepted Answer · 2021-07-07T02:25:33.617

8

you can use .values()

Random randFireworkEffect = new Random();
e = FireworkEffect.builder()
        .flicker(true)
        .withColor(c)
        .withFade(c)
        .with(FireworkEffect.Type.values([randFireworkEffect.nextInt(5)])
        .trail(true)
        .build();

edited Jul 07 '21 at 02:25

answered Jul 06 '21 at 07:34

Ofek

1,065
6
19

I tried a for loop to iterate through the Type.values() but could not find a way to randomly select an effect, is there any forum where similar information might be provided? – RainDragon Ink Jul 07 '21 at 01:21
@RainDragonInk I fixed my code, the problem was that you use the same seed for the random. you used 5 as the seed instead of the max value (I just copied your code because you asked to make is smaller) – Ofek Jul 07 '21 at 02:26

Stultuske · Answer 2 · 2021-07-06T07:37:09.483

To make this:

Random randFireworkEffect = new Random(5);
                                switch(randFireworkEffect.nextInt()) {
                                case 0:
                                    e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.BALL).trail(true).build();
                                    break;
                                case 1:
                                    e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.BALL_LARGE).trail(true).build();
                                    break;
                                case 2:
                                     e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.BURST).trail(true).build();
                                    break;
                                case 3:
                                     e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.CREEPER).trail(true).build();
                                    break;
                                case 4:
                                     e = FireworkEffect.builder().flicker(true).withColor(c).withFade(c).with(Type.STAR).trail(true).build();
                                    break;

shorter, just look at what is exactly the difference between all those lines, it's the Type:

Type[] types = new Type[]{Type.BALL, Type.BALL_LARGE, Type.BURST, Type.CREEPER, Type.STAR};
Random effect = new Random(5);
e = FireworkEffect.builder().flicker(true)
    .withColor(c).withFade(c)
    .with(types[effect.nextInt()])
    .trail(true).build();

And yes, you can put that in a loop, to have more values set.

@Ofek I've adapted the Type. as for the values(), since I'm not familiar with this Type class, I wasn't going to bet on it :) — Stultuske, Jul 06 '21 at 07:37
its from spigot: https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/FireworkEffect.Type.html (im familiar with this too, but it looked like spigot so i searched it) — Ofek, Jul 06 '21 at 07:38

score 2 · Answer 3 · answered Jul 06 '21 at 07:36

There is the following best practice to avoid dummy case-statements. First some map of pre-defined relations should be created. Then you are able to take values from it in runtime.

private static final Map<Integer, Type> typeMap = new Hashmap<>();

static {
  typeMap.put(0, Type.BALL);
  typeMap.put(1, Type.BALL_LARGE);
  typeMap.put(2, Type.BURST);
  typeMap.put(3, Type.CREEPER);
  typeMap.put(4, Type.STAR);
}

public FireworkEffect getFireworkEffect() {
   Random randFireworkEffect = new Random(5);
   Type type = typeMap.get(randFireworkEffect);
   return FireworkEffect.builder()
          .flicker(true)
          .withColor(c)
          .withFade(c)
          .with(type)
          .trail(true)
          .build();
}

In case of integers array is ok of course. But I am speaking about the whole principle when ifs and cases replace with maps. — kirill.login, Jul 06 '21 at 07:38

Nikolai Shevchenko · Answer 4 · 2021-07-06T07:44:28.733

0

Iterating over .values() is not optimal. It's better to use pre-populated hash map

Map<Integer, Type> typeMap = new HashMap<>();
typeMap.put(0, Type.BALL);
typeMap.put(1, Type.BALL_LARGE);
// ... other options

Random randFireworkEffect = new Random(5);
e = FireworkEffect.builder()
        .flicker(true)
        .withColor(c)
        .withFade(c)
        .with(typeMap.get(randFireworkEffect.nextInt()))
        .trail(true)
        .build();

edited Jul 06 '21 at 07:44

answered Jul 06 '21 at 07:38

Nikolai Shevchenko

7,083
8
33
42

Why not a pre-populated *list*, then? – Ole V.V. Jul 06 '21 at 08:10
if you use pre-populated array, you probably will use `.values()`. also using `.values()` for an enum with 5 element its not too slow. – Ofek Jul 06 '21 at 08:35
@OleV.V. since there's no guarantee that given integers are relevant to ordinals of `Type` enum items. – Nikolai Shevchenko Jul 06 '21 at 08:48
Are we assuming that `randFireworkEffect.nextInt()` returns an `int`? I didn’t understand your argument, you may want to explain. – Ole V.V. Jul 06 '21 at 14:56
@OleV.V. @Ofek i did mean that you can't conclude from OP's code sample that Type items are in exact order (BALL, BALL_LARGE, BURST, CREEPER, STAR) and thus e.g. `Type.values()[0]` is `BALL` and `Type.values()[1]` is `BALL_LARGE` , etc. However this order is assumed in the Accepted Answer – Nikolai Shevchenko Jul 07 '21 at 05:42

Can I make this switch statement any smaller? Preferably with a for loop

4 Answers4