0

I have a class Product, which three variables:

class Product implements Comparable<Product>{
   private Type type;                 // Type is an enum
   Set<Attribute> attributes;        // Attribute is a regular class
   ProductName name;                 // ProductName is another enum
}

I used Eclipse to automatically generate the equal() and hashcode() methods:

@Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + ((attributes == null) ? 0 : attributes.hashCode());
        result = prime * result + ((type == null) ? 0 : type.hashCode());
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        Product other = (Product) obj;
        if (attributes == null) {
            if (other.attributes != null)
                return false;
        } else if (!attributes.equals(other.attributes))
            return false;
        if (type != other.type)
            return false;
        return true;
    }

Now in my application I need to sort a Set of Product, so I need to implement the Comparable interface and compareTo method:

@Override
    public int compareTo(Product other){
        int diff = type.hashCode() - other.getType().hashCode();
    if (diff > 0) {
        return 1;
    } else if (diff < 0) {
        return -1;
    }

    diff = attributes.hashCode() - other.getAttributes().hashCode();

    if (diff > 0) {
        return 1;
    } else if (diff < 0) {
        return -1;
    }

    return 0;
    }

Does this implementation make sense? What about if I just want to sort the product based on the String values of "type" and "attributes" values. So how to implement this?

Edit: The reason I want to sort a Set of is because I have Junit test which asserts on the string values of a HashSet. My goal is to maintain the same order of output as I sort the set. otherwise, even if the Set's values are the same, the assertion will fail due to random output of a set.

Edit2: Through the discussion, it's clear that to assert the equality of String values of a HashSet isn't good in unit tests. For my situation I currently write a sort() function to sort the HashSet String values in natural ordering, so it can consistently output the same String value for my unit tests and that suffice for now. Thanks all.

user697911
  • 10,043
  • 25
  • 95
  • 169
  • 1
    Why would you **ever** use a hashCode within compareTo? Makes no sense. What would be the need to sort by hashCode? How is that a "natural" ordering of the class? – Hovercraft Full Of Eels Jun 28 '17 at 21:35
  • Ok. doesn't make sense. How to implement the natural ordering of the class? – user697911 Jun 28 '17 at 21:36
  • So to answer your question, no your implementation makes absolutely no sense. – Hovercraft Full Of Eels Jun 28 '17 at 21:36
  • How do you want it ordered? By what criteria? **That's** what matters most, and that's what you should use within the compareTo. – Hovercraft Full Of Eels Jun 28 '17 at 21:36
  • @user697911: Does your class logically have a natural ordering? What do you want to sort by? – SLaks Jun 28 '17 at 21:37
  • Let me edit my post to explain my needs for sorting. – user697911 Jun 28 '17 at 21:37
  • I don't understand your edit, but it sounds like the tail wagging the dog -- letting your unit testing drive the intrinsic structure of the program rather than the other way around. This has a code smell to it. – Hovercraft Full Of Eels Jun 28 '17 at 21:44
  • @HovercraftFullOfEels, how to compare based on Type and Attribute based on natural ordering? – user697911 Jun 28 '17 at 21:46
  • @user697911: that all depends on what you feel the **natural ordering** should be. The unit testing requirements should not in any way determine this. Again -- tail wagging the dog here. Define the ordering based on intrinsic need first, then adapt unit tests to that requirement. If there is no true natural ordering, then don't use Comparable and instead use a Comparator. – Hovercraft Full Of Eels Jun 28 '17 at 21:47
  • Can you give an example of how to use Comparator to do this? – user697911 Jun 28 '17 at 21:49
  • ??? If you know how to implement Comparable, then surely you can find and use a Comparator??? – Hovercraft Full Of Eels Jun 28 '17 at 21:52
  • Why are you using hashcode to compare 2 objects? `int diff = type.hashCode() - other.getType().hashCode();` ?? Hashcode is a random number which completely bears no meaning whatsoever. – ACV Jun 28 '17 at 22:05
  • @user697911 If an user answered your question please also **accept** his answer ([Accepting Answers: How does it work?](https://meta.stackexchange.com/questions/5234/how-does-accepting-an-answer-work)). If not than please specify what remains unanswered, this is a really crucial part of StackOverflow, thank you very much. – Zabuzard Jul 27 '17 at 11:59

2 Answers2

1

Looks like from all the comments in here you dont need to use Comparator at all. Because:

1) You are using HashSet that does not work with Comparator. It is not ordered.

2) You just need to make sure that two HashSets containing Products are equal. It means they are same size and contain the same set of Products.

Since you already added hashCode and equals methods to Product all you need to do is call equals method on those HashSets.

HashSet<Product> set1 = ...
HashSet<Product> set2 = ...

assertTrue( set1.equals(set2) );
tsolakp
  • 5,858
  • 1
  • 22
  • 28
  • that's true, but in JUnit test, my method returns a set of Product, and it's not convenient for me to create another Set to compare to, because it not straighforwd to create the Product object in Junit test, since Product contains other objects. The easiest way to assert the equality of the returned Hashset is to use the String value, since I can directly see the String value in Eclipse Junit. And that's why I want to assert on String values of HashSet. – user697911 Jun 28 '17 at 22:51
  • 1
    Then just call `equals` on the returned `Set`s. You should never rely on `toString` except for human visualization of that object. The `toString` on `Set` can change in between JUnit runs on different machines and platforms. – tsolakp Jun 28 '17 at 23:24
  • To equals what? I have the returned Set, which is fine, but then compare it to what? I don't have another Set to be compared with Equal, and that's why I use the String value of the Set to compare, because the String value is known and is easy to get, but to create another Set in my Junit test code is hard. – user697911 Jun 29 '17 at 18:53
  • Again, the right way is to compare 2 sets. One that is returned from your method and another one that you need to create either manually by adding Products to it or you can parse whatever string representation you have into a set of Products. – tsolakp Jun 29 '17 at 19:02
  • @tsllakp, that makes sense. What I did for now is to write a sort() function to sort the hashset string values in natural ordering, so that every time it can consistently generate the same string representation. – user697911 Jun 29 '17 at 21:22
0

This implementation does not seem to be consistent. You have no control over how the hash codes look like. If you have obj1 < obj2 according to compareTo in the first try, the next time you start your JVM it could be the other way around obj1 > obj2.

The only thing that you really know is that if diff == 0 then the objects are considered to be equal. However you can also just use the equals method for that check.

It is now up to you how you define when obj1 < obj2 or obj1 > obj2. Just make sure that it is consistent.

By the way, you know that the current implementation does not include ProductName name in the equals check? Dont know if that is intended thus the remark.

The question is, what do you know about that attributes? Maybe they implement Comparable (for example if they are Numbers), then you can order according to their compareTo method. If you totally know nothing about the objects, it will be hard to build up a consistent ordering.

If you just want them to be ordered consistently but the ordering itself does not play any role, you could just give them ids at creation time and sort by them. At this point you could indeed use the hashcodes if it does not matter that it can change between JVM calls, but only then.

Zabuzard
  • 25,064
  • 8
  • 58
  • 82
  • Yes, the actual ordering doesn't matter much. I just want to achieve consistent HashSet output based on any ordering. – user697911 Jun 28 '17 at 21:48
  • `LinkedHashSet` always outputs in the order you have added the elements, maybe that is enough. `HashSet` itself does not maintain any order, `TreeSet` holds its content sorted by using `compareTo`. Your hashcode implementation defines an ordering that is consistent as long as you do not restart the `JVM`. At the next start the order will probably be different, only equal elements will stay equal but the rest will change. – Zabuzard Jun 28 '17 at 21:56
  • LinkedHashSet will have a performance impact, and that's not what I want. What's the right way to ensure correct and same order of a Product set? – user697911 Jun 28 '17 at 22:46
  • The only difference between HashSet and LinkedHashSet is that it will hold the insertion order (by a LinkedList). It is not possible to get the advantages of a HashSet (constant get access) by also holding an order. Are you sure you need a `HashSet`? Maybe the implementations of `SortedSet` are more what you need, for example a `TreeSet` which holds its elements sorted by their `compareTo` or a `Comparator`. In any case **you** as the programmer need to specify the ordering. What are your exact needs? Is the question still unanswered? – Zabuzard Jun 28 '17 at 23:47
  • I only need the ordering in TestCase for JUnit to assert on HashSet String values. – user697911 Jun 29 '17 at 00:22
  • What is wrong with the answer of @tsolakp? Doesn't it solve your problems? I suggest you implement the stuff from the answers and comments and then callback if there is still a problem left. – Zabuzard Jun 29 '17 at 00:31