Is there something wrong with the local class defined withn a constructor body

Question

I have the following enum:

enum FilterFactory {

    INSTANCE;

    private final Map<FilterType, Creator> creators;

    private FilterFactory() {
        creators = new HashMap<>();

        class SimplCreator implements FilterCreator{
            @Override
            public FilterDTO createDto() {
                return new FilterDTO();
            }
        } //Local class within the constructor

        creators.put(FilterType.DATE, new FilterCreator(){
            @Override
            public FilterDTO createDto() {
                return new DynamicDTO();
            }
        });

        creators.put(FilterType.DROP_DOWN_LIST, new SimplCreator());
        creators.put(FilterType.FIELD, new SimplCreator());
    }

    private static interface Creator{
        public FilterDTO createDto();
    }
    //Other staff
}

The thing is I've never used the local classes within constructors bodies. Can it cause some bugs, is it bad to do so? Also, the constructor enu's constructor.

Answer 1

Your approach is fine, but in Java 8 you can make it a bit nicer using method references or lambdas (and replacing your Creator with the more standard Supplier<FilterDTO>):

import java.util.function.Supplier;

enum FilterFactory {

    INSTANCE;

    private final Map<FilterType, Supplier<FilterDTO>> creators;

    private FilterFactory() {
        creators = new EnumMap<>(FilterType.class); // a bit more efficient :)
        creators.put(FilterType.DATE, DynamicDTO::new);
        creators.put(FilterType.DROP_DOWN_LIST, SimpleDTO::new);
        creators.put(FilterType.FIELD, SimpleDTO::new);
    }

    // other stuff ... 
}

Here I've used the constructor's method reference to instantiate the Supplier<FilterDTO>. I could have just as well used a lambda expression that says "given nothing, give me a FilterDTO":

creators.put(FilterType.DATE, () -> new DynamicDTO());
...

The two variants (method reference vs lamdbas) are basically equivalent, and you should use whichever is clearer to you (citation: Java's language architect himself). I personally find the method reference visually clearer, though it does take a bit of getting used to.

Answer 2

The only problem I see, is that you are creating a new instances of FilterCreator for each instance of FilterFactory (which takes more memory). You could prevent that by creating some constants :

enum FilterFactory {

    INSTANCE;

    private final Map<FilterType, Creator> creators = new HashMap<>();

    private static final SimplCreator DEFAULT_CREATOR = new Creator() {
        @Override
        public FilterDTO createDto() {
            return new FilterDTO();
        }
     }

    private static final FilterCreator DYNAMIC_CREATOR = new Creator(){
        @Override
        public FilterDTO createDto() {
            return new DynamicDTO();
        }
    }

    private FilterFactory() {
        creators.put(FilterType.DATE, DYNAMIC_CREATOR);
        creators.put(FilterType.DROP_DOWN_LIST, DEFAULT_CREATOR);
        creators.put(FilterType.FIELD, DEFAULT_CREATOR);
    }

    private static interface Creator{
        FilterDTO createDto();
    }
    //Other stuff
}