Bukkit/Spigot plugin for Minecraft servers

I have created a Bukkit/Spigot plugin for Minecraft servers because I wanted to change the dynamic of mining to prevent people with really bright monitors or the Fullbright mod from being able to skip using torches when mining.

I created this plugin a while back when I used to run a Minecraft server, but since then have updated it a bit to allow a chance of not receiving broken blocks instead of just preventing them from being broken. It's fully configurable.

It functions fully besides a few data validation checks. You can skip those if you want since this site isn't about fixing invalid code.

This is my first open source project, so I would love any contributions or ideas!

The code is also on Github.

com/imcpwn/nofullbrightmining/NoFullBrightMining.java

```
/* NoFullBrightMining Bukkit/Spigot plugin by IMcPwn.
* Copyright (C) 2016 IMcPwn

* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.

* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.

* You should have received a copy of the GNU General Public License
* along with this program. If not, see .

* For the latest code and contact information visit: http://imcpwn.com
*/

package com.imcpwn.nofullbrightmining;

import java.util.ArrayList;
import java.util.List;
import java.util.logging.Logger;
import org.bukkit.ChatColor;
import org.bukkit.Location;
import org.bukkit.Material;
import org.bukkit.block.Block;
import org.bukkit.command.Command;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;
import org.bukkit.event.EventHandler;
import org.bukkit.event.EventPriority;
import org.bukkit.event.L

public static NoFullBrightMining plugin;

This static field seems unused, in which case it can be removed.

// Global variables
ArrayList block_list = new ArrayList();
boolean perm_nfbm_disabled = false;
// Configuration variables
boolean nfbm_enabled;

I'm not really sure why you have both a perm_nfbm_disabled and nfbm_enabled flag. Try to use only one. Or if they don't mean the same thing, give them names that make that fact clear.

int minHeight;
int minLightLevel;
boolean dropChance_enabled;
double dropPercent;
boolean materials_blacklist;
List materials;

It seems all these variables are only used in this class, in which case they should be made private. You are also mixing naming conventions. Pick one and stick to it. The regular Java convention for variables is camelCase, so I suggest using that one.

onEnable

// Save default configuration
    this.saveDefaultConfig();

This comment doesn't add much and can be removed (after all the code says the same thing). Perhaps it is worth mentioning why you are saving the configuration when the plugin gets enabled.

loadConfig

if ((nfbm_enabled != true) && (nfbm_enabled != false)) {

This if statement will never be true.

if ((minHeight  256)) {
        logger.warning("Cannot load \"minimumheight\" configuration option! Default value is 50.");
        temp_disabled = true;
        logger.warning(pdfFile.getName()
                + " could not be loaded and is now disabled. ");
    }

Here you are giving a warning that the configuration is wrong, but are still continuing with the invalid value. Why not set the value to its default value?

minHeight = DEFAULT_MINIMUM_HEIGHT;

Overriding the incorrect configuration with the default value would save you from having to keep track of the temp_disabled flag.

onCommand

final String enabledMSG = "NoFullBrightMining: enabled";
    final String disabledMSG = "NoFullBrightMining: disabled";
    final String reloadFailedMSG = "NoFullBrightMining: configuration reload failed. NoFullBrightMining is now disabled. Check console for details.";
    final String invalidArgMSG = "NoFullBrightMining: invalid argument count";
    final String reloadMSG = "NoFullBrightMining: reloaded";
    final String noPermissionMSG = "NFBM: No permission";

These strings are all constants and should be defined at the beginning of your class. They should look like this:

private static final String ENABLED_MESSAGE = "NoFullBrightMining: enabled";

Defining them early and making them static gives you less stuff to think about, which is always nice (especially when you're reading this code in six months).

The next piece of code is too long and contains too much stuff so lets start breaking it up. First let's create separate methods for both of the commands you're accepting.

private boolean onNfbmCommand(CommandSender sender, String[] args) {
}   

private boolean onGetllCommand(CommandSender sender) {
}

The onCommand method will now look something like this:

@Override
public boolean onCommand(CommandSender sender, Command cmd,
        String commandLabel, String[] args) {
    if (commandLabel.equalsIgnoreCase("nfbm")) {
        return onNfbmCommand(sender, args);
    }
    else if (commandLabel.equalsIgnoreCase("getll")) {
        return onGetllCommand(sender);
    }
    return false;
}

Notice how all it is doing is passing the call on the the other methods when it is appropriate.

onNfbmCommand

Let's look at the onNfbmCommand Method. This part is difficult because it contains a very large nested if statement. This makes it hard to think about, so let's see what we can simplify.

It seems both branches are actually almost the same, except for the specific message they are sending. In other words, it doesn't matter whether the sender is a player or not. We can use that to our advantage!

First lets create a method that sends a message to the sender with a color if it's a Player and without a color if it's not:

private void sendMessage(CommandSender sender, String message, ChatColor color) {
    if (sender instanceof Player) {
        sender.sendMessage(color + message);
    } else {
        sender.sendMessage(message);
    }
}

We can now use this method in the onNfbmCommand method and we don't have to worry about the difference between player and sender anymore. Hurray, that's one less if-statement to worry about!

private boolean onNfbmCommand(CommandSender sender, String[] args) {

But we're still stuck with quite a big nested if-statement. To get rid of this nesting we can use something called a guard clause. Which basically means we're checking things as early as possible so we can return quickly. So in this case we can do this:

if (!sender.hasPermission("nfbm.admin")) {
        sendMessage(sender, noPermissionMSG, ChatColor.RED);
        return true;
    }

If the sender doesn't have permission we send a message and ret

StackExchange Code Review Q#123306, answer score: 3