Avoiding the arrow anti-pattern

When you read this, chances are high that someone saw your code and decided that you did something that is considered horribly bad: using the arrow anti-pattern.

What the arrow anti-pattern is

Look at the following code:

    @EventHandler
    public void onPlaceWaterInNether(BlockPlaceEvent event) {
        if(!event.isCancelled()) {
            if(event.getBlock().getWorld().getEnvironment() == World.Environment.NETHER) {
                if(event.getBlockReplacedState().getType() == Material.AIR) {
                    if(event.getItemInHand() != null) {
                        if(event.getItemInHand().getType() == Material.WATER_BUCKET) {
                            event.getItemInHand().setAmount(event.getItemInHand().getAmount()-1);
                            event.setCancelled(true);
                            event.getBlock().setType(Material.WATER);
                        }
                    }
                }
            }
        }
    }

It looks like a huge arrow (>) symbol. It’s ugly, unreadable and hard to maintain. Now, let’s say you want to expand that code:

    @EventHandler
    public void onPlaceWaterInNether(BlockPlaceEvent event) {
        if(!event.isCancelled()) {
            if(event.getBlock().getWorld().getEnvironment() == World.Environment.NETHER) {
                if(event.getBlockReplacedState().getType() == Material.AIR) {
                    if(event.getItemInHand() != null) {
                        if(event.getItemInHand().getType() == Material.WATER_BUCKET) {
                            event.getItemInHand().setAmount(event.getItemInHand().getAmount()-1);
                            event.setCancelled(true);
                            event.getBlock().setType(Material.WATER);
                        }
                    }
                }
            } else {
                if(event.getBlockReplacedState().getType() == Material.AIR) {
                    if(event.getItemInHand() != null) {
                        if(event.getItemInHand().getType() == Material.WATER_BUCKET) {
                            event.getItemInHand().setAmount(event.getItemInHand().getAmount()-1);
                            event.setCancelled(true);
                            event.getBlock().setType(Material.LAVA);
                        }
                    }
                }
            }
        }

Wow. What a mess. Can you instantly tell to which if statement the else statement belongs? I cannot. Also, three if statements are duplicated. That’s just extremely bad, and unmaintainable, and unnecessary.

How to properly do it

Instead of nesting all those if statements, simply return from your listener whenever a condition is NOT met. The second example from above will now look like this:

    @EventHandler
    public void onPlaceWaterInNether(BlockPlaceEvent event) {

        if(event.isCancelled()) return;
        if(event.getBlockReplacedState().getType() != Material.AIR) return;
        if(event.getItemInHand() == null) return;
        if(event.getItemInHand().getType() != Material.WATER_BUCKET) return;
        
        event.getItemInHand().setAmount(event.getItemInHand().getAmount()-1);
        event.setCancelled(true);
        
        if(event.getBlock().getWorld().getEnvironment() == World.Environment.NETHER) {
            event.getBlock().setType(Material.WATER);
        } else {
            event.getBlock().setType(Material.LAVA);
        }
    }

Finally, people can easily understand what your code does, and you reduced the chance for bugs by a huge amount. For example, it’s easily visible that the else block belongs to the part where we are checking whether the world is a nether world.