Thread: Searching for code criticism. (Asteria, Slayer class)

Results 1 to 3 of 3
  1. #1 Searching for code criticism. (Asteria, Slayer class) 
    Registered Member
    CTucker's Avatar
    Join Date
    Oct 2008
    Posts
    2,422
    Thanks given
    263
    Thanks received
    281
    Rep Power
    343
    Lately I've been reading over Major's thread Writing better Java and trying to improve my bad habits I picked up, I've just started school and should get into the more advanced things with programming here soon, but as of right now I'm stuck in pre-req classes, working with things such as Microsoft Excel, etc. So, I've been using online material and other people to help me increase my knowledge.

    I'm looking for criticism, with helpful information, comments like "This is bad" don't help at all, please explain why it's bad, or what I shouldn't do what I have done. If there's a better way to do it, if so what it is. Links to reference are always nice. I rewrote this class after Major telling me to use an Optional<T> instead of setting a variable to null, this lead me to do a little research on it. (I noticed then later that he posted about it in his thread I posted above, but I missed it.) I was confused about something, so I searched for help on StackOverflow and got the answers that I required there.

    Here's the class, it's not commented, sorry.

    Slayer.java

    Code:
    package com.asteria.world.entity.player.skill.impl;
    
    import java.util.ArrayList;
    import java.util.Arrays;
    import java.util.List;
    
    import com.asteria.util.Utility;
    import com.asteria.world.entity.player.Player;
    import com.asteria.world.entity.player.skill.Skills;
    
    public class Slayer {
    
    	public enum Difficulty { 
    		EASY, MEDIUM, HARD
    	}
    	
    	public enum SlayerTask {
    		MAN(1, 1, Difficulty.EASY, "Found south of home area.", 1);
    		
    		private int taskId;
    		private int levelRequired;
    		private Difficulty difficulty;
    		private String location;
    		private int npcId;
    		
    		SlayerTask(int taskId, int levelRequired, Difficulty difficulty, String location, int npcId) {
    			this.taskId = taskId;
    			this.levelRequired = levelRequired;
    			this.difficulty = difficulty;
    			this.location = location;
    			this.npcId = npcId;
    		}
    		
    		public int getTaskId() {
    			return taskId;
    		}
    		
    		public int getLevelRequired() {
    			return levelRequired;
    		}
    		
    		public Difficulty getDifficulty() {
    			return difficulty;
    		}
    
    		public String getLocation() {
    			return location;
    		}
    		
    		public int getNpcId() {
    			return npcId;
    		}
    
    		public static List<SlayerTask> byType(Player player, Difficulty difficulty) {
    			List<SlayerTask> set = new ArrayList<>();
    			Arrays.stream(values()).filter(task -> (task.getDifficulty() == difficulty) 
    						&& (task.getTaskId() != 0)
    						&& (player.getSkills()[Skills.SLAYER].getLevel() >= task.getLevelRequired())
    				).forEach(task -> set.add(task));
    			
    			return set;
    		}
    		
    		public static List<SlayerTask> getAll() {
    			ArrayList<SlayerTask> list;
    			(list = new ArrayList<>(Arrays.asList(SlayerTask.values()))).remove(0);
    			return list;
    		}
    		
    	}
    	
    	public static SlayerTask getRandomTask(Player player) {
    		List<SlayerTask> availableTasks = SlayerTask.getAll();
    		int index = Utility.RANDOM.nextInt(availableTasks.size());
    		while(player.getSkills()[Skills.SLAYER].getLevel() < availableTasks.get(index).getLevelRequired()) {
    			index = Utility.RANDOM.nextInt(availableTasks.size());
    		}
    		return availableTasks.get(index);
    	}
    	
    	public static SlayerTask getTask(Player player, Difficulty difficulty) {
    		List<SlayerTask> availableTasks = SlayerTask.byType(player, difficulty);
    		return availableTasks.get(Utility.RANDOM.nextInt(availableTasks.size()));
    	}
    	
    	public static void appendSlayerExperience(Player player, int npcHealth) {
    		Skills.experience(player, (npcHealth * 8), Skills.SLAYER);
    	}
    	
    	
    }
    The use of Optional<T> is in Player.java

    Code:
        public Optional<SlayerTask> getSlayerTask() {
        	return slayerTask;
        }
        
        public void setSlayerTask(Optional<SlayerTask> slayerTask) {
        	this.slayerTask = slayerTask;
        }
        
        public int getSlayerKills() {
        	return slayerKills;
        }
        
        public void setSlayerKills(int slayerKills) {
        	this.slayerKills = slayerKills;
        }
        
        public void decrementSlayerKills() {
        	this.slayerKills--;
        	if(slayerKills < 1) {
        		getPacketBuilder().sendMessage("You have completed your slayer task.");
        		setSlayerTask(Optional.empty());
        	}
        }
    Which is used like so in the NpcDeath class

    Code:
            killer.ifPresent(k -> k.getSlayerTask().ifPresent(t-> {
            	if(t.getNpcId() == entity.getNpcId()) {
            		k.decrementSlayerKills();
            		Slayer.appendSlayerExperience(k, entity.getMaxHealth());
            	}
            }));

    Is this the correct usage of an Optional<T>?
    Reply With Quote  
     

  2. #2  
    Banned Searching for code criticism. (Asteria, Slayer class) Market Banned


    Join Date
    Jan 2011
    Age
    26
    Posts
    3,112
    Thanks given
    1,198
    Thanks received
    1,479
    Rep Power
    0
    Code:
    		private int taskId;
    		private int levelRequired;
    		private Difficulty difficulty;
    		private String location;
    		private int npcId;
    should all be final


    Code:
    		public static List<SlayerTask> byType(Player player, Difficulty difficulty) {
    			return Arrays.stream(SlayerTask.values()).filter(task -> (task.getDifficulty() == difficulty) 
    						&& (task.getTaskId() != 0)
    						&& (player.getSkills()[Skills.SLAYER].getLevel() >= task.getLevelRequired())
    				).collect(Collectors.toList());
    		}
    is much better looking, utilize collectors

    Code:
    		public static List<SlayerTask> getAll() {
    			ArrayList<SlayerTask> list;
    			(list = new ArrayList<>(Arrays.asList(SlayerTask.values()))).remove(0);
    			return list;
    		}
    not sure what you're trying to do here

    Code:
    	public static SlayerTask getRandomTask(Player player , Difficulty difficulty) {
                    List<SlayerTask> list = SlayerTask.byType(player, difficulty);
                    list.remove(player.getLastTask()); // So we don't get the same task twice.
                     
                    return list.((int) (Math.random() * list.size()));
    	}
    the original code you posted was way more complicated than it needed to be



    otherwise looking good bro
    Reply With Quote  
     

  3. #3  
    Registered Member

    Join Date
    Nov 2014
    Posts
    253
    Thanks given
    39
    Thanks received
    146
    Rep Power
    248
    Well first you could do so much more with that difficulty enum, it could store the minimum and maximum amount of the task you get at that difficulty or even the slayer levels you must be around to get that task (so people with 99 slayer don't get men as a slayer task)

    Also, you have a ton of static methods in the slayer class in which you pass in player, this is a good indication that you should probably instantiate the player class inside the Player consructor and store Slayer related variables inside the Slayer class

    I.e. player.getSlayer().getSlayerPoints(); in which you can store slayerPoints, kills left, AND slayertask (which you'd save by index - plus you could remove all those getters inside SlayerTask). I mean just looking at it logically "slayerKills" sure is a member of Player but it is also a member of Slayer and can be grouped together with other slayer variables (also can be modified locally without all these getters (personally i'd split the enum and class) - i.e. you're having to do things like player.setSlayerKills when you really cold just be doing player.getSlayer().handleKill(npc) and it can do all the great logic stuff for you (also with easy access to all relevant variables - player's task, killAmount ect)

    Code:
    public class SlayerHolder {
    
        private int taskAmount, totalTasks, slayerPoints;
        /**
         * SlayerHolder that they have, save the index
         */
        private SlayerTask task;
    
        public boolean assignTask(final int slayerLevel) {
            if(taskAmount > 0)
                return false;
            task = SlayerTask.forLevel(slayerLevel);
            taskAmount = task.getDifficulty().getAmount();
            return true;
        }
    
        public int killedTask(final int npcid) {
            if(task == null) return 0;
            if(isTask(npcid)) {
                if(--taskAmount == 0) {
                    totalTasks++;
                    slayerPoints += task.getDifficulty().getSlayerPoints() + handleTotalTasks();
                }
                return task.getXP();
            }
            return 0;
        }
    
        public boolean isTask(final int npcId) {
            return task != null && task.getIds().contains(npcId) && taskAmount > 0;
        }
    
    
        public boolean resetTask() {
            if(slayerPoints < 20)
                return false;
            slayerPoints -= 20;
            taskAmount = 0;
            return true;
        }
    //omitted
    and something like

    Code:
    public enum SlayerTask {
    
        /** The elite tasks */
        GENERAL_GRAARDOR(27, Difficulty.ELITE, 1, 1100, 6260),
    
        /** The hard tasks */
        BLACK_DEMON(4,Difficulty.HARD, 1, 170,
                84),
        GORAK(5, Difficulty.HARD, 68, 120,
                6218),
    //omitted
        private final Difficulty difficulty;
        private final int slayerLevel, slayerXP, index; //might want to base slayerXp on npc combat level since multiple support
        private final List<Integer> ids = new ArrayList<>();
        private static final int EXP_MULTIPLIER = 14;
    
        private SlayerTask(final int index,final Difficulty difficulty, final int slayerLevel, final int slayerXP, final int... ids) {
            this.difficulty = difficulty;
            this.slayerLevel = slayerLevel;
            this.slayerXP = slayerXP * EXP_MULTIPLIER;
            this.index = index;
            for(int i : ids)
                this.ids.add(i
                );
        }
    
        public Difficulty getDifficulty() { return difficulty; }
        public List<Integer> getIds() { return ids; }
        public int getXP() { return slayerXP; }
        public int getId() { return index; }
    
        public static SlayerTask forLevel(final int slayerLevel) {
            final SlayerTask task = values()[Misc.random(values().length - 1)];
            if(slayerLevel >= task.slayerLevel && Math.abs(slayerLevel - task.difficulty.slayerLevel) <= 25) //ensure task is not too easy and they have the level for it
                return task;
            else return forLevel(slayerLevel);
        }
    
        static enum Difficulty {
            EASY(10, 40, 0, 4),
            MEDIUM(20, 80, 26, 6),
            DIFFICULT(40, 100, 50, 7),
            HARD(60, 150, 75, 8),
            ELITE(5, 10, 99, 8);
    
            private final int minAmount, maxAmount, slayerLevel, slayerPoints;
            private Difficulty(final int minAmount, final int maxAmount, final int slayerLevel, final int slayerPoints) {
                this.minAmount = minAmount;
                this.maxAmount = maxAmount;
                this.slayerLevel = slayerLevel;
                this.slayerPoints = slayerPoints;
            }
    
            public int getAmount() {
                return minAmount + Misc.random(maxAmount - minAmount);
            }
    
            public int getSlayerPoints() {
                return slayerPoints;
            }
    
    
        }
    
        @Override public String toString() {
            return TextUtils.titleCase(super.toString().replaceAll("_", " ").toLowerCase());
        }
    //omitted
    But you see how you don't have to actually call any methods from player to accomplish this?

    Now this is the way it was done by me personally (even though I don't support every single piece of code its easier to stick with what's there)

    Code:
        @Override
        public boolean clickObject(final Player player, final int type, final int npcId, final int slot, final int objId, final int a) {
            if(type == ClickType.EAT) { //slayer gem
                player.sendMessage("You have "+player.getSlayer().getTaskAmount()+ " "+player.getSlayer().getTask()+" npcs left to kill",
                        "You have "+player.getSlayer().getSlayerPoints()+" slayer points",
                        "You have completed "+player.getSlayer().getTotalTasks()+ " tasks");
                return true;
            }
            if(type == ClickType.NPC_OPTION1) { // talk to slayer masker
                DialogueManager.openDialogue(player, 174);
                return true;
            }
            if(type == ClickType.NPC_DEATH) {
                int slayerXP = player.getSlayer().killedTask(npcId);
                if(slayerXP > 0) {
                    ContentEntity.addSkillXP(player, slayerXP, Skills.SLAYER);
                    if(player.getSlayer().getTaskAmount() == 0) {
                        player.sendf("You have completed %d tasks in a row and have %d slayer points", player.getSlayer().getTotalTasks(),player.getSlayer().getSlayerPoints());
                    }
                }
                return false;
            }
    //omitted
    Reply With Quote  
     


Thread Information
Users Browsing this Thread

There are currently 1 users browsing this thread. (0 members and 1 guests)


User Tag List

Similar Threads

  1. Replies: 15
    Last Post: 04-02-2013, 04:59 AM
  2. Need help making a search for my website.
    By David in forum Application Development
    Replies: 3
    Last Post: 01-25-2009, 02:09 AM
  3. Searching for Actionbutton's ?
    By Ayton in forum Configuration
    Replies: 5
    Last Post: 10-25-2008, 06:10 PM
  4. Replies: 6
    Last Post: 05-09-2008, 07:07 PM
  5. Searching for the light™
    By Tagger™ in forum Showcase
    Replies: 2
    Last Post: 02-24-2008, 09:26 PM
Posting Permissions
  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •