11

I'm currently working on a webapp where we often need to condition some server logic based on the page that is going to be returned to the user.

Each page is given a 4-letter page code, and these page codes are currently listed in a class as static Strings:

public class PageCodes {
    public static final String FOFP = "FOFP";
    public static final String FOMS = "FOMS";
    public static final String BKGD = "BKGD";
    public static final String ITCO = "ITCO";
    public static final String PURF = "PURF";
    // etc..
}

And often in the code we see code like this (1st form):

if (PageCode.PURF.equals(destinationPageCode) || PageCodes.ITCO.equals(destinationPageCode)) {
    // some code with no obvious intent
} 
if (PageCode.FOFP.equals(destinationPageCode) || PageCodes.FOMS.equals(destinationPageCode)) {
    // some other code with no obvious intent either
} 

Which is kind of horrible to read because it doesn't show what common property of these pages led the author of the code to put them together here. We have to read the code in the if branch to understand.

Current solution

These ifs have partly been simplified using lists of pages which are declared by different people in different classes. This makes the code look like (2nd form):

private static final List<String> pagesWithShoppingCart = Collections.unmodifiableList(Arrays.asList(PageCodes.ITCO, PageCodes.PURF));
private static final List<String> flightAvailabilityPages = Collections.unmodifiableList(Arrays.asList(PageCodes.FOMS, PageCodes.FOFP));

// later in the same class
if (pagesWithShoppingCart.contains(destinationPageCode)) {
    // some code with no obvious intent
} 
if (flightAvailabilityPages.contains(destinationPageCode)) {
    // some other code with no obvious intent either
} 

...which is expresses the intent much better. But...

Current Problem

The problem here is that if we add a page, we would in theory need to go through all the code base to find if we need to add our page to an if() or to a list like that.

Even if we moved all those lists to the PageCodes class as static constants, it still needs discipline from developers to check if their new page fits in any of those lists and add it accordingly.

New solution

My solution was to create an enum (because there is a finite well-known list of page codes) where each page contain some properties that we need to set:

public enum Page {
    FOFP(true, false),
    FOMS(true, false),
    BKGD(false, false),
    PURF(false, true),
    ITCO(false, true),
    // and so on

    private final boolean isAvailabilityPage;
    private final boolean hasShoppingCart;

    PageCode(boolean isAvailabilityPage, boolean hasShoppingCart) {
        // field initialization
    }

    // getters
}

Then the conditional code now looks like this (3rd form):

if (destinationPage.hasShoppingCart()) {
    // add some shopping-cart-related data to the response
}
if (destinationPage.isAvailabilityPage()) {
    // add some info related to flight availability
}

Which is very readable. In addition, if someone needs to add a page, he/she is forced to think about each boolean and whether this is true or false for his new page.

New Problem

One problem I see is that there will be maybe 10 booleans like this, which makes the constructor really big and it might be hard to get the declaration right when you add a page. Does anyone have a better solution?

13

You can take the idea one step further and define an enum for your page features instead of using booleans.

This makes it easy to add/remove a feature to a page and makes the page definitions instantly readable even when there are 30-40 potential features.

public enum PageFeature {
    AVAIL_PAGE,
    SHOPPING_CART;
}

public enum Page {
    FOFP(AVAIL_PAGE),
    FOMS(AVAIL_PAGE),
    BKGD(),
    PURF(SHOPPING_CART, AVAIL_PAGE),

    private final EnumSet<PageFeature> features;

    PageCode(PageFeature ... features) {
       this.features = EnumSet.copyOf(Arrays.asList(features));
    }

    public boolean hasFeature(PageFeature feature) {
       return features.contains(feature);
    }
 }
  • I thought about this, but here, the developer is not forced to give an answer to all the questions "is it an avail page?", "does it have a shopping cart?" etc. When there is a lot of them, you easily forget one. – Joffrey Jan 15 '16 at 17:01
  • 5
    @Joffrey When you think about it, having ten booleans doesn't force them to give an answer either, at least not an answer they think about. The most likely scenario is that they'll just copy another page's definition or just let the IDE fill all parameters with false, then modify one or two (probably the wrong one, as it's so hard to keep track). There is no perfect protection from stupid, and there comes a point where you must trust your developers to do the right thing. – biziclop Jan 15 '16 at 17:09
  • Right, I didn't think about it this way :) And I don't think the small extra "fool-proof" given by the boolean is worth the degradation in lisibility, so I'll probably go with the vararg solution. Thanks for your insight! – Joffrey Jan 15 '16 at 18:30
  • 1
    Good solution, upvoted. Though the part of me that coded on 6502s wants to cram everything into bits. :-) – user949300 Jan 15 '16 at 18:37
  • @user949300 the first vararg-like solution I thought about was actually bit masks :) but a true vararg with an enum type is cleaner – Joffrey Jan 15 '16 at 18:41

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.