Counting the number of occurrences in an array of "Flower Objects"

Question

I'm attempting to create a method that counts the number of Flower objects (created a class called flower btw) that returns the number of particular flowers in an object flower array.

I'm using a HashMap in order to map an integer (sum amount) to a key (key being the flower object). However, when I output the array, the memory address of the key in the heap alongside the key's location in the HashMap table.

My intention however is to print flower (it's name mainly) with the amount of flowers in that array that are of the same Flower Object type.

My code is as follows:

private void displayFlowers(Flower flowerPack[]) {
        // TODO: Display only the unique flowers along with a count of any
        // duplicates
        /*
         * For example it should say Roses - 7 Daffodils - 3 Violets - 5
         */
        HashMap<Flower, Integer> flowerFrequency = new HashMap<Flower, Integer>();
        for (Flower aFlower : flowerPack) {
            if (flowerFrequency.containsKey(aFlower)) {
                Integer i = flowerFrequency.get(aFlower);
                i++;
            } else {
                flowerFrequency.put(aFlower, new Integer(1));
            }
        }
        System.out.println(flowerFrequency);
}

The output is like this:

1: Add an item to the pack.
2: Remove an item from the pack.
3: Search for a flower.
4: Display the flowers in the pack.
0: Exit the flower pack interfact.
4
{null=1, Flower@6bc7c054=1, Flower@42a57993=1, Flower@75b84c92=1}

As instructed, I have also added the toString() and equals() methods to the Flower class as follows:

I added the toString() and equals() methods in the Flower class as follows:

public String toString() {
        return this.color + " " + this.name + " smells like " + this.scentType + " is Thorny " + this.hasThorns;
}


@Override
public boolean equals(Object otherFlower) {

        if (otherFlower == null) {
            return false;
        }
        if (!Flower.class.isAssignableFrom(otherFlower.getClass())) {
            return false;
        }
        final Flower other = (Flower) otherFlower;
        if ((this.name == null) ? (other.name != null) : !this.name.equals(other.name)) {
            return false;
        }
        if (!(this.color.equals(other.color))) {
            return false;
        }

        if (!(this.scentType.equals(other.scentType))) {
                return false;

        }

        if (this.hasThorns != other.hasThorns) {
                return false;
        }

        return true;

}

Answer 1

Besides overriding toString() to return the string you want to read, your code contains a real bug:

if (flowerFrequency.containsKey(aFlower)) {
  Integer i = flowerFrequency.get(aFlower);
  i++;

Will not work. You see, Integer objects are immutable. Your code fetches the counter from the map, turns that thing into an int (does the above even compile?!) ... to then forget about it! You can't modify the value of the Integer you stored in that map, instead, you have to put an updated value back into the map:

if (flowerFrequency.containsKey(aFlower)) {
  Integer currentCounter = flowerFrequency.get(aFlower);
  flowerFrequency.put(aFlower, currentCounter+1);

Edit: and of course, in order to make that Map really work with your flower objects, you have to provide meaningful overrides for equals()/hashCode() (see here for some guidance).

Answer 2

The problem seems to be how you're interacting with the map. You're both wrapping an int via Integer's constructor (unnecessary due to autoboxing), and you're incrementing a value but never placing it back.

        if (flowerFrequency.containsKey(aFlower)) {
            Integer i = flowerFrequency.get(aFlower); //uses two lookups
            i++; //only increments our immediate value, not the object
        } else {
            flowerFrequency.put(aFlower, new Integer(1)); //does not need wrapping
        }

Ideally, you would retrieve once and place once. HashMap returns null when there is no mapping for the key, so you can use that to your advantage:

Integer amount = flowerFrequency.get(aFlower);
if (amount == null) {
    amount = 0;
}
flowerFrequency.put(aFlower, amount + 1);

Shortened via Java 8:

Integer amount = flowerFrequency.getOrDefault(aFlower, 0); //default 0 for no value
flowerFrequency.put(aFlower, amount + 1);

As for simplifying the problem, Collectors has a nice utility for this:

Map<Flower, Integer> frequency = Arrays.stream(flowerPack)
      .collect(Collectors.groupingBy(Function.identity(), Collectors.summingInt(t -> 1)));

But if you still like utilizing the loop instead, there's also Map#compute for a one-liner solution:

Map<Flower, Integer> frequency = new HashMap<>();
for (Flower f : flowerPack) {
    frequency.compute(f, (key, old) -> old == null ? 1 : old + 1); //increment
}

Answer 3

If you don't override the toString() (comes from java.lang.Object), when you print the object to console/log, it simply prints the hex representation of the object (like Flower@6bc7c054, etc..). But you can override this default behavior by overriding toString() method in your Flower class to actually the give the name.

public class Flower {
    private String name;
    //other flower variables

   public String toString() {
      return name;
   } 
}

public String toString() : Returns a string representation of the object. In general, the toString method returns a string that "textually represents" this object.

Please refer the toString() api here https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html