Need Judgement and Simplification

Good evening fellas, I am decently new to BUKKIT coding, and I really feel like I use too many if statements in this and honestly feel like I am doing this all wrong. The goal of the plugin is to create two teams, and when someone joins, it picks a random team(fire or water). And in the Random function, it chooses the fire team everytime. What can I do better? Much help would be appreciated Here is my code:

Code:

package me.spigot.package1;
import java.util.HashMap;
import java.util.Random;
import org.bukkit.command.Command;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;
import org.bukkit.event.player.PlayerJoinEvent;
import org.bukkit.plugin.java.JavaPlugin;
import net.md_5.bungee.api.ChatColor;
public class Main extends JavaPlugin implements Listener {
    static int loginNum = 0;
    HashMap<Player, Integer> hashmap = new HashMap<Player, Integer>();
    static Random rint = new Random();
    public void onEnable() {
        System.out.println("This plugin is working.. - yours truly");
        getServer().getPluginManager().registerEvents(this, this);
    }
    public void onDisable() {
        System.out.println("bye");
    }
    public static void main(String args[]) {
        System.out.println(rint.nextInt(2));
    }
    public boolean onCommand(CommandSender sender, Command cmd, String arg2, String[] args) {
        Player p = (Player) sender;
        if (sender instanceof Player) {
            if (cmd.getName().equalsIgnoreCase("logins")) {
                p.sendMessage(ChatColor.GRAY + "You have logged in " + ChatColor.AQUA + loginNum + ChatColor.GRAY
                        + " times. Keep at it!");
                return true;
            }
            if (cmd.getName().equalsIgnoreCase("team")) {
                if (hashmap.get(p) == 1) {
                    p.sendMessage("You belong to the team of water!");
                } else if (hashmap.get(p) == 0) {
                    p.sendMessage("You belong to the team of fire!");
                }
                return true;
            }
            return true;
        }
        return false;
    }
    @EventHandler
    public void logOn(PlayerJoinEvent e) {
        Player p = e.getPlayer();
        loginNum++;
        if (!p.hasPlayedBefore()) {
            hashmap.put(p, rint.nextInt(2));
            if (hashmap.get(p) == 1) {
                p.sendMessage("You have been assigned to the team of water!");
            } else if (hashmap.get(p) == 0) {
                p.sendMessage("You have been assigned to the team of fire!");
            }
        } else {
            if(hashmap.isEmpty()) {
                hashmap.put(p, rint.nextInt(2));
            }
            p.sendMessage(ChatColor.GRAY + "You have logged in " + ChatColor.AQUA + loginNum + ChatColor.GRAY
                    + " times. Keep at it!");
            if (hashmap.get(p) == 1) {
                p.sendMessage("Welcome back to the team of water!");
            } else if (hashmap.get(p) == 0) {
                p.sendMessage("Welcome back to the team of fire!");
            }
        }
    }
}

 

@Speurox
1. I don't see why you have a main method, just use the onEnable method.
2. Try removing static from your variables.
3. About having alot of if statements you can do something like this:

Code:Java

if (!(sender instanceof Player)) {
    sender.sendMessage("Players only");
    return true;
}
Player player = (Player) sender;
//Continue code here with the sender being a player
//This should stop the nesting

 

@Speurox
okay so here are some changes you have to make:

- Don't save a player instance in the hashmap. Every time the player rejoins it is another instance of player. Use his name or his uuid instead

- don't check if the hashmap is empty. Check if the hashmap not contains the player as a key

- im not sure for how long the player should be a member of the team. If its forever p.hasplayedbefore makes sense, but then it makes no sense that he gets a new team when rejoining. You would have to save the data in a file because the hashmap gets cleared every time the server restarts. If the membership only lasts until the server restarts, p.hasplayedbefore makes no sense because it only returns true if the player joins for the very first time, with other words only once in the accounts lifetime.

okay and another thing that you might have notived is that the counter of how often you logged in counts every player not only "you"

@KarimAKL
I think he has the main method for testing if the random function is working. I would do it the same way for debugging

 

Oh, i see. :7

 

Please mark this thread as Solved

 

If it's solved.. nobody confirmed that his problem is solved

 

I think that this should solve the issue. Unless you have like 10 alts (which you should without going into some shady stuff) then it should be like that.



@Speurox, is this thread solved? If yes then please mark it as solved and possibly reply. If not then tell us how you are testing it.

 

Hi.

I think you should use more private functions that handle only one function of your bigger function. Also, using static variables in a main class in a Bukkit project is unneccesary I think. It's better to use private variables because you get more control over those memorywise. Finally I would like to point out that the way to clean up your if-statements is to check for exceptions first. What I mean by this is for example that if the Sender isn't an instance of Player, your instantly return in your code without using else statements.

@KarimAKL also pointed out that checking for exceptions first is cleaner.

So here's a quick attempt at making cleaner code (in my opinion) of your code.

Code:

private int loginNum;
    private HashMap<UUID, Integer> hashmap;
    private Random rint;
  
    @Override
    public void onEnable() {
        System.out.println("This plugin is working.. - yours truly");
        getServer().getPluginManager().registerEvents(this, this);
      
        rint = new Random();
        hashmap = new HashMap<UUID, Integer>();
    }
  
    @Override
    public void onDisable() {
        System.out.println("bye");
    }
  
  
    public boolean onCommand(CommandSender sender, Command cmd, String arg2, String[] args) {
        if(!(sender instanceof Player)) return true;
        Player p = (Player) sender;
        if (cmd.getName().equalsIgnoreCase("logins"))
           handleLoginsCommand(p);
        else if (cmd.getName().equalsIgnoreCase("team"))
           handleTeamCommand(p);
        else return false;
      
        return true;
    }
  
    private void handleLoginsCommand(Player p){
        p.sendMessage(ChatColor.GRAY + "You have logged in " + ChatColor.AQUA + loginNum + ChatColor.GRAY
                + " times. Keep at it!");
    }
  
    private void handleTeamCommand(Player p){
         if (hashmap.get(p.getUniqueId()) == 1) {
             p.sendMessage("You belong to the team of water!");
         } else if (hashmap.get(p.getUniqueId()) == 0){
             p.sendMessage("You belong to the team of fire!");
         }
    }
  
    @EventHandler
    public void logOn(PlayerJoinEvent e) {
        Player p = e.getPlayer();
        loginNum++;
        if (!p.hasPlayedBefore()) {
            handleNotPlayedBefore(p);
            return;
        }
        handlePlayerJoin(p);
    }
  
    private void handleNotPlayedBefore(Player p){
        hashmap.put(p.getUniqueId(), rint.nextInt(2));
        if (hashmap.get(p.getUniqueId()) == 1) {
            p.sendMessage("You have been assigned to the team of water!");
        } else if (hashmap.get(p.getUniqueId()) == 0) {
            p.sendMessage("You have been assigned to the team of fire!");
        }
    }
    private void handlePlayerJoin(Player p){
        if(hashmap.isEmpty())
            hashmap.put(p.getUniqueId(), rint.nextInt(2));
      
        p.sendMessage(ChatColor.GRAY + "You have logged in " + ChatColor.AQUA + loginNum + ChatColor.GRAY
                + " times. Keep at it!");
        if (hashmap.get(p.getUniqueId()) == 1) {
            p.sendMessage("Welcome back to the team of water!");
        } else if (hashmap.get(p.getUniqueId()) == 0) {
            p.sendMessage("Welcome back to the team of fire!");
        }
    }