Java (need I say more?)



  • Just spotted this incomprehensible wall of text in the source code for the java.awt.RenderingHints class:

    // Note that this system can fail in a mostly harmless
    // way.  If we end up generating the same identity
    // String for 2 different classes (a very rare case)
    // then we correctly avoid throwing the exception above,
    // but we are about to drop through to a statement that
    // will replace the entry for the old Key subclass with
    // an entry for the new Key subclass.  At that time the
    // old subclass will be vulnerable to someone generating
    // a duplicate Key instance for it.  We could bail out
    // of the method here and let the old identity keep its
    // record in the map, but we are more likely to see a
    // duplicate key go by for the new class than the old
    // one since the new one is probably still in the
    // initialization stage.  In either case, the probability
    // of loading 2 classes in the same VM with the same name
    // and identityHashCode should be nearly impossible.
    

    I am in awe at the levels of WTF-ery required to necessitate such a comment.



  • Looks like an answer to somebody using the meetings strategy I outlined at Unreadable Code 101. Whoever he is, he just pointed a bug due to hash colision, that's the spirit.



  • How is this wall of text incomprehensible? It seems to me a comment made by someone who has spotted a problem and explains it so you don't have to work it out yourself (and might miss it).



  • This is the kind of comment ment for people who think about changing the class or who happen to run into a weird situation involving it, so they don't have to start staring at the code and try to figure it out for themselves. If that is the only comment, it certainly is insufficient.



  •  I just don't like using "we" in code.



  • I was actually trying to focus less on the writing style and more on this line: "In either case, the probability of loading 2 classes in the same VM with the same name and identityHashCode should be nearly impossible." Try to imagine why that sentence would be necessary in describing the internals of a class that lets you change antialiasing and font rendering settings. (If you haven't heard the term accidental complexity, this is what it means.)

    ...In fact, let's look at some other parts of the source code.

    public class RenderingHints
        implements Map<Object,Object>, Cloneable

    Unless you're writing a new general-purpose map implementation, why yould you ever implement Map yourself?

         * Instances of this class are immutable and unique which
         * means that tests for matches can be made using the
         * {@code ==} operator instead of the more expensive
         * {@code equals()} method.

    Why? Who cares if we have to use equals() instead of == when all anyone ever does with these keys is set them once per render and then forget about them?

    private static HashMap identitymap = new HashMap(17);

    Why is the HashMap initialized to a size of 17? Where did this magic number come from? It can't possibly be relevant, because if there are more than 17 entries in the map it will just expand at a tiny little cost and no one will notice. What kind of keys and values does the map have? I don't know, because this is Java 7 and the code still hasn't been updated to use generics. (From looking at the code below, I figured out that the actual type of the field is HashMap<String, WeakReference<Key>>. Obviously.)

            /**
             * Returns true if the specified object is a valid value
             * for this Key.
             * @param val the Object to test for validity
             * @return true if val is valid;
             *         false otherwise.
             */
            public abstract boolean isCompatibleValue(Object val);
    

    There are so many better ways to do this.

    HashMap hintmap = new HashMap(7);

    Again with the magic numbers. Why 7?

        public Object put(Object key, Object value) {
            if (!((Key) key).isCompatibleValue(value)) {
                throw new IllegalArgumentException(value+
                                                   " incompatible with "+
                                                   key);
            }
            return hintmap.put((Key) key, value);
        }

    Aha. So that's why they implemented Map themselves. That's... a really stupid reason.

        public String toString() {
            if (hintmap == null) {
                return getClass().getName() + "@" +
                    Integer.toHexString(hashCode()) +
                    " (0 hints)";
            }
    
        return hintmap.toString();
    }</pre>
    

    Why is there a case for hintmap being null? It's initialized on construction and nothing ever sets it again. Furthermore, the other methods in the class reference it as if it can't be null.

    Finally, virtually every reference to the RenderingHints class in client code consists of one or both of the following lines:

        g.addRenderingHints((Map<?, ?>) Toolkit.getDefaultToolkit().getDesktopProperty("awt.font.desktophints"));
        g.setRenderingHint(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);

    So really, the whole thing is an overcomplicated, excessively-general way to turn on settings that should be on by default.



  • @arotenbe said:

    Finally, virtually every reference to the RenderingHints class in client code consists of one or both of the following lines:

        g.addRenderingHints((Map<?, ?>) Toolkit.getDefaultToolkit().getDesktopProperty("awt.font.desktophints"));
        g.setRenderingHint(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);

    So really, the whole thing is an overcomplicated, excessively-general way to turn on settings that should be on by default.

    There is a good reason why those settings are off by default. Early VMs (and even 1.5 on Apple about two years ago) barely used the graphics card. I have seen framerates below 1 fps caused by clearing the background with fillRect and anti-aliasing on each frame.


  • @pjt33 said:

    @arotenbe said:

    Finally, virtually every reference to the RenderingHints class in client code consists of one or both of the following lines:

        g.addRenderingHints((Map<?, ?>) Toolkit.getDefaultToolkit().getDesktopProperty("awt.font.desktophints"));
        g.setRenderingHint(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);

    So really, the whole thing is an overcomplicated, excessively-general way to turn on settings that should be on by default.

    There is a good reason why those settings are off by default. Early VMs (and even 1.5 on Apple about two years ago) barely used the graphics card. I have seen framerates below 1 fps caused by clearing the background with fillRect and anti-aliasing on each frame.
    I get framerates below 0 fps on my quantum computer.


  • @arotenbe said:

    What kind of keys and values does the map have? I don't know, because this is Java 7 and the code still hasn't been updated to use generics. (From looking at the code below, I figured out that the actual type of the field is HashMap<String, WeakReference<Key>>. Obviously.)

     

    Not so fast young grasshopper - until you try it, you cannot merely assume that the true type parameters of the HashMap are String, WeakReference<Key>, just because that's how it's used in the code you've examined. I've definitely encountered legacy code written in the Dark Ages Before Generics where the type of an ArrayList contained inside a class named Entry was sometimes ArrayList<Entry>, sometimes ArrayList<ArrayList<Entry or ArrayList>>, whose entries were children of the parent Entry. Why would you do this? I have no idea. Presumably because it's too much effort to store leaf nodes in one place and internal nodes in another.



  • @dhromed said:

    I just don't like using "we" in code.

     We hates it, we hates it, we hates it forever!

    FTFY.

     



  • @Tacroy said:

    I've definitely encountered legacy code written in the Dark Ages Before Generics where the type of an ArrayList contained inside a class named Entry was sometimes ArrayList<Entry>, sometimes ArrayList<ArrayList<Entry or ArrayList>>, whose entries were children of the parent Entry. Why would you do this? I have no idea. Presumably because it's too much effort to store leaf nodes in one place and internal nodes in another.

     

    I once wrote some Hella Elegant™ pathfinding code for a college project which involved something very similar to the following:

    class Pathfinder {
        // SNIP: basic Djikstra's algorithm implementation
        class Node {
            ArrayList<Edge> map;
        }
        class Edge {
            Node endpoint;
            double cost;
        }
    }

    I was a foolish child.



  • @Ben L. said:

    I get framerates below 0 fps on my quantum computer.
     

    I once tried to rotate my VM along a vertical axis, but I got like 0.5i fps, so that wasn't a very good idea.



  • @arotenbe said:

    I was actually trying to focus less on the writing style and more on this line: "In either case, the probability of loading 2 classes in the same VM with the same name and identityHashCode should be nearly impossible." Try to imagine why that sentence would be necessary in describing the internals of a class that lets you change antialiasing and font rendering settings. (If you haven't heard the term accidental complexity, this is what it means.)

    ...In fact, let's look at some other parts of the source code.

    public class RenderingHints
        implements Map<Object,Object>, Cloneable

    Unless you're writing a new general-purpose map implementation, why yould you ever implement Map yourself?

         * Instances of this class are immutable and unique which
         * means that tests for matches can be made using the
         * {@code ==} operator instead of the more expensive
         * {@code equals()} method.

    Why? Who cares if we have to use equals() instead of == when all anyone ever does with these keys is set them once per render and then forget about them?

    private static HashMap identitymap = new HashMap(17);

    Why is the HashMap initialized to a size of 17? Where did this magic number come from? It can't possibly be relevant, because if there are more than 17 entries in the map it will just expand at a tiny little cost and no one will notice. What kind of keys and values does the map have? I don't know, because this is Java 7 and the code still hasn't been updated to use generics. (From looking at the code below, I figured out that the actual type of the field is HashMap<String, WeakReference<Key>>. Obviously.)

            /**
             * Returns true if the specified object is a valid value
             * for this Key.
             * @param val the Object to test for validity
             * @return true if val is valid;
             *         false otherwise.
             */
            public abstract boolean isCompatibleValue(Object val);
    

    There are so many better ways to do this.

    HashMap hintmap = new HashMap(7);

    Again with the magic numbers. Why 7?

        public Object put(Object key, Object value) {
            if (!((Key) key).isCompatibleValue(value)) {
                throw new IllegalArgumentException(value+
                                                   " incompatible with "+
                                                   key);
            }
            return hintmap.put((Key) key, value);
        }

    Aha. So that's why they implemented Map themselves. That's... a really stupid reason.

        public String toString() {
            if (hintmap == null) {
                return getClass().getName() + "@" +
                    Integer.toHexString(hashCode()) +
                    " (0 hints)";
            }
    
        return hintmap.toString();
    }</pre>
    

    Why is there a case for hintmap being null? It's initialized on construction and nothing ever sets it again. Furthermore, the other methods in the class reference it as if it can't be null.

    Finally, virtually every reference to the RenderingHints class in client code consists of one or both of the following lines:

        g.addRenderingHints((Map<?, ?>) Toolkit.getDefaultToolkit().getDesktopProperty("awt.font.desktophints"));
        g.setRenderingHint(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);

    So really, the whole thing is an overcomplicated, excessively-general way to turn on settings that should be on by default.

     

    This class [url=http://docs.oracle.com/javase/1.4.2/docs/api/java/awt/RenderingHints.html]predates Generics[/url] and is also part of the rendering library that Sun recommended you stop using in favor of Swing in Java 1.2, which, incidentally, was released 14 years ago.

     



  • @powerlord said:

    This class ... is also part of the rendering library that Sun recommended you stop using in favor of Swing in Java 1.2, which, incidentally, was released 14 years ago.


    Not entirely correct. This class is one of the things which was added to AWT in 1.2, and Swing was built on top of AWT.



  • @arotenbe said:

    public class RenderingHints
        implements Map<Object,Object>, Cloneable

    Unless you're writing a new general-purpose map implementation, why yould you ever implement Map yourself?

     

    My guess is that at one point in the design phase RenderingHints had a superclass. Since they wanted to add validation to the put() method, and since Java doesn't have multiple inheritence, the only way to get the Map polymorphism they wanted would have been to implement the interface themselves.

    Then when the design changed they decided to just roll with it.


Log in to reply