The Hero of Time Posted December 3, 2016 Share Posted December 3, 2016 (edited) So, to give you a quick run through, i have created a class Fighter, which represents a fighter with a combat style ENUM: COMBAT public enum Combat { MAGIC, MELEE, RANGED } CLASS: FIGHTER public class Fighter { private char initial; private Combat combatStyle; /** * * @param _initial The first initial of the fighter * @param _combatStyle The combat style the fighter mastered */ public Fighter(char _initial, Combat _combatStyle) { initial = _initial; combatStyle = _combatStyle; } public char getInitial() { return initial; } public Combat getCombatStyle() { return combatStyle; } } usage: Fighter combatant = new Fighter('E',Combat.RANGED); Now we have a class barracks, the barracks is full of fighters. The ArrayList inside barracks that represents the barracks is: private ArrayList<Fighter> fighters = new ArrayList<>(); now the class Barracks has a few methods. CLASS: BARRACKS /** * Creates a fighter and puts him in the barracks * @param lr the first initial of the fighter * @param type the combat style the fighter mastered */ public void createFighter(char lr, Combat type) { Fighter r = new Fighter(Character.toUpperCase(lr),type); fighters.add(r); } /** * Creates fighters and puts them into the barracks * @param lr the first initial of the fighter * @param type the combat style the fighter mastered * @param amount the amount of fighters * @[member='Return'] */ public void createFighters(char lr, Combat type, int amount) { for(int i = 0; i < amount; i++) createFighter(lr,type); } now if you have a basic understanding of the classes, the problem i have is with this method. The problem is that the fighters will not be removed from the barracks. They will all be called by a phrase, and the list with calledfighters will be correctly filled, but they just wont be removed from the barracks if there are 2 fighters with the same initial /** * Calls all fighters that have initials that are in the name. Example: name = war. All fighters with initials 'w','a', or 'r' will be called and removed from the barracks. * @param phrase The phrase with initials * @param combatStyle The combat style the fighter is specialized in */ public ArrayList<Fighter> callFighters(String phrase, Combat combatStyle) { //make the phrase fully uppercase phrase = phrase.toUpperCase(); //Used to check if we have enough fighters in our army ArrayList<Fighter> tempBarracks = new ArrayList<>(); //fighters to remove from the barracks after they have been called ArrayList<Fighter> toRemoveFightersFromBarracks = new ArrayList<>(); //list of fighters that are called from the barracks ArrayList<Fighter> calledFighters = new ArrayList<>(); //fill the temporary barracks with the contents real barracks, so that we dont have to modify the real barracks if we call fighters and dont have enough fighters tempBarracks.addAll(fighters); //Fill the arraylist with chars of the parameter name //war will be 'W','A','R' ArrayList<Character> nameInChars = new ArrayList<>(); for(char chr: phrase.toCharArray()) nameInChars.add(chr); //Call all fighters int i = 0; int removedFighters = 0; for(Fighter fgt: tempBarracks) { if (nameInChars.contains(fgt.getInitial()) && fgt.getCombatStyle().equals(combatStyle) && i < nameInChars.size()) { Fighter r = new Fighter(nameInChars.get(i), combatStyle); calledFighters.add(r); for (Fighter fghtr : tempBarracks) { if (fghtr.getInitial() == nameInChars.get(i) && fghtr.getCombatStyle().equals(combatStyle) && removedFighters == 0) { toRemoveFightersFromBarracks.add(fghtr); removedFighters++; } } removedFighters = 0; i++; } } //toRemoveFightersFromBarracks will now be Correctly filled with all the fighters from the given phrase for(Fighter fgt: toRemoveFightersFromBarracks) //HERE LIES THE PROBLEM I'M HAVING. { if(tempBarracks.contains(fgt)) { //Will also be FALSE if indexOfRm = -1 as described below. } int indexOfRm = tempBarracks.indexOf(fgt); //Will be -1 if we call 2 fighters with the same initials and combat style tempBarracks.remove(fgt); //Wont remove object fgt from tempBarracks, probably has something to do with the fact that the index of fgt in tempBarracks is -1 } So basically if there is a second fighter with the same initial, the object fgt from toRemoveFightersFromBarracks will not be removable from tempBarracks, the index of that fighter in tempbarracks is -1, and contains returns false, but it does contain a fighter with that exact initial and that exact combat style I've made a .gif to show it if(calledFighters.size() == phrase.length()) { //Finally: remove the fighters from the barracks! this.fighters.clear(); this.fighters.addAll(tempBarracks); return calledFighters; } else { System.out.println("We dont have enough fighters for this phrase"); return null; } } gif: EDIT: Images instead since this editor is annoying callFighters Edited December 3, 2016 by The Hero of Time Quote Link to comment Share on other sites More sharing options...
KEVzilla Posted December 3, 2016 Share Posted December 3, 2016 can u format Quote Link to comment Share on other sites More sharing options...
Explv Posted December 3, 2016 Share Posted December 3, 2016 (edited) So, to give you a quick run through, i have created a class Fighter, which represents a fighter with a combat style ENUM: COMBAT public enum Combat { MAGIC, MELEE, RANGED } CLASS: FIGHTER public class Fighter { private char initial; private Combat combatStyle; /** * * @param _initial The first initial of the fighter * @param _combatStyle The combat style the fighter mastered */ public Fighter(char _initial, Combat _combatStyle) { initial = _initial; combatStyle = _combatStyle; } public char getInitial() { return initial; } public Combat getCombatStyle() { return combatStyle; } } usage: Fighter combatant = new Fighter('E',Combat.RANGED); Now we have a class barracks, the barracks is full of fighters. The ArrayList inside barracks that represents the barracks is: private ArrayList<Fighter> fighters = new ArrayList<>(); now the class Barracks has a few methods. CLASS: BARRACKS /** * Creates a fighter and puts him in the barracks * @param lr the first initial of the fighter * @param type the combat style the fighter mastered */ public void createFighter(char lr, Combat type) { Fighter r = new Fighter(Character.toUpperCase(lr),type); fighters.add(r); } /** * Creates fighters and puts them into the barracks * @param lr the first initial of the fighter * @param type the combat style the fighter mastered * @param amount the amount of fighters * @[member='Return'] */ public void createFighters(char lr, Combat type, int amount) { for(int i = 0; i < amount; i++) createFighter(lr,type); } now if you have a basic understanding of the classes, the problem i have is with this method. The problem is that the fighters will not be removed from the barracks. They will all be called by a phrase, and the list with calledfighters will be correctly filled, but they just wont be removed from the barracks if there are 2 fighters with the same initial /** * Calls all fighters that have initials that are in the name. Example: name = war. All fighters with initials 'w','a', or 'r' will be called and removed from the barracks. * @param phrase The phrase with initials * @param combatStyle The combat style the fighter is specialized in */ public ArrayList<Fighter> callFighters(String phrase, Combat combatStyle) { //make the phrase fully uppercase phrase = phrase.toUpperCase(); //Used to check if we have enough fighters in our army ArrayList<Fighter> tempBarracks = new ArrayList<>(); //fighters to remove from the barracks after they have been called ArrayList<Fighter> toRemoveFightersFromBarracks = new ArrayList<>(); //list of fighters that are called from the barracks ArrayList<Fighter> calledFighters = new ArrayList<>(); //fill the temporary barracks with the contents real barracks, so that we dont have to modify the real barracks if we call fighters and dont have enough fighters tempBarracks.addAll(fighters); //Fill the arraylist with chars of the parameter name //war will be 'W','A','R' ArrayList<Character> nameInChars = new ArrayList<>(); for(char chr: phrase.toCharArray()) nameInChars.add(chr); //Call all fighters int i = 0; int removedFighters = 0; for(Fighter fgt: tempBarracks) { if (nameInChars.contains(fgt.getInitial()) && fgt.getCombatStyle().equals(combatStyle) && i < nameInChars.size()) { Fighter r = new Fighter(nameInChars.get(i), combatStyle); calledFighters.add(r); for (Fighter fghtr : tempBarracks) { if (fghtr.getInitial() == nameInChars.get(i) && fghtr.getCombatStyle().equals(combatStyle) && removedFighters == 0) { toRemoveFightersFromBarracks.add(fghtr); removedFighters++; } } removedFighters = 0; i++; } } //toRemoveFightersFromBarracks will now be Correctly filled with all the fighters from the given phrase for(Fighter fgt: toRemoveFightersFromBarracks) //HERE LIES THE PROBLEM I'M HAVING. { if(tempBarracks.contains(fgt)) { //Will also be FALSE if indexOfRm = -1 as described below. } int indexOfRm = tempBarracks.indexOf(fgt); //Will be -1 if we call 2 fighters with the same initials and combat style tempBarracks.remove(fgt); //Wont remove object fgt from tempBarracks, probably has something to do with the fact that the index of fgt in tempBarracks is -1 So basically if there is a second fighter with the same initial, the object fgt from toRemoveFightersFromBarracks will not be removable from tempBarracks, the index of that fighter in tempbarracks is -1, and contains returns false, but it does contain a fighter with that exact initial and that exact combat style I've made a .gif to show it } if(calledFighters.size() == phrase.length()) { //Finally: remove the fighters from the barracks! this.fighters.clear(); this.fighters.addAll(tempBarracks); return calledFighters; } else { System.out.println("We dont have enough fighters for this phrase"); return null; } } gif: If I understand you correctly, you are trying to write a method that gets all the fighters that have any of the initials in a String, and the same Combat style, and you want to remove those from the barracks and return them as a new list? If so, why not get rid of all those Lists and just use an iterator: private List<Fighter> callFighters(String initial, Combat combat) { String upperCaseInitial = initial.toUpperCase(); List<Fighter> calledFighters = new ArrayList<>(); for (Iterator<Fighter> fighterIterator = barracks.iterator(); fighterIterator.hasNext();) { Fighter fighter = fighterIterator.next(); if (upperCaseInitial.contains(Character.toString(fighter.getInitial())) && fighter.getCombatStyle() == combat) { calledFighters.add(fighter); fighterIterator.remove(); } } return calledFighters; } Edited December 3, 2016 by Explv Quote Link to comment Share on other sites More sharing options...
The Hero of Time Posted December 3, 2016 Author Share Posted December 3, 2016 can u format yeah i reformatted it, ipb editor is annoying as fk If I understand you correctly, you are trying to write a method that gets all the fighters that have any of the initials in a String, and the same Combat style, and you want to remove those from the barracks and return them as a new list? If so, why not get rid of all those Lists and just use an iterator: private List<Fighter> callFighters(String initial, Combat combat) { String upperCaseInitial = initial.toUpperCase(); List<Fighter> calledFighters = new ArrayList<>(); for (Iterator<Fighter> fighterIterator = barracks.iterator(); fighterIterator.hasNext();) { Fighter fighter = fighterIterator.next(); if (upperCaseInitial.contains(Character.toString(fighter.getInitial())) && fighter.getCombatStyle() == combat) { calledFighters.add(fighter); fighterIterator.remove(); } } return calledFighters; } Because i'm trying to learn from this and understand what's causing this weird problem Quote Link to comment Share on other sites More sharing options...
Tom Posted December 3, 2016 Share Posted December 3, 2016 indent or no help Quote Link to comment Share on other sites More sharing options...
The Hero of Time Posted December 3, 2016 Author Share Posted December 3, 2016 indent or no help created an image instead, easier to read Quote Link to comment Share on other sites More sharing options...
Tom Posted December 3, 2016 Share Posted December 3, 2016 (edited) created an image instead, easier to read nvm This is the problem in my eyes, not only does it look like shit, but its where your mistake lies. You are looping through every Fighter again for every fighter,, and then checking if removedFighter == 0 If it is == 0, then you are adding 1 to it, making the rest of the loop pointless, as its now equals 1, and it wont add any more fighters to the remove list. edited just change to for(Fighter fgt : tempBarracks){ if(nameInChars.contains(fgt.getInitial()) && fgt.getCombatStyle.equals(combatStyle) && i < nameInChars.size()){ calledFighters.add(new Fighter(nameInChars.get(i), combatStyle)); if(fgt.getInitial() == nameInChars.get(i)){ toRemoveFightersFromBarracks.add(fgt); } } i++; } Edited December 3, 2016 by Tom 1 Quote Link to comment Share on other sites More sharing options...
The Hero of Time Posted December 3, 2016 Author Share Posted December 3, 2016 (edited) nvm This is the problem in my eyes, not only does it look like shit, but its where your mistake lies. You are looping through every Fighter again for every fighter,, and then checking if removedFighter == 0 If it is == 0, then you are adding 1 to it, making the rest of the loop pointless, as its now equals 1, and it wont add any more fighters to the remove list. edited just change to for(Fighter fgt : tempBarracks){ if(nameInChars.contains(fgt.getInitial()) && fgt.getCombatStyle.equals(combatStyle) && i < nameInChars.size()){ calledFighters.add(new Fighter(nameInChars.get(i), combatStyle)); if(fgt.getInitial() == nameInChars.get(i)){ toRemoveFightersFromBarracks.add(fgt); } } i++; } No, that's not the problem. the list will be filled correctly. The reason why it looks like shit is because i've tried cleaner methods with less code, but none of them work. in every scenario the list will be filled correctly, but when i try to remove fighters from the barracks from the contents of toRemoveFightersFromBarracks, it will never work with duplicates the reason why i posted this topic is to understand what the problem is exactly. also with your code the toremove list contains 2 'W' 's and mine contains warw which is correct ( barracks.callFighters("warw", Combat.RANGED); ) Edited December 3, 2016 by The Hero of Time Quote Link to comment Share on other sites More sharing options...
Drkohler Posted December 3, 2016 Share Posted December 3, 2016 No, that's not the problem. the list will be filled correctly. The reason why it looks like shit is because i've tried cleaner methods with less code, but none of them work. in every scenario the list will be filled correctly, but when i try to remove fighters from the barracks from the contents of toRemoveFightersFromBarracks, it will never work with duplicates the reason why i posted this topic is to understand what the problem is exactly. also with your code the toremove list contains 2 'W' 's and mine contains warw which is correct ( barracks.callFighters("warw", Combat.RANGED); ) Have you ever thought about using a hashmap? Quote Link to comment Share on other sites More sharing options...
Lemons Posted December 3, 2016 Share Posted December 3, 2016 The problem you are facing is you are comparing fighters by their initials and combat level. If you add two "a" fighters with same combat, when you go through your loop of fighters in the second for loop (the one nested in the first), you're comparing the initial and Combat enums. This means, it will always pick the first Fighter in the Barracks with those initials/Combat enum, and will think it needs to remove that specific Fighter from the Barracks twice. This results in the -1 you are seeing, as the second time around the Fighter is already gone. If you wanna stick with your current solution, simply replace: if (fghtr.getInitial() == nameInChars.get(i) && fghtr.getCombatStyle().equals(combatStyle) && removedFighters == 0) with if (fghtr == fgt) This will result in the correct fighter being removed. This is because we are checking if its the same fighter object, not that it just has the same initials/Combat enum. Now, you are reiterating through lists for no real reason, and I think your logic is slightly off (calling "aabb" if the array has 4 "a"s would return 4 "a"s instead of 2 a and 2 b), here is a simpler solution: http://pastebin.com/LmngC84M And from there, you could do: barracks.createFighters('a', Combat.MELEE, 3); barracks.createFighters('b', Combat.MELEE, 3); barracks.createFighters('c', Combat.MELEE, 3); barracks.callFighters("aabbcc", Combat.MELEE); which results in: We have 9 fighters. Removing A:MELEE from barracks Removing A:MELEE from barracks Removing B:MELEE from barracks Removing B:MELEE from barracks Removing C:MELEE from barracks Removing C:MELEE from barracks We removed 6 fighters, leaving us with 3 in the barracks. 1 Quote Link to comment Share on other sites More sharing options...
The Hero of Time Posted December 3, 2016 Author Share Posted December 3, 2016 (edited) The problem you are facing is you are comparing fighters by their initials and combat level. If you add two "a" fighters with same combat, when you go through your loop of fighters in the second for loop (the one nested in the first), you're comparing the initial and Combat enums. This means, it will always pick the first Fighter in the Barracks with those initials/Combat enum, and will think it needs to remove that specific Fighter from the Barracks twice. This results in the -1 you are seeing, as the second time around the Fighter is already gone. If you wanna stick with your current solution, simply replace: if (fghtr.getInitial() == nameInChars.get(i) && fghtr.getCombatStyle().equals(combatStyle) && removedFighters == 0) with if (fghtr == fgt) This will result in the correct fighter being removed. This is because we are checking if its the same fighter object, not that it just has the same initials/Combat enum. Now, you are reiterating through lists for no real reason, and I think your logic is slightly off (calling "aabb" if the array has 4 "a"s would return 4 "a"s instead of 2 a and 2 b), here is a simpler solution: http://pastebin.com/LmngC84M And from there, you could do: barracks.createFighters('a', Combat.MELEE, 3); barracks.createFighters('b', Combat.MELEE, 3); barracks.createFighters('c', Combat.MELEE, 3); barracks.callFighters("aabbcc", Combat.MELEE); which results in: We have 9 fighters. Removing A:MELEE from barracks Removing A:MELEE from barracks Removing B:MELEE from barracks Removing B:MELEE from barracks Removing C:MELEE from barracks Removing C:MELEE from barracks We removed 6 fighters, leaving us with 3 in the barracks. Thank you, this is exactly what i was asking. i dont care about the solution i just want to know/understand what was wrong. cheers man Edited December 3, 2016 by The Hero of Time Quote Link to comment Share on other sites More sharing options...
Efpkaf Posted February 22, 2017 Share Posted February 22, 2017 (edited) for(Fighter fgt : tempBarracks){ if(nameInChars.contains(fgt.getInitial()) && fgt.getCombatStyle.equals(combatStyle) && i < nameInChars.size()){ calledFighters.add(new Fighter(nameInChars.get(i), combatStyle)); if(fgt.getInitial() == nameInChars.get(i)){ toRemoveFightersFromBarracks.add(fgt); } } i++; } Instead of i, you can use tmpBarracks.indexOf(fgt) , or if u just want to use variable i, use normal for (int i =0 ...) i++ in foreach looks bad or try stream tmpBarracks.stream().filter(p->{ if(nameInChars.contains(p.getInitial()) && p.getCombatStyle.equals(combatStyle) && tmpBarracks.indexOf(p) < nameIncars.size()){ calledFighters.add(new fighter(nameInchars.get(tmpBarracks.indexOf(p), combatStyle)); if(p.getInitial().equals(nameInChars.get(tmpBarracks.indexOf(p)){ return true; } }else {return false;}) .collect(Collectors.toList()) Streams are pretty useful, code looks better. BUT REMEMBER BECAREFUL WITH list.indexOf() !!! if u have 2 the same objects in list, its going to get first element all the time! also this if if(nameInChars.contains(fgt.getInitial()) && fgt.getCombatStyle.equals(combatStyle) && i < nameInChars.size()){ looks too long, move this things to other method. Edited February 22, 2017 by Efpkaf Quote Link to comment Share on other sites More sharing options...