Need advice

I have no problem whatsoever i just need advice on this class. Also best way to do this?

Code:java

 
package Libs;
 
import org.bukkit.ChatColor;
import org.bukkit.GameMode;
import org.bukkit.Location;
import org.bukkit.Material;
import org.bukkit.entity.Player;
import org.bukkit.inventory.ItemStack;
import org.bukkit.inventory.PlayerInventory;
 
import java.util.Collections;
import java.util.UUID;
 
/**
* Created by jan on 9/25/16.
*/
public class Staffmode {
 
    private Location savedLocation;
    private ItemStack[] savedContents = new ItemStack[41];
    private Player savedPlayer;
    private GameMode savedGamemode;
    private double savedHealth;
    private int savedFood;
 
    public Staffmode(Player player) {
        this.savedContents = player.getInventory().getContents();
        this.savedLocation = player.getLocation();
        this.savedPlayer = player;
        this.savedGamemode = player.getGameMode();
        this.savedHealth = player.getHealth();
        this.savedFood = player.getFoodLevel();
    }
 
    public void staffModeOff() {
        savedPlayer.getInventory().setContents(savedContents);
        savedPlayer.teleport(savedLocation);
        savedPlayer.setGameMode(savedGamemode);
        savedPlayer.setHealth(savedHealth);
        savedPlayer.setFoodLevel(savedFood);
    }
 
    public void staffModeOn() {
        savedPlayer.getInventory().clear();
        savedPlayer.setHealth(20);
        savedPlayer.setFoodLevel(20);
        savedPlayer.setGameMode(GameMode.CREATIVE);
        PlayerInventory playerInventory = savedPlayer.getInventory();
        playerInventory.addItem(ItemLib.giveItemMeta(Material.COMPASS, ChatColor.GOLD + "AdminCompass", Collections.singletonList(ChatColor.GOLD + "Use this to teleport around")));
        playerInventory.addItem(ItemLib.giveItemMeta(Material.BED, ChatColor.GOLD + "Player GUI", Collections.singletonList(ChatColor.GOLD + "Right click to get a list of Players")));
        playerInventory.setItem(8, ItemLib.giveItemMeta(Material.PAPER, ChatColor.GOLD + "AdminStatus", Collections.singletonList(ChatColor.GOLD + "AdminMode:" + ChatColor.AQUA + " Active")));
    }
 
    public UUID getUUID() {
        return savedPlayer.getUniqueId();
    }
}
 

How i use it

Code:java

 
        if (commandSender instanceof Player) {
            Player player = (Player) commandSender;
            boolean playerIsInStaffMode = false;
            for (int i = 0; i < staffModeList.size(); i++) {
                Staffmode loadedStaffMode = staffModeList.get(i);
                if (loadedStaffMode.getUUID().equals(player.getUniqueId())) {
                    loadedStaffMode.staffModeOff();
                    staffModeList.remove(i);
                    playerIsInStaffMode = true;
                    player.sendMessage(ChatColor.GOLD+">> "+ChatColor.AQUA+"Staff mode turned off");
                    break;
                }
            }
            if (!playerIsInStaffMode) {
                Staffmode saveStaffMode = new Staffmode(player);
                staffModeList.add(saveStaffMode);
                saveStaffMode.staffModeOn();
                player.sendMessage(ChatColor.GOLD+">> "+ChatColor.AQUA+"Staff mode turned on");
            }
        }
 

 

@JanTuck
Please use the Insert -> Code option. Paste it there. Then just change the tags from code to syntax. This way the indentation is correct.

 

@I Al Istannen
Happy?

 

Formatting is horrible, sorry.

 

@DoggyCode™
Better? I use Intellij's automatic reformat.

 

@JanTuck
Apart from a few naming convention misses, the class looks good to me.

 

@JanTuck

I would recommend you change the names of all these variables. They are very descriptive, and it is hard to fully understand what all the variables are.

Instead of doing this, break out of the for loop.

This bit has been duplicated twice. If this is in your actual code (which I doubt since this would have errors in it), then remove this duplicated bit of code.

Besides this, it looks like it should work. Nothing stands out as being inefficient.

 

@JanTuck
Much better.

• That should be equals.
• Make the variables private
• Follow naming convetions (e.g. "LastLoc").
  
• Make a defensive copy for the mutable passed values. I think this is also true for the passed Array
  
• Follow naming conventions "ItemMeta" is a method.
• Use varargs for the lore in your ItemLib class. This way you don't need to construct a new list everytime
• Maybe just use Collections.singletonList(<string>) if you really want to pass a list?
• You save a player instance. Be sure to null it when the player leaves. Or just store the UUID instead.

EDIT: And "staffmodeon" should be "staffModeOn" or something.

 

What is it fixed in above?

@I Al Istannen
is that good?

EDIT by Moderator: merged posts, please use the edit button instead of double posting.

 

A LOT BETTER

 

@Zombie_Striker @I Al Istannen

What about now? Good enough?