Reflections on reflection..



  • Note: this is my first attempt at large-scale anonymization. I have opted to completely remove all traces of the original application's functions, and written new code that demonstrates something like the same kind of WTF.

    The setup: imagine an interface for classes that "do" shapes. That is, draws them, display them in some manner, talks about them, etc. This part is made up by me; the original app was something much more concrete.

    public interface ShapeDoer {
        void doTriangle();
        void doSquare();
        void doDiamond();
        void doLabelledBox(String s);
    }

    Now, there are several implementations of these methods, some for graphical systems, some for text screens, others for audio-only interfaces, etc. For brevity we only look at one of these implementations, which is again completely made up by me and not really like anything from the original code:
     

    public class AsciiShapeDoer implements ShapeDoer {

        @Override
        public void doTriangle() {
            System.out.println("  /\\ ");
            System.out.println(" /__\\");
        }

        @Override
        public void doSquare() {
            System.out.println("+--+");
            System.out.println("|  |");
            System.out.println("+--+");
        }

        @Override
        public void doDiamond() {
            System.out.println(" /\\ ");
            System.out.println("/  \\");
            System.out.println("\\  /");
            System.out.println(" \\/ ");
        }

        @Override
        public void doLabelledBox(String s) {
            bannerFor(s);
            System.out.println("| " + s + " |");
            bannerFor(s);
           
        }

        private void bannerFor(String s) {
            System.out.print("+-");
            for(int i=0; i<s.length(); i++) {
                System.out.print("-");
            }
            System.out.println("-+");
        }



    }

     

    Now the real fun starts. In order to shield the rest of the application from dealing with all of the various ShapeDoer implementations, the programmer created a factory class to hide the details. So the rest of the app might have code like this:

            ShapeDoerFactory sdf = new ShapeDoerFactory();
           
            sdf.doLabelledBox("Brilliant!");
            sdf.doTriangle();
            sdf.doSquare();
            sdf.doDiamond();

    This is already starting to smell a little strange, because you (or at least I) would expect the factory to return an instance of ShapeDoer, rather than having all of the methods be called on the factory itself. This is indeed present in the original code, but overall it's OK compared to what you see *inside* the factory:

     

    import java.lang.reflect.*;
    import java.util.*;

    public class ShapeDoerFactory {

        private static final String TYPE_PLAIN_TEXT = "TYPE_PLAIN_TEXT";
        private static final String TYPE_ASCII = "TYPE_ASCII";
        private static final String TYPE_AUDIO = "TYPE_AUDIO";
        private static final String TYPE_SWING = "TYPE_SWING";
        private static final String TYPE_SWT = "TYPE_SWT";
        
        
        private final static Map classMap = new HashMap();
        
        private final static String displayType;
        
        static {
            
            try {
            
                classMap.put( TYPE_PLAIN_TEXT, Class.forName("PlainTextShapeDoer") );
                classMap.put( TYPE_ASCII, Class.forName("AsciiShapeDoer") );
                classMap.put( TYPE_AUDIO, Class.forName("AudioShapeDoer") );
                classMap.put( TYPE_SWING, Class.forName("SwingShapeDoer") );
                classMap.put( TYPE_SWT, Class.forName("SWTShapeDoer") );

            } catch (ClassNotFoundException e) {
                e.printStackTrace();
            }
            
            displayType = determineDisplayType();
            
        }
        
        
        
        private static final String METHOD_DO_LABELLED_BOX = "doLabelledBox";
        private static final String METHOD_DO_TRIANGLE = "doTriangle";
        private static final String METHOD_DO_SQUARE = "doSquare";
        private static final String METHOD_DO_DIAMOND = "doDiamond";
        
        
        public void doLabelledBox(String message) {
            try {
                
                Class clazz = (Class) classMap.get(displayType);
                
                if(clazz == null) {
                    throw new IllegalArgumentException("Invalid: " + displayType);
                }

                Class[] paramTypes = new Class[1];
                Object[] paramValues = new Object[1];
                paramTypes[0] = message.getClass();
                paramValues[0] = message;
                
                Object doer = clazz.newInstance();
                Method m = clazz.getMethod(METHOD_DO_LABELLED_BOX, paramTypes);
                m.invoke(doer, paramValues );
            } catch (NoSuchMethodException e) {
                e.printStackTrace();
            } catch (IllegalArgumentException e) {
                e.printStackTrace();
            } catch (IllegalAccessException e) {
                e.printStackTrace();
            } catch (InvocationTargetException e) {
                e.printStackTrace();
            } catch (InstantiationException e) {
                e.printStackTrace();
            }
        }

        
        public void doTriangle() {
            try {
                
                Class clazz = (Class) classMap.get(displayType);
                
                if(clazz == null) {
                    throw new IllegalArgumentException("Invalid: " + displayType);
                }

                Class[] paramTypes = new Class[0];
                Object[] paramValues = new Object[0];
                
                Object doer = clazz.newInstance();
                Method m = clazz.getMethod(METHOD_DO_TRIANGLE, paramTypes);
                m.invoke(doer, paramValues );
            } catch (NoSuchMethodException e) {
                e.printStackTrace();
            } catch (IllegalArgumentException e) {
                e.printStackTrace();
            } catch (IllegalAccessException e) {
                e.printStackTrace();
            } catch (InvocationTargetException e) {
                e.printStackTrace();
            } catch (InstantiationException e) {
                e.printStackTrace();
            }
        }

        public void doSquare() {
            try {
                
                Class clazz = (Class) classMap.get(displayType);
                
                if(clazz == null) {
                    throw new IllegalArgumentException("Invalid: " + displayType);
                }

                Class[] paramTypes = new Class[0];
                Object[] paramValues = new Object[0];
                
                Object doer = clazz.newInstance();
                Method m = clazz.getMethod(METHOD_DO_SQUARE, paramTypes);
                m.invoke(doer, paramValues );
            } catch (NoSuchMethodException e) {
                e.printStackTrace();
            } catch (IllegalArgumentException e) {
                e.printStackTrace();
            } catch (IllegalAccessException e) {
                e.printStackTrace();
            } catch (InvocationTargetException e) {
                e.printStackTrace();
            } catch (InstantiationException e) {
                e.printStackTrace();
            }
        }

        public void doDiamond() {
            try {
                
                Class clazz = (Class) classMap.get(displayType);
                
                if(clazz == null) {
                    throw new IllegalArgumentException("Invalid: " + displayType);
                }

                Class[] paramTypes = new Class[0];
                Object[] paramValues = new Object[0];
                
                Object doer = clazz.newInstance();
                Method m = clazz.getMethod(METHOD_DO_DIAMOND, paramTypes);
                m.invoke(doer, paramValues );
            } catch (NoSuchMethodException e) {
                e.printStackTrace();
            } catch (IllegalArgumentException e) {
                e.printStackTrace();
            } catch (IllegalAccessException e) {
                e.printStackTrace();
            } catch (InvocationTargetException e) {
                e.printStackTrace();
            } catch (InstantiationException e) {
                e.printStackTrace();
            }
        }
        
        
        private static String determineDisplayType() {
            // dummy implementation just for the example. pretend the app checks system properties or something.
            return classMap.keySet().toArray()[ new Random().nextInt(classMap.size()) ].toString();
        }


        
    }




  • Wow, that's just... icky.  And can I say for the record that I can't stand when people create variables called "clazz"?  Just call it "cl" or something.  The meaning will be  obvious from the context, and you don't end up looking like an idiot who can't spell.

     



  • Well it's certainly a "factory" alright - creating a new instance of the class with every single method called on it. Also love how any lookup failure in the static initializer disables that class and all the classes that would be looked for after it, instead of just the one failing. Also, isn't the usual idiom for Java to just put the fully-qualified classname into a system property, and just have the factory go off that?

    @cconroy said:

    Wow, that's just... icky.  And can I say for the record that I can't stand when people create variables called "clazz"?  Just call it "cl" or something.  The meaning will be  obvious from the context, and you don't end up looking like an idiot who can't spell.

     

    I believe the java API itself uses 'clazz' for parameters that are Class objects, so it makes sense to use the same name for consistency reasons, but I do agree there are other names that could be better used. However, I can see someone possibly taking 'cl' (and just 'c') as hungarian notation. You also have 'cls' (though you may have the same problem), as well as underscored/otherwise-mangled variants of 'class' ('_class', 'cl4ss, etc...), and there's 'type' too (anything someone might call a type in Java that you can get info on at runtime is going to be represented as a Class object - yes, even the primitives (at least since Java 1.5 anyway)). Or you could always give the Class object an actual descriptive name (you know, like you give to your other variables of less specific-case types?), like 'whatToInstatiate' (eg when constructing), 'haystack' (eg for member lookups), 'castTarget', 'arrayType', etc...



  • I'm guessing the original programmer has a dynamically-typed language background. In PHP it's perfectly valid to do...

    $class = 'MyClass';
    $instance = new $class();

    ... which was quite useful before interfaces became available. Here though, the programmer declared an interface for the methods, and then completely pissed away on it. Good job.

     



  • @Sunstorm said:

    I'm guessing the original programmer has a dynamically-typed language background. In PHP it's perfectly valid to do...

    $class = 'MyClass';
    $instance = new $class();

    ... which was quite useful before interfaces became available. Here though, the programmer declared an interface for the methods, and then completely passed away on it. Good job.

     

    Yes, the major malfunction I was trying to communicate was that the developer completely missed the point of having an interface. Glad you caught that.

    I've got nothing against dynamically loaded classes, when justified. Or even full-blown reflection. When justified. Most of the java frameworks that people use everyday could not be written without it. (How are database drivers loaded? How does an application server automagically load any code you throw into the right directories? How does a web app framework call methods on a bean based on parameters from an HTTP request?)

    But I know a WTF in this area when I see it.



  • Good lord.

    If it makes you feel better, I feel a lot better about my code that uses reflection after seeing this. At least I'm pretty certain that I'm using mine properly...



  • @Volmarias said:

    At least I'm pretty certain that I'm using mine properly...

    Excuse me while I snigger. Everybody thinks that. 


Log in to reply