4

I wanted to make a method to check if I can see if each of my piles have 6 cards in it. This is my method 

public boolean checkIfPileHasSixCards() {

        map.put("tpile1", tpile1);
        map.put("tpile2", tpile2);
        map.put("tpile3", tpile2);
        for (ArrayList<Card> value : map.values()) {
              int size=value.size();
              if(size==6) {
                  return true;
              }
        }
        return false;

    }

Is my logic correct, is there a more efficient way I can iterate over the values and check if each value (ArrayList) size is 6?

Mureinik
  • 297,002
  • 52
  • 306
  • 350
Clyde
  • 85
  • 6

1 Answers1

3

Returning true inside the loop is not correct - this way, your method will return true if any list in the map has six elements, not if all of them do. Instead, you should return false if a list doesn't have six elements, and only return true after you're done iterating the values:

for (List<Card> value : map.values()) {
      int size = value.size();
      if(size != 6) {
          return false;
      }
}
return true;

Note, by the way, that using Java 8's stream could make this snippet a lot cleaner. It won't be more efficient (it's still an O(n) operation in the worst case), but it will definitely be more concise:

return map.values().stream().allMatch(l -> l.size() == 6);
Mureinik
  • 297,002
  • 52
  • 306
  • 350
  • It looks to me, from the code, that the OP is creating the `Map` just to loop over it. I'm not sure the `Map` is even required. – Boris the Spider Sep 29 '18 at 16:08
  • The `size` variable is redundant, IMHO. – Nicholas K Sep 29 '18 at 16:13
  • how else can I loop through multiple arrayLists at once? – Clyde Sep 29 '18 at 16:13
  • 2
    @NicholasK I wanted to make the minimal amount of changes to make OP's code work, but I agree, you could just lose and use `value.size()` directly – Mureinik Sep 29 '18 at 16:16
  • @Clyde, simplest? - you have declared the variables so just hardcode it. `x1.size() == 6 && x2.size() == 6` etc – Boris the Spider Sep 29 '18 at 16:16
  • 1
    would hardcoding be effecient for 12 arrayLists that I need to check? I feel like iterating through the HashMap would just be easier than to type it all out again. I just added two arrayLists here to check if my method was ok but I have added the other ten now. – Clyde Sep 29 '18 at 16:27
  • If you have declared 12 variables, then yes @Clyde; hardcoding is efficient - think of it this way; you are _already_ hardcoding at the creation of the `Map` so why not just do the check there? There is absolutely no point to make a `Map` when the variables exist. If you got rid of your variables and just used a `Map` - i.e. you had a dynamic number of variables as the call site - then things would change. And the `Stream` based answer here would be the one to go for. – Boris the Spider Sep 29 '18 at 16:36