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.

There is nothing wrong with doing this. In fact, it's probably good practice, since there is no need to complicate the code for other developers by exposing SimplCreator outside of the method that uses it. (By the way, if FilterType is an enum, you probably should use [EnumMap](http://docs.oracle.com/javase/8/docs/api/java/util/EnumMap.html) rather than HashMap.) — VGR, Aug 10 '15 at 13:55
Also, if you're using Java 8 this sounds like a job for lambdas :) — yshavit, Aug 10 '15 at 14:25
@yshavit Couldn't you provide the answer with labdas for Java 8? — St.Antario, Aug 11 '15 at 04:10

Francis Toth · Answer 1 · 2015-08-10T14:37:48.117

1

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
}

edited Aug 10 '15 at 14:37

answered Aug 10 '15 at 13:55

Francis Toth

1,595
1
11
23

See, I'd not like to expose the DEFAULT_CREATOR object outside the constructor.... – St.Antario Aug 10 '15 at 14:01
I believe you meant `=new Creator() {`? – Eric Stein Aug 10 '15 at 14:22
"for each instance of FilterFactory" -- all one instances? FilterFactory is a single-field-enum singleton. :) – yshavit Aug 10 '15 at 14:30
@yshavit : right ! considering this, creating local classes inside the constructor shouldn't be a problem. However, code is cleaner that way, don't you think ? – Francis Toth Aug 10 '15 at 14:39
St.Antario : Could you explain why ? – Francis Toth Aug 10 '15 at 14:39
I dunno, they're both more or less same clarity to me -- I think the clearest thing would be to use lambdas if possible, and of the verbose (pre-lambda) options, they're all pretty equivalent to my mind. My IDE would collapse the lines and make them look like lambdas, anyway. :) But seeing as your answer said that the duplicate instances are "the only problem," I thought it's relevant that there aren't any. :) – yshavit Aug 10 '15 at 14:59

score 1 · Accepted Answer · edited May 23 '17 at 10:26

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.

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

2 Answers2