Need Help with Team Coding.

Hello everyone, I have ran into an issue with some old code. Everything works other than when someone chooses to go red (1 player) it will add another player to the Attacker team making it have (2 players) on that team when I'm testing with just my alt account and my one account. I'm not sure what is causing this.

CODE:

Code:

@EventHandler
    public void selectteam(InventoryClickEvent event) {
    if (event.getInventory() != null && event.getInventory().getTitle() != null && event.getInventory().getTitle().equals(teams.getTitle())) {
     if (event.getCurrentItem() != null) {
      Player p = (Player) event.getWhoClicked();
      if (plugin.players.contains(p)) {
       if (event.getCurrentItem().equals(attackers)) {
        if (p.hasPermission("dtp.selectteam") || p.hasPermission("dtp.*")) {
         if (!(plugin.attackers.contains(p.getUniqueId()))) {
          boolean go = false;
          if ((plugin.attackers.size() + 1) <= (plugin.maxplayers / 4) && (plugin.attackers.size() + 1) <= (Bukkit.getOnlinePlayers().size() / 2)) {
           go = true;
          }
          if (go) {
           if (plugin.defenders.contains(p.getUniqueId())) {
            plugin.defenders.remove(p.getUniqueId());
            plugin.removeBlue(p.getPlayer());
            defenders.setAmount(plugin.defenders.size());
            teams.setItem(6, defenders);
           }
           plugin.attackers.add(p.getPlayer().getUniqueId());
           plugin.addRed(p);
           attackers.setAmount(plugin.attackers.size());
           teams.setItem(2, attackers);
           p.playSound(p.getLocation(), Sound.NOTE_PLING, 1, 1);
           p.closeInventory();
           p.sendMessage(plugin.prefix + "You're now on the Attacking team.");
          } else {
           p.closeInventory();
           p.sendMessage(plugin.prefix + "There are too many players on the selected team.");
          }
         } else {
          p.closeInventory();
          p.sendMessage(plugin.prefix + "You are already in this team.");
         }

        } else {
         p.sendMessage(plugin.prefix + "You don't have the required permission to choose team.");
         p.closeInventory();
        }
       } else if (event.getCurrentItem().equals(defenders)) {
        if (p.hasPermission("dtp.selectteam") || p.hasPermission("dtp.*")) {
         if (!(plugin.defenders.contains(p.getUniqueId()))) {
          boolean go = false;
          if ((plugin.defenders.size() + 1) <= (plugin.maxplayers / 4) && (plugin.defenders.size() + 1) <= (Bukkit.getOnlinePlayers().size() / 2)) {
           go = true;
          }
          if (go) {
           if (plugin.attackers.contains(p.getUniqueId())) {
            plugin.attackers.remove(p.getUniqueId());
            plugin.removeRed(p.getPlayer());
            attackers.setAmount(plugin.attackers.size());
            teams.setItem(2, attackers);
           }
           plugin.defenders.add(p.getUniqueId());
           plugin.addBlue(p);
           defenders.setAmount(plugin.defenders.size());
           teams.setItem(6, defenders);
           p.playSound(p.getLocation(), Sound.NOTE_PLING, 1, 1);
           p.closeInventory();
           p.sendMessage(plugin.prefix + "You're now on the Defending team.");
          } else {
           p.closeInventory();
           p.sendMessage(plugin.prefix + "There are too many players on the selected team.");
          }
         } else {
          p.closeInventory();
          p.sendMessage(plugin.prefix + "You are already in this team.");
         }
        } else {
         p.sendMessage(plugin.prefix + "You don't have the required permission to choose team.");
         p.closeInventory();
        }
       } else if (event.getCurrentItem().equals(autoselect)) {
        if (plugin.attackers.contains(p.getUniqueId())) {
         plugin.attackers.remove(p.getUniqueId());
         plugin.removeRed(p.getPlayer());
         attackers.setAmount(plugin.attackers.size());
         teams.setItem(2, attackers);
        }
        if (plugin.defenders.contains(p.getUniqueId())) {
         plugin.defenders.remove(p.getUniqueId());
         plugin.removeBlue(p.getPlayer());
         defenders.setAmount(plugin.defenders.size());
         teams.setItem(6, defenders);
        }
        p.playSound(p.getLocation(), Sound.ENDERDRAGON_HIT, 20, 20);
        p.closeInventory();
        p.sendMessage(plugin.prefix + "You will now automatically join a team once the game starts.");
       }
      }
     }
    }
    }

Please help if you can.

 

I tried to read your code but... it is extremely unreadable. Get rid of all this nesting first, im sure it will be clear after that. https://en.wikibooks.org/wiki/Computer_Programming/Coding_Style/Minimize_nesting

 

Thanks, hopefully it's a little easier now I updated it in the post.

 

@VinexAx789 do you want to keep this code or would you change it if it can be improved? The way it is made atm is more complicated than it would need to be. you could use a Map<Material, Set<UUID>> in which you store the teams. the material represents the key and is the material of the itemstack you have to click in the inventory to join the team. the set<uuid> represents the member of the team.

so once sb clicks on a item in the inventory you just do smth like that:

Code:

// this is pseudo code, it wont work exactly like this!
Material clickedMaterial = getClickedItemStack().getMaterial();
Set<UUID> team = teamMap.get(clickedMaterial);
// check here if team is full or what ever you want to check before adding him
UUID playerId = getPlayerWhoClicked().getUUID();
if(!team.contains(playerId){
// player joins team -> message him
team.add(playerId);
}else{
// player is already in the team -> message him
}

You also dont have to check if the event contains null for the inventory or the player. Events should always be filled with information. And you can easily avoid null checks by checking in the other direction. here an example:

Code:

final String str1 = "this string is filled";
final String str2 = null; // this string is null

// instead of
if(str2!=null&&str2.equals(str1)){ ... }

// use
if(str1.equals(str2)) { ... }

// this way you spare code and make it more readable

 

@Shmobi It's old code so yeah I think we should make it more improved.

 

@VinexAx789 Well then as i said you could use a map for this. this would also give you the possibility of 2+ teams. But i guess you only need 2, dont you? So what you want to do is to make a Map<Material, Set<UUID>>. Material as the key where the material represents the material of the itemstack which must be clicked in the inventory to join the team and the Set<UUID> as the value which represents the member of a team. this way you can simply access a teams member list and add new member or remove them Did you ever work with maps before?

 

@Shmobi No, not really.

 

Did you ever work with objects before? There is not a single reason to use maps here when he needs to do more than add and remove players from teams. You see he has addRed()/removeRed() and addBlue()/removeBlue() methods? Using maps would make this a lot more complicated than the way he currently has it set up.

 

@HoeMC I have. And yeah I want to keep it how I currently have it setup, not change a bunch of things.

Could this be why?

Code:

if ((plugin.attackers.size() + 1) <= (plugin.maxplayers / 4) && (plugin.attackers.size() + 1) <= (Bukkit.getOnlinePlayers().size() / 2)) {

 

@VinexAx789 What that basicly does is checking if the playeramount in the team is smaller than a quater of the max amount of the players for this plugin and smaller than half of the players on the server. Don't think this causes the error you're having.

btw you could leave the +1 and also change "<=" to "<". it does the same and is less confusing.

 

I fixed it thanks for the help though I also recoded it a better way thanks a ton.