Guava: TreeMultiSet clear is not working

[rohit5ram]

I have a TreeMultiSet as a member of the singleton object. When I clear the set using clear(), it doesn't clear any.

class Campaign {
   private static Campaign singleton;
   private TreeMultiset<Item> items;

   public Campaign() {
        items = TreeMultiSet.create();
   }
   public void destroy() {
      items.clear();
   }
   public void setItem(Item item) {
      items.add(item);
   }
   public static Campaign getSingleton() {
     if (singleton == null) {
            synchronized (Campaign.class) {
                if (singleton == null) {
                    singleton = new Campaign();
                }
            }
        }
        return singleton;
   }
}

Item item = new Item();
Campaign.getSingleton().setItem(item);
Campaign.getSingleton().destroy(); //its not clearing the set. no errors either.

[shengxuanyi]

Hi Rohit5ram,
I tested your method,changed the Item class to integer and my test result turned out the clear() method is ok.

[rohit5ram]

Hi @shengxuanyi

Using this on Android 4.2.2
Item extends some ParentItem and implements Comparable,Parcelable

public class Item extends ParentItem implements Comparable<Item>, Parcelable {
public int priority;
public String id;
//and so many fields
@Override
    public int compareTo(@NonNull Item item) {
        int value = item.priority.compareTo(this.priority);
        return (value != 0) ? value : 1; //to insert items with same priority.else equal priority items are overwritten
    }
}

In Campaign class I filter the set a lot many times.

Collections2.filter(items, new Predicate<Item>() {
        @Override
            public boolean apply(Item item) {
               return item.id.equals("MAIN"); 
           }
} );

Anything to do with these ?

[Maaartinus]

You compareTo is seriously broken, you really shouldn't expect any collection using it to work:

The implementor must ensure sgn(x.compareTo(y)) == -sgn(y.compareTo(x)) for all x and y.

[jbduncan]

@rohit5ram, could you perhaps explain to us what sort of ordering you're trying to achieve with these Item objects in your TreeMultiset? (For example, least-to-greatest ordering by priority, greatest-to-least ordering...)

Is this something which you're at liberty to talk about?

[rohit5ram]

@jbduncan greatest-to-least ordering

[jbduncan]

Hi @rohit5ram, since you're on Android, I presume you cannot use Java 8 features?

If so, I think you can fix your problem if you simplify your compareTo implementation from

int value = item.priority.compareTo(this.priority);
return (value != 0) ? value : 1;

to

return item.priority.compareTo(this.priority);

I believe this should compare your Items by their priority in reverse order (so greatest-to-least order), but I've not tested it, so please don't hold me to it. :)

My solution assumes you're _only_ interested in comparing Items by their priority.

If you also want to compare by id and the other fields in your Item class, then you'll need something different (which I can help you with again).

Hope this helps!

[rohit5ram]

Hi @jbduncan
The issue is with clear(). I am able to insert in greatest-to-least order with duplicates as well using the existing code. I have given heads up on my implementation so that it can help you to find a reason on why isn't it able to clear the set.
Can you please refer @shengxuanyi comments above

[lowasser]

@rohit5ram, in the absence of a correct implementation of compareTo, TreeMultiset will display undefined, unpredictable behavior like clear() mysteriously not working. There is no way of dealing with this other than using a correct compareTo implementation as @jbduncan describes. @shengxuanyi's comment actually demonstrates this: that if you substitute in a class with a correct compareTo, the code will work.

[rohit5ram]

@Maaartinus @lowasser Is that the reason for clear() not able to work? The existing code is able to insert all the values into the set. If the priority is same then it's overwriting the old object. Hence I avoided and inserted it again.

I assumed TreeMultiSet take duplicates. But with this
item.priority.compareTo(this.priority);
its replacing the old object of same priority

Can you please help ?

[lowasser]

As far as TreeMultiset is concerned, it does take duplicates, but it will also collapse them into a single element. That's the point of its existence, basically; it just keeps track of how many duplicates there are, not keeping track of all the individual duplicates. That is how all Multisets work.

It sounds like what you actually want is a comparator that compares both priority and some other fields, and then to store that in a TreeSet.

[lowasser]

To answer your other question, yes, your contract violating comparator is absolutely the reason clear() is not working.

[jbduncan]

I couldn't have said it any better myself, regarding @lowasser's comments on Multisets and comparators!

@rohit5ram, does this answer all of your concerns for now? :)

Or is there anything else we can do to help you with understanding/using TreeMultiset and/or solving your overall problem (like comparing by multiple fields e.g. priority and then id and then your class's other fields)?

[rohit5ram]

@jbduncan @lowasser The requirement is that every object is unique(pre-known) and should be sorted based on priority. In order to sort it at the time of insertion, I chose TreeMultiSet. I feel the data structure should not be of set.
The best-suited data structure for this would be an ArrayList. So I changed the code as below.

ArrayList<CampaignFileItem> campaignFileItems; Collections.sort(campaignFileItems)

return item.priority.compareTo(this.priority);

Please suggest if you feel on adding anything or else you can close the issue. Thanks a lot !

[jbduncan]

Hi @rohit5ram, there are two things I can think of which may allow you to improve your code.

1. I'd consider moving your current Item.compareTo implementation into it's own Ordering comparator, like so:

private static final Ordering<Item> ITEM_BY_PRIORITY_REVERSE_ORDERING =
  new Ordering<Item>() {
    @Override
    public int compare(Item left, Item right) {
      Preconditions.checkNotNull(left, "left");
      Preconditions.checkNotNull(right, "right");
      return Ints.compare(left.priority, right.priority);
    }
  }.reverse();

The advantage of this is that you can implement Item.compareTo separately in terms of _all_ its fields rather than just priority, which is what future readers of your code would expect compareTo to do, I imagine.

1. I'd then consider replacing ArrayList<CampaignFileItem> campaignFileItems = ... /* your unsorted items */; Collections.sort(campaignFileItems); with

ImmutableList<Item> campaignFileItems = ITEM_BY_PRIORITY_REVERSE_ORDERING.immutableSortedCopy(... /* your unsorted items */);

The advantage of this is you end up with an immutable data structure, which is easier to reason about as you can never accidentally change it (assuming Item itself is immutable, i.e. all its fields are final and they are of primitive types and/or object types which themselves cannot be mutated e.g. String).