Avoid Working With Classes and Reflection Where Possible

As with all my posts, there are exceptions and caveats. But in your general code the only time you should be dealing with MyClass.class is when initialising your logger:

Logger logger = LoggerFactory.getLogger(HelloWorld.class);

In day to day code you should have no other reason to be handling raw class files. It’s anti-object orientation and it’s often difficult to understand for someone reading the code.

The common pitfalls:

Switch Statements/instanceof

This tends to be the most common, where a supertype is passed in and then a big block of if-statements to figure out what the type is. Here is a simple example that should look familiar:

List<ASmallThing> allTheSmallThings = new ArrayList<>();

    public void doSomething(AThing thing){
        if(thing instanceof ABigThing){
            ABigThing abt = (ABigThing) thing;
            allTheSmallThings.addAll(abt.smallThings());
        }else if(thing instanceof ASmallThing){
            ASmallThing ast = (ASmallThing) thing;
            allTheSmallThings.add(ast);
        }
    }

    private interface AThing {
    }

    private class ABigThing extends AThing{
        public List<ASmallThing>smallThings() {
            return asList(new ASmallThing(), new ASmallThing());
        }
    }

    private class ASmallThing extends AThing{
    }

We have different behaviour based on what the type is that’s coded here into a big if statement. This goes against everything object oriented and should be avoided. If you want specific behaviour dependent on the type of object then that behaviour should be encapsulated on the object itself. Let’s look at this refactored example:

 List<ASmallThing> allTheSmallThings = new ArrayList<>();

    public void doSomething(AThing thing){
        thing.appendSmallObjectsTo(allTheSmallThings);
    }

    public interface AThing {
        void appendSmallObjectsTo(List<ASmallThing> allTheSmallThings);
    }

    public static class ABigThing implements AThing{
        public List<ASmallThing>smallThings() {
            return asList(new ASmallThing(), new ASmallThing());
        }

        @Override
        public void appendSmallObjectsTo(List<ASmallThing> allTheSmallThings) {
            allTheSmallThings.addAll(smallThings());
        }
    }

    public static class ASmallThing implements AThing {
        @Override
        public void appendSmallObjectsTo(List<ASmallThing> allTheSmallThings) {
            allTheSmallThings.add(this);
        }
    }

Much cleaner and object oriented. This version supports the ideal of “Tell don’t ask”. We could have easily added a “smallObjects()” method to each and just called add on that, however then we’re querying the objects state. In the version above, we are giving the Thing objects our list and saying “please add all your small objects”. A much better, and cleaner design.

Unfortunately this usually isn’t the end of the story. This sort of code usually tends to pop up when trying to access implementation details on a third party library. This means we can’t change the object in question. Alternatively, the interface in question may have many many implementations and it isn’t possible or practical to add a new method to all of them. In this case, the always handy decorator pattern can come to the rescue. Sure, we still have to have the casting take place, but at least it’s hidden to an extent and allows some element of OO. Let’s look at this example from swagger-core:

for (String keyProp: m.getProperties().keySet()) {
            Property p = m.getProperties().get(keyProp);
            if (p instanceof ArrayProperty) {
                ArrayProperty ap = (ArrayProperty) p;
                if (ap.getItems() instanceof RefProperty) {
                    RefProperty rp = (RefProperty) ap.getItems();
                    String simpleRef = rp.getSimpleRef();
                    referencedDefinitions.add(simpleRef);
                }
            } else if (p instanceof RefProperty) {
                RefProperty rp = (RefProperty) p;
                String simpleRef = rp.getSimpleRef();
                referencedDefinitions.add(simpleRef);
            }
        }

As you can see, this code is pretty terrible. It has clear duplication of the code and is difficult to read and understand. However, Property has a lot of subclasses and it makes no sense to add a new method just for this. Let’s look at a better, refactored sample:

for (String keyProp: m.getProperties().keySet()) {
            Property p = m.getProperties().get(keyProp);
            new AccessibleReferenceProperty(p).addAllReferencesTo(referencedDefinitions);
        }

private class AccessibleReferenceProperty {
        private Property p;

        public AccessibleReferenceProperty(Property p) {
            this.p = p;
        }

        public void addAllReferencesTo(Set<String> referencedDefinitions) {
            if (p instanceof ArrayProperty) {
                ArrayProperty ap = (ArrayProperty) p;
                if (ap.getItems() instanceof RefProperty) {
                    new AccessibleReferenceProperty(ap.getItems())
                        .addAllReferencesTo(referencedDefinitions);
                }
            } else if (p instanceof RefProperty) {
                RefProperty rp = (RefProperty) p;
                String simpleRef = rp.getSimpleRef();
                referencedDefinitions.add(simpleRef);
            }
        }
    }

Hopefully you’ll agree this is much clearer. In this example I’ve kept the functionality in this Decorator class, much as you would be forced to if working with 3rd party code. If you had access to the classes (RefProperty and ArrayProperty) you could move the functionality onto those classes directly which would be much preferred.

Reflection and Dependency Injection

As a rule, avoid reflection like a plague. Although there will always be a case here and there where it’s needed, as a rule of thumb if you’re considering a solution involving it then chances are your design is wrong. Deal exclusively in Objects.

A prime example of where this has been done badly in public is in Jetty. I think it’s a great library, but the initialisation is utterly terrible. You’re basically given two options; classpath scanning, which I’m against as i prefer to see everything explicitly wired up. It’s much easier to trace through your code and removes any chance that you’ll be stuck banging your head against the desk trying to debug a problem which is just due the scanning not behaving as you’d expected it to.

Option 2 is to register class files. They have effectively forced their own version of Dependency Injection upon the user. For example from the documentation:

ResourceConfig resourceConfig = new ResourceConfig(MyResource.class);
resourceConfig.register(org.glassfish.jersey.server.filter.UriConnegFilter.class);
resourceConfig.register(org.glassfish.jersey.server.validation.ValidationFeature.class);
resourceConfig.register(org.glassfish.jersey.server.spring.SpringComponentProvider.class);
resourceConfig.register(org.glassfish.jersey.grizzly2.httpserver.GrizzlyHttpContainerProvider.class);

This means that Jetty is a framework- it is taking control of initialising your code. If you are writing code you should not force anyone to do this. You should always pass fully formed objects around; that way the person using your code (even if it’s you) can create that object however they please. Don’t impose your views on others programatically.

(Note: I know that in Jetty you can use fully formed objects, but it’s a bit of a hack around. The documentation, and things that sit on Jetty such as swagger, assume by default you will use class registration. I wish it didn’t!)

Simple take away — if your code has an instanceof or uses a reference to a .class file, that’s a big code smell. You can write your code in a better way, and you should.