Vanisher plugin for Minecraft (Written with the Bukkit API)

Would this code for my Vanisher plugin for the game Minecraft, written with the Bukkit API, be optimised and conventionally right?

VanishAPI:

package com.azoraqua.vanisher.api;

import com.azoraqua.vanisher.exception.VanishException;
import org.bukkit.entity.Player;

import java.util.Collection;
import java.util.UUID;

public interface VanishAPI {

    void vanish(Player player) throws VanishException;

    void unVanish(Player player) throws VanishException;

    boolean isVanished(Player player);

    Collection<UUID> getVanished();
}

Main:

package com.azoraqua.vanisher;

import com.azoraqua.vanisher.api.VanishAPI;
import com.azoraqua.vanisher.command.VanishCommand;
import com.azoraqua.vanisher.command.VanishedCommand;
import com.azoraqua.vanisher.exception.VanishException;
import com.azoraqua.vanisher.listener.*;
import org.bukkit.Bukkit;
import org.bukkit.GameMode;
import org.bukkit.entity.Player;
import org.bukkit.plugin.java.JavaPlugin;
import org.bukkit.potion.PotionEffect;
import org.bukkit.potion.PotionEffectType;

import java.util.*;

public class Main extends JavaPlugin implements VanishAPI {

    private static Main instance;
    private final List<UUID> vanished = new ArrayList<>();

    @Override
    public final void onEnable() {
        Main.instance = this;

        getCommand("vanish").setExecutor(new VanishCommand());
        getCommand("vanished").setExecutor(new VanishedCommand());

        getServer().getPluginManager().registerEvents(new PlayerQuitListener(), this);
        getServer().getPluginManager().registerEvents(new PlayerDropItemListener(), this);
        getServer().getPluginManager().registerEvents(new PlayerPickupItemListener(), this);
        getServer().getPluginManager().registerEvents(new BlockBreakListener(), this);
        getServer().getPluginManager().registerEvents(new BlockPlaceListener(), this);
        getServer().getPluginManager().registerEvents(new PlayerInteractListener(), this);
    }

    @Override
    public final void onDisable() { }

    public static Main getInstance() {
        return instance;
    }

    @Override
    public final void vanish(Player player) throws VanishException {
        if(this.isVanished(player)) {
            throw new VanishException("Player is already vanished.");
        }

        vanished.add(player.getUniqueId());

        if(player.getGameMode() != GameMode.CREATIVE || player.getGameMode() != GameMode.SPECTATOR) {
            player.setAllowFlight(true);
            player.setFlying(true);
        }

        player.addPotionEffect(new PotionEffect(PotionEffectType.INVISIBILITY, Integer.MAX_VALUE, Integer.MAX_VALUE, false, false));
        Bukkit.getOnlinePlayers().forEach((otherPlayer) -> otherPlayer.hidePlayer(player));
    }

    @Override
    public final void unVanish(Player player) throws VanishException {
        if(!this.isVanished(player)) {
            throw new VanishException("Player is not yet vanished.");
        }

        vanished.remove(player.getUniqueId());

        if(player.getGameMode() != GameMode.CREATIVE || player.getGameMode() != GameMode.SPECTATOR) {
            player.setAllowFlight(false);
            player.setFlying(false);
        }

        player.removePotionEffect(PotionEffectType.INVISIBILITY);
        Bukkit.getOnlinePlayers().forEach((otherPlayer) -> otherPlayer.showPlayer(player));
    }

    @Override
    public final boolean isVanished(Player player) {
        return vanished.contains(player.getUniqueId());
    }

    @Override
    public final Collection<UUID> getVanished() {
        return Collections.checkedCollection(vanished, UUID.class);
    }
}

VanishCommand:

package com.azoraqua.vanisher.command;

import com.azoraqua.vanisher.Main;
import org.bukkit.Bukkit;
import org.bukkit.command.Command;
import org.bukkit.command.CommandExecutor;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;

import java.util.UUID;

public class VanishCommand implements CommandExecutor {

    @Override
    public boolean onCommand(CommandSender sender, Command cmd, String s, String[] args) {
        if (sender instanceof Player) {
            Player player = (Player) sender;
            Player target;

            if (args.length > 0) {
                if(sender.hasPermission("vanisher.vanish.others")) {
                    String input = args[0];

                    if (input.matches("^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}$")) {
                        target = Bukkit.getPlayer(UUID.fromString(input));
                    } else {
                        target = Bukkit.getPlayer(input);
                    }
                } else {
                    sender.sendMessage("§cYou cannot vanish other players. Missing permission '§bvanisher.vanish.others§c'.");
                    return true;
                }
            } else {
                if(sender.hasPermission("vanisher.vanish")) {
                    target = player;
                } else {
                    sender.sendMessage("§cYou cannot vanish yourself. Missing permission '§bvanisher.vanish§c'.");
                    return true;
                }
            }

            if (target != null) {
                if (target.equals(player)) {
                    if(Main.getInstance().isVanished(player)) {
                        Main.getInstance().unVanish(player);
                        sender.sendMessage("§aYou have been unvanished.");
                        return true;
                    } else {
                        Main.getInstance().vanish(player);
                        sender.sendMessage("§aYou have been vanished.");
                        return true;
                    }
                } else {
                    if(Main.getInstance().isVanished(target)) {
                        Main.getInstance().unVanish(target);
                        sender.sendMessage(String.format("§aYou have unvanished §b%s.", target.getName()));
                        return true;
                    } else {
                        Main.getInstance().vanish(target);
                        sender.sendMessage(String.format("§aYou have vanished §b%s.", target.getName()));
                        return true;
                    }
                }
            } else {
                sender.sendMessage("§cThat player is not online.");
                return true;
            }
        }

        return false;
    }
}

VanishedCommand:

package com.azoraqua.vanisher.command;

import com.azoraqua.vanisher.Main;
import org.bukkit.Bukkit;
import org.bukkit.command.Command;
import org.bukkit.command.CommandExecutor;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;

import java.util.Collection;
import java.util.UUID;

public class VanishedCommand implements CommandExecutor {

    @Override
    public boolean onCommand(CommandSender sender, Command cmd, String s, String[] args) {
        if(sender.hasPermission("vanisher.list")) {
            Collection<UUID> vanished = Main.getInstance().getVanished();
            StringBuilder sb = new StringBuilder(String.format("§6Vanished Players (%s): ", vanished.size()));

            int checked = 0;

            for(UUID onlineUUID : vanished) {
                Player onlinePlayer = Bukkit.getPlayer(onlineUUID);

                sb.append(String.format("§8%s", onlinePlayer.getName()));
                checked++;

                if(checked < vanished.size()) {
                    sb.append("§f, §8");
                }
            }

            String list = sb.toString().trim();

            sender.sendMessage(list);
            return true;
        }

        return false;
    }
}

This is really interesting! I’ve never seen the source of a Minecraft mod before 🙂

Using a set

If you don’t need the ability to store duplicate elements, sets are usually a better choice than lists. The most significant difference in this case is the contains method. It runs in constant time on a HashSet and linear time on an ArrayList. This is because the ArrayList has to walk through each element until it finds the searched element. The HashSet only needs one lookup to decide if an element is contained within it.

In fact, when using a set there’s much less need to call contains anyway. The add and remove methods return true if the operation went through and false if not. If you change the type of vanished to a set, you can omit the call to isVanished from both vanish and unVanish.

If you need more advice on which collection to use, check out this flowchart: What Java Collection should I use?

Nested if statements

Having too many nested if statements can make code hard to read. I think the way you handle if statements in vanish and unVanish is the right way to do it.

if(this.isVanished(player)) {
    throw new VanishException("Player is already vanished.");
}

Here, you test for the error case and throw/return early. You could employ the same principle in the onCommand method to decrease indentation and improve readability.

1.

if (sender instanceof Player) {
    // ... 50 lines of code ...
}

return false;

2.

if (!sender instanceof Player) {
    return false;
}

// ... 50 lines of code ...

// nothing has returned?
return false;

In number 2 it’s easier to see what happens in both cases. This refactoring can be applied in a couple of places in your code.

Methods are your friend

Don’t be hesitant to split your methods up into smaller pieces! It can really improve how easily understandable it is. Because VanishCommand.onCommand is quite long and does several distinct tasks (parse sender name, check permissions etc), I think you should refactor it into smaller private methods.