Solved Better organizing of class

My class is an absolute mess. Due to its nature (Buying Commands), it is tons of if statements.
Without saying more, IT IS A MESS.

Code:java

package me.Tecno_Wizard.CommandsForSale.commandProcessors;
 
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.UUID;
 
import me.Tecno_Wizard.CommandsForSale.core.Main;
import net.milkbowl.vault.economy.Economy;
import net.milkbowl.vault.economy.EconomyResponse;
 
import org.bukkit.ChatColor;
import org.bukkit.Sound;
import org.bukkit.command.Command;
import org.bukkit.command.CommandExecutor;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;
/**
* Handles the purchase of commands when the economy has loaded successfully. Handles /buycmd, /confirm, /deny
* @author Ethan Zeigler
*
*/
public class BuyCmdExecutor implements CommandExecutor
{
    private HashMap<UUID, String> awaitingConfirm = new HashMap<UUID, String>(); //stores players that need a /confirm or /deny response
    Main main;
    Economy econ;
    String pluginPrefix;
 
    //handles /buycmd, /confirm, /deny
 
    public BuyCmdExecutor(Main main, Economy econ)
    {
        this.main = main;
        this.econ = econ;
        pluginPrefix = main.getConfig().getString("PluginPrefix");
    }
 
    @Override
    public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args)
    {
        if(sender instanceof Player)
        {
            if(cmd.getName().equalsIgnoreCase("buycmd"))
            {
                if(args.length == 1)
                {
                    args[0] = args[0].toLowerCase();
                    if(awaitingConfirm.containsKey(((Player) sender).getUniqueId()))//if player is already prompted for confirmation
                    {
                        sender.sendMessage(String.format("%s[%s] You are currently awaiting confirmation for another command. %s/confirm %sor %s/deny %sfirst.", ChatColor.RED, pluginPrefix, ChatColor.GOLD, ChatColor.RED, ChatColor.GOLD, ChatColor.RED));
                    }
                    else//if not awaiting confirmation
                    {
                        if(main.getConfig().getStringList("MainCommands").contains(args[0]))//if the command that is trying to be purchased is purchasable
                        {
                            if(!main.getSave().getStringList(((Player) sender).getUniqueId().toString()).contains(args[0]))//if the player does not already have the command
                            {
                                if(sender.hasPermission("cmdsforsale." + main.getConfig().getString("CommandOptions." + args[0] + ".permission")) || main.getConfig().getString("CommandOptions." + args[0] + ".permission") == null)
                                {
                                    double price = main.getConfig().getDouble("CommandOptions." + args[0] + ".price");//gets the price of the command
 
                                    if(econ.getBalance((Player)sender) >= price)//if the user has enough money to buy the command
                                    {
                                        sender.sendMessage(String.format("%s[%s] Do you want to buy the command %s%s %sfor %s%d %s%s? %s\n/confirm or /deny", ChatColor.YELLOW, pluginPrefix, ChatColor.GOLD, args[0], ChatColor.YELLOW, ChatColor.GOLD, price, ChatColor.YELLOW, econ.currencyNamePlural(), ChatColor.YELLOW));
 
                                        awaitingConfirm.put(((Player) sender).getUniqueId(), args[0]);//adds to confirmation list
                                    }
                                    else //if the user is a poor old man (does not have enough money)
                                    {
                                        sender.sendMessage(String.format("%s[%s] You do not have enough %s to buy this command", ChatColor.RED, pluginPrefix, econ.currencyNamePlural()));
 
                                    }
                                }
                            }
                            else//if the player already has the command
                            {
                                sender.sendMessage(String.format("%s[%s] You already have this command", ChatColor.AQUA, pluginPrefix));
 
                            }
                        }
                        else //if the requested command is not purchasable
                        {
                            sender.sendMessage(String.format("%s[%s] That command is not purchasable. If you are typing in an aliase of the main command, such as bal instead of balance, it will not be recognized.", ChatColor.RED, pluginPrefix));   
                        }
                    }
                }
                else//if there was not a single argument
                {
                    sender.sendMessage(String.format("%s[%s] The correct usage is %s/buycmd <Command Name>", ChatColor.YELLOW, pluginPrefix, ChatColor.GOLD));
                }
            }
 
            ////////////////////////////////////////////////////////////////////////////////////
            ////////////////////////////////////////////////////////////////////////////////////
 
            else if(cmd.getName().equalsIgnoreCase("confirm"))
            {
                if(awaitingConfirm.containsKey(((Player) sender).getUniqueId()))
                {
                    String toBuy = awaitingConfirm.get(((Player) sender).getUniqueId());
                    awaitingConfirm.remove(((Player) sender).getUniqueId());//plyaer removed from awaiting confirmation
                    double price = main.getConfig().getDouble("CommandOptions." + toBuy + ".price");
                    if(econ.getBalance((Player)sender) >= price)//double check of if player can afford
                    {
                        EconomyResponse eResponse = econ.withdrawPlayer((Player)sender, price);//takes out the money (if it can)
                        if(eResponse.transactionSuccess())
                        {
                            sender.sendMessage(String.format("%s[%s] You bought the command %s!", ChatColor.GOLD, pluginPrefix, toBuy));
                            ((Player) sender).playSound(((Player) sender).getLocation(), Sound.CHICKEN_EGG_POP, 1, 5);
                            if(!main.getSave().contains(((Player) sender).getUniqueId().toString()))
                                main.getSave().set(((Player) sender).getUniqueId().toString(), new ArrayList<String>());//if the player did not have a save section, one is created
                            main.getSave().saveConfig();//save that config
 
                            List<String> boughtCmds = main.getSave().getStringList(((Player) sender).getUniqueId().toString());//commands to be given to the player
                            boughtCmds.add(toBuy);//adds main command
 
                            main.getSave().set(((Player) sender).getUniqueId().toString(), boughtCmds);
                            List<String> alises = main.getConfig().getStringList("Aliases." + toBuy);//gets the aliases of the command
                            for(String alis: alises)
                            {
                                boughtCmds.add(alis);
                            }
                            main.getSave().set(((Player) sender).getUniqueId().toString(), boughtCmds);
                            main.getSave().saveConfig();
                        }
                        else
                        {
                            sender.sendMessage(String.format("%s[%s] Purchase Failed: %s", ChatColor.RED, pluginPrefix, eResponse.errorMessage));
                        }
                    }
                    else //if player cannot afford
                    {
                        sender.sendMessage(String.format("%s[%s] You cannot afford this!", ChatColor.RED, pluginPrefix));
                    }
                }
                else//nothing awaiting to be confirmed
                {
                    sender.sendMessage(String.format("%s[%s] You are not trying to purchase any command", ChatColor.RED, pluginPrefix));
                }
 
            }
 
            //////////////////////////////////////////////////////////////////////////////////
            //////////////////////////////////////////////////////////////////////////////////
            else//will be deny
            {
                if(awaitingConfirm.containsKey(((Player) sender).getUniqueId()))//if awaiting confirmation
                {
                    sender.sendMessage(String.format("%s[%s] Purchase cancelled", ChatColor.GRAY, pluginPrefix));
                    awaitingConfirm.remove(((Player) sender).getUniqueId());
                }
                else//if not awaiting confirmation
                {
                    sender.sendMessage(String.format("%s[%s] You are not trying to purchase any command", ChatColor.RED, pluginPrefix));
                }
            }
        }
        else//if the sender is the console
        {
            sender.sendMessage(String.format("[%s] Silly console, buying commands is for players!", pluginPrefix));
        }
        return true;
    }
}
 

Does anyone know of a more efficient way to do this? I'm desperate.

 

Tecno_Wizard
I prefer not using else statements. Rather I do it like this:
"normal" if-else statement

Code:

if(condition) {
    // stuff
} else {
    // send your 'else' message
}

My preferred way:

Code:

if(!condition) {
    // send your 'else' message
    return;
}
 

// stuff

In my opinion it doesn't make the code look so chaotic.

 

Alright. I've looked at your code, and frankly it can be greatly improved with some proper use of OOP. You should be able to reduce your onCommand method to a mere ten lines or so using what we all know and love: Abstraction. You should also apply the DRY principle. I'll give you some tips to get you going.

What I would do, is make an interface called BuyCommandHandler (or something similar). This interface should declare one method:

Code:java

void perform(Player sender, boolean awaitingConfirm, String arg);

With this new interface created, you can now declare a Map<Command, BuyCommandHandler> in your BuyCmdExecutor. What you should also have in your BuyCmdExecutor is a method to register BuyCommandHandlers. Something along these lines:

Code:java

public void registerCommand(Command cmd, BuyCommandHandler handler) {
    //Put the cmd and handler in the newly created map
    cmd.setExecutor(this);
}

With this in mind, you no longer need to set the executor from the main class, and this method could work as a replacement for that. Nifty.

Still, not quite satisfied. We still need to handle these commands. Your onCommand method needs an overhaul. You could keep the instanceof Player check, since that's just a guard, but all the concrete implementation has to go (or rather, be moved somewhere else). What you should do is create new classes that implement the BuyCommandHandler interface. One class per command. When implementing the perform method, know that the sender argument is the player who sent the command, awaitingConfirm is, well, whether the player is awaiting confirmation from a previous execution of /buycmd, and arg is any extra argument that may be needed for the command (namely the arg when using /buycmd).

You could now move the implementation of each command from the onCommand method, to each of your new classes respectively, making any necessary adjustments for the transfer. What's important here is that each command does not need to manually check the awaitingConfirm Map, as the information is already given to them by the onCommand method. Let's make the onCommand method actually do that.

You should now have moved a lot of implementation away from the onCommand method, and it probably contains a lot of empty code blocks. Know that all your onCommand method needs to do is this:

• Check if the sender is a player. (You already have that)
• Check the length of args (needed for a later point)
• Check if the sender is in the awaitingConfirm Map (also needed for a later point)
• Get the BuyCommandHandler mapped to the command that was passed to onCommand
• Execute the perform method of said handler with these arguments:
  • sender (the sender of the command, as a player)
  • awaitingConfirm (whether or not the player was present in the awaitingConfirm map)
  • arg (the first argument passed to the command, if one exists. null otherwise)

It would look something like this:

Code:java

BuyCommandHandler handler = handlers.get(cmd);
 
Player ply = (Player) sender
boolean awaiting = awaitingConfirm.containsKey(ply.getUniqueId())
String arg = args.length >= 1 ? args[0] : null;
 
handler.perform(ply, awaiting, arg);

I also see your commands are altering the awaitingConfirm map, which they wouldn't directly be able to if they are in different classes. A simple solution to this is to declare public methods to remove and add players from the Map in the BuyCmdExecutor class, and make sure the handlers have a reference to the BuyCmdExecutor instance. An even better solution (and the one I'd personally go with) would be to make the perform methods return a result code (likely an enum constant) and make the onCommand method check the result code and act appropriately according to the returned code. Example:

Code:java

public enum BuyCommandResult {
 
    CONFIRM_ADD,
    CONFIRM_REMOVE,
    NONE;
 
}

Code:java

//NB: This is in the onCommand method
 
BuyCommandResult result = handler.perform(ply, awaiting, arg);
switch (result) {
    case CONFIRM_ADD:
        //Put player and arg into the map
        break;
    case CONFIRM_REMOVE:
        //Remove player from the map
        break;
}

Wow. Got a bit excited there. I've given you some instructions and some code, and some might call this spoonfeeding. I don't. This is an opportunity for you to learn something. The most important thing for me when you read this is that you don't just copy my instructions and code and forget why you do it. Please, if you're copying code, know why the code does what it does.

If you have any questions I'll be happy to help!

 

Rocoty
Absolutely amazing. Thank you so much! One issue I found though, setExecutor() is not a method of Command. Besides that, spoon feeding is only spoon feeding when you can't learn from it. I will definitely use this format in the future.

 

Tecno_Wizard Ah, yes. That should be PluginCommand then. A mistake on my part.

 

Rocoty, I have finished it. It's not the way you had exactly shown, but what you gave me was not sufficient for the complexity that was necessary. It is MUCH better though.
(The main class here is the BuyCommandController)
https://github.com/Ethan160503/Comm.../CommandsForSale/commandProcessors/buyCommand