4
\$\begingroup\$

I want to create a factory to return all Source instances that match given entry parameters- that is, I want to iterate through some Collection of objects and for each entry check boolean method. I thought about enum, as I'm guaranteed to iterate through them all. I came up with something like this:

simple interface

public interface Source {

    public boolean isPresent(SourceCalculationData data);

    public Source2 getType();
}

my take on enum factory:

public enum Source2 {

    IB("IB", SourceIb.class),
    CCWEW("BOT", SourceCcwew.class),
    OBD("CUF", SourceCuf.class),
    PB("PB", SourcePb.class),
    COK("COK", SourceCok.class),
    CCZEW("CCZ", SourceCczew.class),
    CRM("CRM", SourceCrm.class),
    APP("APP", SourceApp.class);

    private static final Logger SENSITIVE_LOGGER = SensitiveLoggerFactory.getSensitiveLogger(Source2.class);

    private String configChannel;

    private Class<? extends Source> clazz;

    private Source2(String configChannel, Class<? extends Source> clazz) {
        this.configChannel = configChannel;
        this.clazz = clazz;
    }

    public Class<? extends Source> getClazz() {
        return clazz;
    }

    public String getConfigChannel() {
        return configChannel;
    }

    public Source getSource() {
        return getAction(clazz);
    }

    private Source getAction(Class<? extends Source> source2Class) {

        try {
            return source2Class.newInstance();

        } catch (InstantiationException | IllegalAccessException e) {
            SENSITIVE_LOGGER.warn("<< Exception occured during init of {}, returning unkown source.", source2Class);
            return new SourceUnkown();
        }

    }

and use case:

        final SourceCalculationData data = new SourceCalculationData();

        final EnumSet<Source2> presentSources = EnumSet.noneOf(Source2.class);
        for (final Source2 source2ToCheck : Source2.values()) {
            if (source2ToCheck.getSource().isPresent(data)) presentSources.add(source2ToCheck);
        }

I'm pretty sure this is not the best code in the world. Any suggestions what will make it better?

\$\endgroup\$
  • \$\begingroup\$ Greetings & Salutations, to prevent this from being closed; I would take out the implementation of SourceIb, since we don't review stubs. Ideally you would also figure out your logging and error handling before submitting the code. \$\endgroup\$ – konijn Oct 24 at 11:47
  • \$\begingroup\$ thank you @konijn, I edited my post \$\endgroup\$ – Khayn Oct 24 at 12:45
3
\$\begingroup\$

Your implementation is good, but I would suggest following points to improve:

  1. It is really hard to test/mock source2ToCheck.getSource().
  2. Source2 contains a lot logic/functionality (it is source type and factory) it has to be simplified.
  3. getSource() creates new course object it is confusing
  4. getSource(...) calls getAction(...) that create a source object. It is wired.
  5. Be aware source2Class.newInstance() is deprecated since java 9 source2Class.getDeclaredConstructor().newInstance()

Suggestions:

  • I would propose to create dedicated interface/class for source factory
  • Rename Source2 to SourceType.
  • Throw custom exception CustomCantCreateSourceException instead of return new SourceUnkown(). Because it is real exceptional situation.
  • Remove public methods modifier from Source interface
  • Minor enhancements you can find in following code

Source Factory interface:

public interface SourceFactory {
    Source create(SourceType type);
}

Factory implementation:

public class SourceFactoryImpl implements SourceFactory {
    @Override
    public Source create(SourceType type) {
            try {
                return type.getClazz().getDeclaredConstructor().newInstance();
            } catch (InstantiationException | IllegalAccessException
                    | NoSuchMethodException | InvocationTargetException e) {
                SENSITIVE_LOGGER.warn("<< Exception occured during init of {}, returning unkown source.", source2Class);
                throw new CustomCantCreateSourceException();
            }
    }
}

Renamed Source2:

public enum SourceType {

    IB("IB", SourceIb.class),
    CCWEW("BOT", SourceCcwew.class),
    OBD("CUF", SourceCuf.class),
    PB("PB", SourcePb.class),
    COK("COK", SourceCok.class),
    CCZEW("CCZ", SourceCczew.class),
    CRM("CRM", SourceCrm.class),
    APP("APP", SourceApp.class);

    private static final Logger SENSITIVE_LOGGER = SensitiveLoggerFactory.getSensitiveLogger(SourceType.class);

    private String configChannel;

    private Class<? extends Source> clazz;

    private SourceType(String configChannel, Class<? extends Source> clazz) {
        this.configChannel = configChannel;
        this.clazz = clazz;
    }

    public Class<? extends Source> getClazz() {
        return clazz;
    }

    public String getConfigChannel() {
        return configChannel;
    }
}

Usage:

public class SourceService {
    private final SourceFactory factory;

    public SourceService(SourceFactory factory) {
        this.factory = factory;
    }

    public EnumSet<SourceType> fillSources() {
        final SourceCalculationData data = new SourceCalculationData();

        final EnumSet<SourceType> presentSources = EnumSet.noneOf(SourceType.class);
        for (final SourceType source2ToCheck : SourceType.values()) {
            if (factory.create(source2ToCheck).isPresent(data)) {
                presentSources.add(source2ToCheck);
            }
        }
        return presentSources;
    }
}   
\$\endgroup\$
  • \$\begingroup\$ thank you! just what I asking for ;-) \$\endgroup\$ – Khayn Oct 28 at 12:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.