0

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.

  • 3
    You're allowed to do whatever you want inside the implementation class, so long as you conform to the API you're implementing. The actual behavior doesn't even have to make sense (it won't, if the code contains bugs). That said, it's probably bad form to inherit from a class when the descendant class won't use its behavior. What purpose would that serve, other than to confuse the programmer who has to read the code after you write it? – Robert Harvey Mar 18 '15 at 22:07
  • Well, what I want access to is the black/white sort algorithm in TreeMap. Collections.sort() are (n log(n)). So, TreeMap is just about getting fast sorting. – red shoe Mar 18 '15 at 22:12
  • whoops. i mean the "red-black tree" sort algorithm of treemap. it is log(n) time. mergesort is (n log(n)). – red shoe Mar 18 '15 at 22:20
  • 2
    This code is too contrived for me to bother deciphering. Is there a reason you can't just use a TreeMap instead of extending it? – Doval Mar 19 '15 at 13:14
  • @Doval I want a Map that sorts by values (not the keys). And, when keys clash, I want to sum their values. I extend TreeMap just so I can re-use the "red-black" sort algorithm. – red shoe Mar 19 '15 at 14:12
0

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.

  • Is passing a reference to "this" during its own construction to the TreeMap constructor an issue? An instance of TreeMap briefly has a reference to an object that is not fully formed. I think I read something about "this" references in constructors, but I'm not sure I understand that issue. – red shoe Mar 19 '15 at 15:42
  • btw: I like this design a lot. Mine was really hard to keep straight in my head. – red shoe Mar 19 '15 at 16:05
  • It is possible cause problems when some operations happen during an instances constructor. For example, maybe a member variable hasn't been assigned a value yet when the operation occurs. However, in this case compare() only ever interacts with sharedMap, which has already been initialized. Additionally, TreeMap only stores the reference in its constructor. It doesn't use the Comparator until you start dealing with values. – unholysampler Mar 19 '15 at 19:51
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.

  • This is a little vague unless you go into things like the "fragile base class problem." – Robert Harvey Mar 19 '15 at 6:04
  • But, the question is more about whether you can just ignore such conventions as long as (1) it is done on a very limited and very contained scale, (2) no other code can access the fragile code. My fragile classes are declared in the constructor method itself. – red shoe Mar 19 '15 at 12:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.