Java enum for mapping classes and specific URL

Question

I have a ZK project in Java 6 (ZK is like ASP.NET but with zul files in stead of aspx). For showing and creating/updating the catalogs I created an abstracted view. This all works with 1 viewmodel at the back. To go to the right catalog I provide a String what is the catalog class his name.

Of course with just that String I don't have enough data so I created a mapper. At first the mapper was an HashMap with a String as key. Then I refactored to Enum cause in mine opinion enum is lightweight, and getting the correct values should go faster.

The names of the enum are very important cause I do the lookup like this:

CatalogMapper.valueOf(currentClass.toUpperCase());

Other point is the link to the detailscreen. For this instant it is all the the same path. I didn't cut it to concatenate in the constructor for the reason that I don't have to concatenate then. (It takes a little more cpu but actually it's negligible.

Questions:

• Is this good practice?
• Could the mapper be done otherwise so I can make the names more correct?(adding _ to the name)
• Should I do the concatenation in the constructor?
• Should I use reflection? (the pojos could be in subpackages in the 'catalog' package)
• Was the Map a better solution for Enum troubles mine insight?

public enum CatalogMapper {

    BERANK(BERank.class, "/WEB-INF/webpages/zk/catalog/details/berankdetails.zul", BERankRepository.class, QBERank.bERank._super),
    COMPONENT(Component.class, ComponentRepository.class, QComponent.component._super),
    CONTINENT(Continent.class, "/WEB-INF/webpages/zk/catalog/details/continentdetails.zul", ContinentRepository.class, QContinent.continent._super),
    COUNTRY(Country.class, "/WEB-INF/webpages/zk/catalog/details/countrydetails.zul", CountryRepository.class, QCountry.country._super),
    FEATURE(Feature.class, FeatureRepository.class, QFeature.feature._super),
    FUNCTION(Function.class, FunctionRepository.class, QFunction.function._super),
    ISSUEADJUNCT(IssueAdjunct.class, "/WEB-INF/webpages/zk/catalog/details/issueadjunctdetails.zul", IssueAdjunctRepository.class, QIssueAdjunct.issueAdjunct._super),
    ISSUECONTACT(IssueContact.class, IssueContactRepository.class, QIssueContact.issueContact._super),
    ISSUEPRIORITY(IssuePriority.class, IssuePriorityRepository.class, QIssuePriority.issuePriority._super),
    ISSUESTATUS(IssueStatus.class, IssueStatusRepository.class, QIssueStatus.issueStatus._super),
    LANGUAGE(Language.class, "/WEB-INF/webpages/zk/catalog/details/languagedetails.zul", LanguageRepository.class, QLanguage.language._super),
    MULTILATERALTYPE(MultilateralType.class, MultilateralTypeRepository.class, QMultilateralType.multilateralType._super),
    MISSIONTYPE(MissionType.class, MissionTypeRepository.class, QMissionType.missionType._super),
    NATORANK(NatoRank.class, NatoRankRepository.class, QNatoRank.natoRank._super),
    OPERATIONMODULE(OperationModule.class, OperationModuleRepository.class, QOperationModule.operationModule._super),
    OPERATIONNOTSTANDARDMATERIAL(OperationNotStandardMaterial.class, "/WEB-INF/webpages/zk/catalog/details/materialdetails.zul", OperationNotStandardMaterialRepository.class, QOperationNotStandardMaterial.operationNotStandardMaterial._super),
    OPERATIONSCENARIO(OperationScenario.class, "/WEB-INF/webpages/zk/catalog/details/operationscenariodetails.zul", OperationScenarioRepository.class, QOperationScenario.operationScenario._super),
    OPERATIONSTATUS(OperationStatus.class, "/WEB-INF/webpages/zk/catalog/details/operationstatusdetails.zul", OperationStatusRepository.class, QOperationStatus.operationStatus._super),
    OPERATIONSTANDARDMATERIAL(OperationStandardMaterial.class, OperationStandardMaterialRepository.class, QOperationStandardMaterial.operationStandardMaterial._super),
    OPERATIONSUBMODULE(OperationSubModule.class,"/WEB-INF/webpages/zk/catalog/details/operationsubmoduledetails.zul", OperationSubModuleRepository.class, QOperationSubModule.operationSubModule._super),
    SECURITYLEVELBE(SecurityLevelBE.class, SecurityLevelBeRepository.class, QSecurityLevelBE.securityLevelBE._super),
    SECURITYLEVELNATO(SecurityLevelNato.class, SecurityLevelNatoRepository.class, QSecurityLevelNato.securityLevelNato._super),
    SEX(Sex.class, SexRepository.class, QSex.sex._super),
    TRANSPORTMEAN(TransportMean.class, TransportMeanRepository.class, QTransportMean.transportMean._super),
    TRANSPORTMODE(TransportMode.class, TransportModeRepository.class, QTransportMode.transportMode._super);

    private String detailUrl = "/WEB-INF/webpages/zk/catalog/details/defaultdetails.zul";
    private QAbstractCatalog abstractCatalog;
    private Class repoClass;
    private Class catalogClass;

    private CatalogMapper(Class catalogClass, String detailUrl, Class repoClass, QAbstractCatalog abstractCatalog) {
        this(catalogClass, repoClass, abstractCatalog);
        this.detailUrl = detailUrl;
    }

    private CatalogMapper(Class catalogClass, Class repoClass, QAbstractCatalog abstractCatalog) {
        this.catalogClass = catalogClass;
        this.abstractCatalog = abstractCatalog;
        this.repoClass = repoClass;
    }

    public String getDetailUrl() {
        return detailUrl;
    }

    public QAbstractCatalog getAbstractCatalog() {
        return abstractCatalog;
    }

    public Class getRepoClass() {
        return repoClass;
    }

    public Class getCatalogClass() {
        return catalogClass;
    }
}

I would like to explain some things because I know ZK is not so widely used. First of all, I have no choice to take whatever framework I want, need to do what they ask with the given frameworks as they want.

Secondly a little more explication of what I created. As you see this class is the real engine what let it all turn.

If I need now a catalog somewhere in a page I just need to set this:

<catalog value="@bind(vm.sex)" catalog="sex" constraint="no empty" readonly="false"/>

This is enough to show in 2 textboxes the code and description, and you can get a popup for choosing(and creating if you have the rights) another value. As you can see I needed to put a value catalog to. This was only needed when the value was null so I couldn't know to what catalog he was pointing.

With the menu I just need to set href="....../catalog?sex and it works. There is an initiator before the page what checks the GET param for validity.

Take in mind that I list all the catalogs with just 1 zul page with 1 viewmodel, and for creation/update the catalogs the same viewmodel, just editpage can be different because that's dependend on the catalogs parameters.

If this was not done => 1 zul with 1 viewmodel for listing + 1 zul and viewmodel for edit/create. As you can see a lot of catalogs, so a lot of work zuls and viewmodels. (and I don't speak yet of the custom component.)

As for refactoring, I don't disagree with changing the classname => watch out or you will have problems. On the other hand, whole ZK is build on that principal. (And I take misadvantage of that)

Example:

<label value="@bind(vm.label)"/>

This means that your viewmodel needs to have a getLabel and setLabel. Refactoring the getters name will lead to faults because zul pages are not automatically refactored.

Same for initialisation of the viewmodel:

<window apply="org.zkoss.bind.BindComposer" viewModel="@id('vm') @init('be.test.SomeVM')" >

As you can see the FQN is hardcoded in the zul with no auto refactoring.

Answer 1

I don't think it is good practice for this sole reason: You are relating a class with its name, meaning that now you cannot refactor the class names without manually changing the enum values.
This can ultimately lead to compile time errors or errors when accessing a web page, it will create maintenance tasks, though I do not think they can cripple a system.

Furthermore, you are using raw Class objects, they should at very least be typed as Class<?> using generics.

And lastly, you should be making variables final wherever they can be, which is not happening in the enum approach right now, despite that it could've been done using this approach.

As alternative I suggest to start by creating a class that holds the data for every CatalogMapping:

public class CatalogMapping {
    private static final String DEFAULT_DETAIL_URL = "/WEB-INF/webpages/zk/catalog/details/defaultdetails.zul";

    private final Class<?> catalogClass;
    private final String detailUrl;
    private final QAbstractCatalog abstractCatalog;
    private final Class<?> repositoryClass;

    public CatalogMapping(final Class<?> catalogClass, final String detailUrl, final QAbstractCatalog abstractCatalog, final Class<?> repositoryClass) {
        this.catalogClass = Objects.requireNonNull(catalogClass, "catalogClass");
        this.detailUrl = Objects.requireNonNull(detailUrl, "detailUrl");
        this.abstractCatalog = Objects.requireNonNull(abstractCatalog, "abstractCatalog");
        this.repositoryClass = Objects.requireNonNull(repositoryClass, "repositoryClass");
    }

    public CatalogMapping(final Class<?> catalogClass, final QAbstractCatalog abstractCatalog, final Class<?> repositoryClass) {
        this(catalogClass, DEFAULT_DETAIL_URL, abstractCatalog, repositoryClass);
    }

    public Class<?> getCatalogClass() {
        return catalogClass;
    }

    public String getDetailUrl() {
        return detailUrl;
    }

    public QAbstractCatalog getAbstractCatalog() {
        return abstractCatalog;
    }

    public Class<?> getRepositoryClass() {
        return repositoryClass;
    }
}

For Objects.requireNonNull you'll need to make your own replacement. I suggest you to also place it in an Objects class such that you can simply remove that class and fix imports once you upgrade to Java 7+.

Another important note is that if the catalog and repository classes are bounded by another class (or interface), then you should use that bound in your Class<?> literals. So assuming that every catalog class extends QCatalog and every repository extends QRepository, you will need:

• Class<? extends QCatalog> catalogClass
• Class<? extends QRepository> repositoryClass

If every catalog also has a generic relation with their repository, then even more interesting patterns can be thought of, but it is always a question of whether you need so much compile time protection.

Then you need to store your mappings somewhere, I suggest a singleton enum for that:

public enum CatalogMap {
    INSTANCE;

    private final Map<Class<?>, CatalogMapping> mapping = new HashMap<Class<?>, CatalogMapping>()
    {{
        put(BERank.class, new CatalogMapping(BERank.class, "/WEB-INF/webpages/zk/catalog/details/berankdetails.zul", BERankRepository.class, QBERank.bERank._super));
        //etc.
    }};

    private CatalogMapping getMappingInternal(final Class<?> clazz) {
        Objects.requireNonNull(clazz, "clazz");
        if (!mapping.containsKey(clazz)) {
            throw new IllegalStateException("No mapping present for " + clazz);
        }
        return mapping.get(clazz);
    }

    public static CatalogMapping getMapping(final Class<?> clazz) {
        return CatalogMap.INSTANCE.getMappingInternal(clazz);
    }
}

For this it also holds that Class<?> should be changed to Class<? extends QCatalog> if possible.

You can now get the values with CatalogMap.getMapping(...) versus the old CatalogMapper.valueOf(...)

(Small note: I barely use Java 6, so hopefully I have not accidentally used a Java 7/8 feature)

Answer 2

I have a strong feeling that you are somewhat off track. I had not heard of the ZK framework until I read your post, but a visit to their website confirms that it should be a better way to do a Java website, albeit at a licencing cost. (I am very familiar with Spring MVC and Struts, know a little PHP and have used ASP.net for one project).

My observations are:

• Your enum seems to be mapping a string variable (enum name) to a URL and a pointer to a model class somewhere in a tree of models & pages (correct me if I am wrong). That is bread and butter work for any web framework, and should not require the complexity you are presenting here. If ZK lives up to its advertising hype, it will provide this functionality out of the box.
• You are stretching the intended use of enums. They are intended as type-safe constants. Once they sprout multiple attributes and overloaded constructors, they should be replaced (in my opinion) by Java classes.
• Don't worry about performance. Looking up a value in a HashMap or looking up an enum is going to be a tiny proportion of the time used to render a page. So ignore it as a design constraint, and revisit it only if the optimiser says it is slow.
• There is a great deal of commonality in your enum entries. All of them refer to the class type (e.g. Component.class) and most refer to a _super method. Again this points towards using standard classes rather than an enum.

Best of luck...

Answer 3

• When I glanced at your code, without even reading it, it looked too complex.

• I also do not think an enum is appropriate here. Performance is not an issue for 20 items. If you are worried about concurrency, use an immutable map.

• Instead of using enum or map, maybe all the elements you group in each enum value should really be grouped in classes (using OO). Maybe put everything in the XXXXRepository class. It looks odd that you have all those nearly identical class names and that you have to define their relationship in an enum. (We don't much about your classes, so I'm not sure this makes sense.)

• I don't like that you have two types of constructors in your enum and that sometimes detailUrl is null. If you still want to keep the optional argument, at least only keep one constructor where the optional argument is at the end and explicitly pass a null when there is no value.

• The QBERank.bERank._super and such look weird.