Optymalizacja kodu

0

Witam. Posiadam tu taki kod pluginu do bukkita.
Czy można go jeszcze bardziej "zoptymalizować"?
pomniejszyć? Coś jest zbędne? I czy się nadaje na wersję finalną.

 package me.przemovi.pandora;
 
import java.util.ArrayList;
import java.util.List;
import org.apache.commons.lang.StringUtils;
import org.bukkit.ChatColor;
import org.bukkit.Material;
import org.bukkit.command.Command;
import org.bukkit.command.CommandSender;
import org.bukkit.enchantments.Enchantment;
import org.bukkit.entity.Monster;
import org.bukkit.entity.Player;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;
import org.bukkit.event.block.Action;
import org.bukkit.event.entity.EntityDeathEvent;
import org.bukkit.event.player.PlayerInteractEvent;
import org.bukkit.inventory.ItemStack;
import org.bukkit.inventory.meta.ItemMeta;
import org.bukkit.plugin.java.JavaPlugin;
 
public class Main extends JavaPlugin implements Listener{
    private List<String> pandrop; //zawiera pobraną z configu listę "pandrop" (lista itemów)
    private List<String> pandoraitemlore; //zawiera pobraną z configu listę opisów skrzynki pandory "apndoraitemlore"
    ArrayList<ItemStack> drops = new ArrayList<ItemStack>(); //lista itemów dodanych do dropów moba
    //ArrayList<ItemStack> pdrops = new ArrayList<ItemStack>();
    ArrayList<String> lores = new ArrayList<String>(); //lista 
 
    String[] pitemlore;
    String[] pdrop;
    int pitemlorelenght;
    int pdroplenght;
    String idmeta;
    String Samount;
    String RawPItemName;
    String PItemName;
    String idimeta[];
    int id;
    int amount;
    //Short shrt = Short.valueOf(idimeta[1]);
    Short shrt;
 
    @Override
    public void onEnable() {
        getServer().getPluginManager().registerEvents(this, this);
        reloadConfig();
        getConfig().options().copyDefaults(true);
        saveDefaultConfig();
 
        getLogger().info("playerdrop " + getDescription().getVersion() + " enabled");
        load();
    }
    @Override
    public void onDisable() {
        getLogger().info("Pandora disabled");
    }
    @Override
    public boolean onCommand(CommandSender sender, Command cmd, String label, String[] args) {
        if (cmd.getName().equalsIgnoreCase("pandora")) {
          if (args.length > 0) {
            if (!sender.hasPermission("pandora.reload")) {
              sender.sendMessage(ChatColor.RED + "Nie masz uprawnień do użycia tej komendy!");
              return true;
            }
            if (args[0].equalsIgnoreCase("reload")) {
              if (!sender.hasPermission("pandora.reload")) {
                sender.sendMessage(ChatColor.RED + "Nie masz uprawnień do użycia tej komendy!");
                return true;
              }
              reloadConfig();
              load();
              sender.sendMessage(ChatColor.GREEN + "Konfiguracja Pandory została przeładowana!");
              return true;
            }
          } else {
            sender.sendMessage(ChatColor.AQUA+"---------- Pandora ----------");
            sender.sendMessage(ChatColor.AQUA+"/pandora reload");
            sender.sendMessage(ChatColor.AQUA+"Pandora v1.2 by PrzemoVi");
            return true;
          }
        }
        return false;
      }
    @SuppressWarnings("deprecation")
    @EventHandler
    public void onEntityDeath(EntityDeathEvent e) {
        Player killer = e.getEntity().getKiller();
        if(killer != null) {
            /*
            String[] pitem = pandoraitemlore.toArray(new String[0]);
            int pitemlorelenght =  pandoraitemlore.size();
            String idmeta = getConfig().getString("pandoraitem");
            String Samount = getConfig().getString("pandoraitemamount");
            String RawPItemName = getConfig().getString("pandoraitemname");
            String PItemName = ChatColor.translateAlternateColorCodes('&', RawPItemName.replace("_", " "));
            String idimeta[] = idmeta.split(":");
            int id = Integer.parseInt(idimeta[0]);
            int amount = Integer.parseInt(Samount);
            //Short shrt = Short.valueOf(idimeta[1]);
            Short shrt = 0;
            if(indexExists(idimeta, 1)) {
                shrt = Short.valueOf(idimeta[1]);
            }
            */
            if (e.getEntity() instanceof Monster) {
                //Random random = new Random();
                //double chance = this.getConfig().getDouble("pandoraMonsterDropChance",50)/100;
                //if(random.nextDouble() <= chance){
            	double chance = getConfig().getDouble("pandoraMonsterDropChance")/100;
            	double d = Math.random();
            	if (d < chance) {
                    ItemStack is = new ItemStack(id, amount, shrt);
                    ItemMeta im = is.getItemMeta();
                    im.setDisplayName(PItemName);
                    int x = 0;
                    lores.clear();
                    while(pitemlorelenght != x){
                        String Lore = pitemlore[x];
                        lores.add(Lore);
                        x++;
                    }
                    im.setLore(lores);
                    is.setItemMeta(im);
                    drops.add(is);
                    if(x == pandoraitemlore.size()){
                        e.getDrops().addAll(drops);
                        x = 0;
                        drops.clear();
                    }
                }
            } //danooct1.com
            if (e.getEntity() instanceof Player) {
                //Random random = new Random();
                //double chance = this.getConfig().getDouble("pandoraPlayerDropChance",50)/100;
                //if(random.nextDouble() <= chance){
            	double chance = this.getConfig().getDouble("pandoraPlayerDropChance")/100;
            	double d = Math.random();
            	if (d < chance) {
                    ItemStack is = new ItemStack(id, amount, shrt);
                    ItemMeta im = is.getItemMeta();
                    im.setDisplayName(PItemName);
                    int x = 0;
                    lores.clear();
                    while(pitemlorelenght != x){
                        String Lore = pitemlore[x];
                        lores.add(Lore);
                        x++;
                    }
                    im.setLore(lores);
                    is.setItemMeta(im);
                    drops.add(is);
                    if(x == pandoraitemlore.size()){
                        e.getDrops().addAll(drops);
                        x = 0;
                        drops.clear();
                    }
                }
            }
        }
    }
    @SuppressWarnings("deprecation")
    @EventHandler
    public void onPlayerInteractEvent(PlayerInteractEvent e){
        if(e.getAction() == Action.LEFT_CLICK_AIR 
                || e.getAction() == Action.RIGHT_CLICK_AIR
                || e.getAction() == Action.LEFT_CLICK_BLOCK
                || e.getAction() == Action.RIGHT_CLICK_BLOCK){
            Player player = e.getPlayer();
            Material MAT = Material.getMaterial(id);
            if(player.getItemInHand() != null && player.getItemInHand().getType() == MAT){
                ItemStack iteminhand = player.getItemInHand();
                ItemMeta metainhand = iteminhand.getItemMeta();
                String inhandname = metainhand.getDisplayName();
                //if(inhandname == PItemName){
                if(inhandname.equals(PItemName)) {
                    //tu kod wykonywany po kliknięciu trzymając dany item...
                    int i = 0;
                      //int moblistlenght = playeritems.size();
                      while(i < pdroplenght){
                                String midsench[] = pdrop[i].split(" ");
                                String idsid = midsench[0];
                                String idsidpod[] = idsid.split(":");
                                int amount = Integer.parseInt(midsench[1]);
                                int id = Integer.parseInt(idsidpod[0]);
                                Short shrt = 0;
                                if(indexExists(idsidpod, 1)) {
                                    shrt = Short.valueOf(idsidpod[1]);
                                }
                                ItemStack is = new ItemStack(id, amount, shrt);
 
                                if(indexExists(midsench, 2)) {
                                    if(!("null".equals(midsench[2]))) {
                                        if(midsench[2].indexOf(",") != -1) {
                                            String LOEchs[] = midsench[2].split(",");
                                            int k = 0;
                                            while(indexExists(LOEchs, k)) {
                                                String Enchant[] = LOEchs[k].split(":");
                                                int enchantLVL = Integer.parseInt(Enchant[1]);
                                                Enchantment enchantTYPE = Enchantment.getByName(Enchant[0]);
                                                is.addUnsafeEnchantment(enchantTYPE, enchantLVL);
                                                k++;
                                            }
                                        } else {
                                            String Enchant[] = midsench[2].split(":");
                                            int enchantLVL = Integer.parseInt(Enchant[1]); //Out of bounds error
                                            Enchantment enchantTYPE = Enchantment.getByName(Enchant[0]);
                                            is.addUnsafeEnchantment(enchantTYPE, enchantLVL);
                                        }
                                    }
                                }
                                if(indexExists(midsench, 3)){
                                    ItemMeta im = is.getItemMeta();
                                    String DisplayName = ChatColor.translateAlternateColorCodes('&', midsench[3].replace("_", " "));
                                    im.setDisplayName(DisplayName);
                                    if(indexExists(midsench, 4)){
                                        int count = StringUtils.countMatches(midsench[4], ",");
                                        if(count >= 1){
                                            lores.clear();
                                            String PodzieloneLore[] = midsench[4].split(",");
                                            int licznik = count+1;
                                            int licznik2 = 0;
                                            while(licznik !=0){
                                                String Lore = ChatColor.translateAlternateColorCodes('&', PodzieloneLore[licznik2].replace("_", " "));
                                                lores.add(Lore);
                                                licznik = licznik-1;
                                                licznik2++;
                                            }
                                            im.setLore(lores);
                                        } else {
                                            lores.clear();
                                            lores.add(midsench[4]);
                                            im.setLore(lores);
                                        }
                                    }
                                    is.setItemMeta(im);
                                }
                                //drops.add(is);
                                player.getInventory().addItem(is);
                                i++;
                      }
                      if(i == pdroplenght){
                        //drops.add(new ItemStack(id, amount, shrt));
                            //e.getDrops().addAll(drops);
                            i = 0;
                            //drops.clear();
                      }
                }
            }
        }
    }
    public boolean indexExists(String[] array,int index){
        if(array!=null && index >= 0 && index < array.length)
            return true;
        else 
           return false;
    }
    public boolean load() {
    	pandrop = getConfig().getStringList("pandrop");
        pandoraitemlore = getConfig().getStringList("pandoraitemlore");
 
        pdrop = pandrop.toArray(new String[0]);
        pdroplenght =  pandrop.size();
        pitemlore = pandoraitemlore.toArray(new String[0]);
        pitemlorelenght =  pandoraitemlore.size();
        idmeta = getConfig().getString("pandoraitem");
        Samount = getConfig().getString("pandoraitemamount");
        RawPItemName = getConfig().getString("pandoraitemname");
        PItemName = ChatColor.translateAlternateColorCodes('&', RawPItemName.replace("_", " "));
        String idimeta[] = idmeta.split(":");
        id = Integer.parseInt(idimeta[0]);
        amount = Integer.parseInt(Samount);
        //Short shrt = Short.valueOf(idimeta[1]);
        shrt = 0;
        if(indexExists(idimeta, 1)) {
            shrt = Short.valueOf(idimeta[1]);
        }
        return true;
    }
}
0

To bym zmienił

public boolean indexExists(String[] array,int index){
        if(array!=null && index >= 0 && index < array.length)
            return true;
        else 
           return false;
}

na

public boolean indexExists(String[] array,int index){
	return array!=null && index >= 0 && index < array.length;
}

To mi się nie podoba: public boolean load() { [...] return true; -> dlaczego zawsze zwraca true?
Przeglądam dalej (coś tak od końca zacząłem :P)

Dalej:
Masz niektóre ify zagnieżdżone, gdzie mogłyby być razem, np. w metodzie onPlayerInteractEvent:

if(indexExists(midsench, 2)) {
	if(!("null".equals(midsench[2]))) {

zamieniłbym na

if (index[Exists(midsench, 2) && (!("null".equals(midsench[2]))) {

I rozważ podzielenie kodu na lekko więcej metod - byłby troszkę bardziej czytelniejszy i nie musiałbyś się powtarzać, np. przy onEntityDeath sprawdź tylko jakiej jest instancji (Player/Monster) i wywołuj inną metodę z różnymi parametrami dla Player i innymi dla Monster bo sporo kodu w tych ifach się dubluje.

Pozdrawiam ;)

1

Nie no bez żartów. Ten kod to by trzeba było napisać od nowa ;] Po pierwsze nie pakować wszystkiego do jednej biednej klasy. Po drugie używać polimorfizmu a nie instanceof. Po trzecie metody krósze niż 15 linijek, nazwy zmiennych które coś znaczą a nie licznik1, licznik2. Po czwarte nie mieszać w kodzie polskiego i angielskiego. Po piąte nie używać gołych tablic. Ten kod to jest dramat trochę. Jakbyś puścił na nim checkstyle/findbugs/sonar albo inspect code z intellij to by ci wszystko zaświeciło na żółto i czerwono. Myśle że raptem kilka linijek by się ostało ;]

0

Naprawdę napisz to od nowa. Dużo się nauczysz przepisując ten kod od początku zgodnie z zasadami SOLID DRY i KISS.

Edit: Usunięte nic nie wnoszące opinie na temat kodu.

0
  • indexExists zło w czystej postaci. Zastanów się co ta metoda robi i co zwraca. Instrukcja if nie jest potrzebna. Sam warunek wystarczy.
  • onEntityDeath zamiast instansof zastosuj polimorfizm. Niech każdy rodzaj zabójcy ma własną metodę. Kod będzie czytelniejszy.
  • onCommand ifologia, to zło. najprościej jest pociąć ten kod na mniejsze metody i użyć polimorficznego wywołania. Jedna komenda, jedna metoda. Sprawdzanie za pomocą ifa i nazwy jest nawet gorsze niż instanceof.
  • onPlayerInteractEvent podziel na mniejsze fragmenty, bo tego nie da się zanalizować w rozsądnym czasie.

Wołam @jarekr000000, tylko bądź delikatny :)

0

Widziałem - ale się autopoddałem. Za dużo.
Ale dodając cokolwiek:
lepiej by było wydzielić metoda load() powinna inicjalizować (konstruktorem) nowy obiekt (z nowej klasy)
tam warto by przenieść te pola i je podładować ( w konstruktorze)

String[] pitemlore;
    String[] pdrop;
    int pitemlorelenght;
    int pdroplenght;
    String idmeta;
    String Samount;
    String RawPItemName;
    String PItemName;
    String idimeta[];

i w tej klasie dopiero działać - (na nim zrobić registerEvents( ;
Trochę by się oczyściło.

Off topic.
Przy okazji nie wiek skąd pomysł, że miałbym być niedelikatny. Przecież ja jako konsultant pracowałem i musiałem się nauczyć "poker face" na widok kodu pokazywanego przez lokalnych "seniorów" ( i to jeszcze z dumą). (Mnie tylko "niemieccy" architekci wyprowadzają z równowagi, ale to też muszę opanować. Gotuje się i wybucham ile razy słyszę "generator kodu").

0

o co chodzi z "niemieckimi programistami" , kolejny raz spotykam sie z śmieszkami na ich temat, moglbys rozwinac temat?

2

Te "niemieccy" to taki mój stereotyp i nie odnosi się do narodowości tylko do stylu (jest dużo polskich "niemieckich" architekci). A w samych Niemczech raczej margines wśród architektów.
Jeśli miałbym główne cechy wymienić

  • miłość do JSF,
  • miłość do UML,
  • pisanie generatorów kodu,
  • ogólnie stałe wpadanie w "inner platform efekt" (czyli np. baza relacyjna w bazie relacyjnej, albo angular w angularze),
  • javadoc i komentarze do zarypania: przy getterach i setterach głownie,
  • metody na 50 linii - ale "porządnie skomentowane",
  • DTO, CTO, ETO, DBO, DAO, BO ..bla bla bla- czyli enterprise wersja "encja na twarz",
  • i wielkie przekonanie, że to jest właśnie clean code :-)

(A nazwy używam - bo kojarzy mi się z tzw. "niemieckim porządkiem" (czyli w praktyce chaosem dobrze ukrytym pod dywanem)).

0

Ok, dzięki, za wszystkie rady, już większość zastosowałem, ale co to jest "polimorfizm"?

0
  1. dwa poziomy zagnieżdżenia if to maks co powinno być w kodzie, u Ciebie jest:
onPlayerInteractEvent > if > if > while > if > if > if > while
  1. nie usunąłeś zakomentowanego kodu
  2. nie stosujesz się do standardu nazewnictwa stosowanego w Javie (np. String DisplayName): http://www.oracle.com/technetwork/java/codeconventions-135099.html
0

Faktycznie, jak po miesiącu wróciłem do analizy kodu to doszedłem do wniosku... Że podzielę to na kilka klas, bo nie mogę teraz nic z tego zrozumieć. Może nawet warto się zastanowić nad przepisaniem kodu od nowa... Tak... Zdecydowanie tak... Dzięki za wszystkie rady, wszystko się przyda. Jak ma ktoś jeszcze jakieś pomysły to możecie się dopisać. Dzięki i Pozdrawiam

0

Nie, no kurde, przeczytałem kilkanaście tutorialów dotyczących poliforizmu i wiele przykładów i ciągle nie mogę tego zrozumieć :/ Mógłbym prosić o jakiś przykład? Najlepiej, na moim kodzie.

Pozdrawiam
PrzemoVi

1 użytkowników online, w tym zalogowanych: 0, gości: 1