Is it ok to break the "is a" relationship if I just want part of a class' functionality?

Question

I asked a question about extending TreeMaps into "sort by value" TreeMaps on the "code review" site on stackoverflow. Based on the response, I re-wrote the code. I like it. But, it feels like I might be just using tricks. Or, maybe this is a legit way to write software? Can someone please take a look:

public class ToolBox {
    public interface SortedMapByValue {
        void put(Object key, Integer val);
        Set<Map.Entry<Object, Integer>> entrySet();        
    }

    public static SortedMapByValue getInstanceSortedMapByValue() {
        class MyComp implements Comparator {
            Map<Object, Integer> sharedMap;

            public int compare(Object key1, Object key2) {
                if(key1.equals(key2)) { return 0; }

                Integer val1 = sharedMap.get(key1);
                Integer val2 = sharedMap.get(key2);

                if(val1 > val2) return -1; 
                return 1;
            }     
        }

        class MyMap<K> extends TreeMap<K, Integer> {
            Map<Object, Integer> sharedMap = new HashMap<Object, Integer>();
            MyMap(Comparator comp) {
                super(comp);
            }

            @Override
            public Integer put(K key, Integer val) {
                if(sharedMap.containsKey(key)) {
                    super.remove(key);
                    val += sharedMap.get(key);
                }
                sharedMap.put(key, val);
                super.put(key, val);
                return val;
            }
        }

        class TreeMapByValue implements SortedMapByValue {
            private MyMap<Object> realMap;

            public TreeMapByValue(MyMap myMap) { this.realMap = myMap; }

            public void put(Object key, Integer val) {
                realMap.put(key, val);
            }

            public Set<Map.Entry<Object, Integer>> entrySet() {
                return realMap.entrySet();
            }
        }

        MyComp myComp = new MyComp();
        MyMap<Object> myMap = new MyMap(myComp);
        myComp.sharedMap = myMap.sharedMap;

        SortedMapByValue treeMapByValue = new TreeMapByValue(myMap);
        return treeMapByValue;
    }
} 

The question is just whether it is ok to write classes that are fragile and that break super class' apis, as long as you wrap them in a static method? The TreeMapByValue does not break the "is a" relationship with TreeMap only because I hide it. That seems like a trick, but maybe that is a standard way to program? I don't know.

btw: the title refers to my wanting the "red-black" sort algorithm of TreeMap, but I didn't want what the api says regarding sort ordering. I kept the algorithm, but broke the api. But then I hid doing this.

Please ignore my complete ignorance of generics. I will study them soon enough.

Answer 1

MyMap only overrides a single method. The interface implementation does nothing besides delegating to the MyMap instance. You could easily just write this in the interface implementation.

class TreeMapByValue implements SortedMapByValue, Comparator {
    private Map<Object, Integer> sharedMap;
    private TreeMap<Object, Integer> tree;

    public TreeMapByValue() {
        sharedMap = new HashMap<Object, Integer>();
        tree = new TreeMap<Object, Integer>(this);
    }

    public void put(Object key, Integer val) {
        if(sharedMap.containsKey(key)) {
            tree.remove(key);
            val += sharedMap.get(key);
        }
        sharedMap.put(key, val);
        tree.put(key, val);
        return val;
    }

    public Set<Map.Entry<Object, Integer>> entrySet() {
        return realMap.entrySet();
    }


    public int compare(Object key1, Object key2) {
        if(key1.equals(key2)) { return 0; }

        Integer val1 = sharedMap.get(key1);
        Integer val2 = sharedMap.get(key2);

        if(val1 > val2) return -1; 
        return 1;
    }    
}

Now you only have one class. There is no shared instance that needs to be explicitly assigned to a different class instance. There isn't a concern about how base class is being overridden. In addition, the factory method isn't needed any more since the initialization isn't multiple steps. If you don't want to expose compare(), you can move it into a private inner class.

---

• sharedMap and tree are bad variable names. The names should provide a description of what they hold or why they are being used.

• Be consistent with your if statements. Sometimes they are one-lined with curly brackets. Sometimes they are one-lined without curly brackets. My preference is to never one-line and always use curly brackets.

Answer 2

Is a and Has a are reasonable guidelines, very useful when you are learning, but in practice you should never use inheritence unless it's so blatently obvious that it's needed that there is just no question.

Inheritene often seems like a much neater solution, but people who have used it extensively have learned that it leads to difficult to maintain code in the long run.