4
\$\begingroup\$

I am managing the changing of views in an Android app using this enum and associated methods. I am particularly interested in the Page enum's use of overriding previous() and next() for the first and last elements.

  1. Is this readable?
  2. Is this idiomatic?
  3. Is there a preferable data structure?
  4. Are there general stylistic improvements that could be made?

public enum Page {
  INTRO { @Override public Page previous() {return this; } },
  PUBLIC_PROFILE, PRIVATE_PROFILE, SOCIAL_ACCOUNTS, LOOPS, INVITE,
  FINISHED { @Override public Page next() { return this; }; };

  public Page next() {
    return values()[ordinal()+1];
  }

  public Page previous() {
    return values()[ordinal()-1];
  }
}

private Page page = Page.INTRO;

private void setPage(Page page) { ... }

private void nextPage() {
  page = page.next();
  setPage(page);
}

private void previousPage() {
  page = page.previous();
  setPage(page);
}
\$\endgroup\$
3
\$\begingroup\$

Interesting question. Overriding methods in enum instances is not something I have encountered (that I recall). This is not to say it's a bad thing, just not very common. (P.S. having just claimed not to have encountered enum overrides, I see it is used in this answer given shortly before you asked this very question

The concept is something I have solved using different mechanisms before, but I can understand why having the override makes sense to you.

What I don't understand is why you did not just use a regular bound-clipping system. Why do you need the overrides at all?

Consider these methods:

  public Page next() {
    if (ordinal() == values().length - 1) {
        return this;
    }
    return values()[ordinal() + 1];
  }

  public Page previous() {
    if (ordinal() == 0) {
        return this;
    }
    return values()[ordinal() - 1];
  }

There is no reason to override them at all.

On the other hand, if you do override them, then please use a standard indentation for the override methods. Cramming it all on one line is not helpful:

public enum Page {
    INTRO {
        @Override
        public Page previous() {
            return this;
        }
    },
    PUBLIC_PROFILE,
    PRIVATE_PROFILE,
    SOCIAL_ACCOUNTS,
    LOOPS,
    INVITE,
    FINISHED {
        @Override
        public Page next() {
            return this;
        };
    };
\$\endgroup\$
  • 1
    \$\begingroup\$ The indentation was an experiment in readability. I agree that it wasn't a great success. The regular bounds checking is certainly a better answer here. I was too close to the code and enjoying the realization that you can override methods in enum members. Thanks. \$\endgroup\$ – Derek May 20 '15 at 16:17
2
\$\begingroup\$

As far as the enum is concerned, the solution seems fine. Even clever, but still very readable, so that's not a problem. Using ordinal () is usually not recommended, but in this case I think it's justified.

An alternative dates structure would be an ADT (abstract data type) representing the page transition actions, implemented as a regular class, with the same methods that you implemented in the enum. In principle, creating an ADT would be the right thing to do, but if you don't anticipate further evolution of managing your page transitions, then your simple solution is fine. If later you find yourself wanting to add any more logic to the enum, that will be a good time to ditch the enum.

\$\endgroup\$
  • \$\begingroup\$ There are times where I modify the local page variable elsewhere to skip a to a specific screen so having it exposed is helpful. \$\endgroup\$ – Derek May 20 '15 at 16:33
  • \$\begingroup\$ Most other sets of pages don't require even this level of logic so the enum has worked well. This was the first time the set was ordered. \$\endgroup\$ – Derek May 20 '15 at 16:34
  • \$\begingroup\$ Actually I totally misread, there was no local variable, I see now it's a member. Deleted that remark \$\endgroup\$ – janos May 20 '15 at 16:59
1
\$\begingroup\$

If Page is an inner class, then it should be a public static enum, because it doesn't rely on anything in the enclosing class.


In these methods, page = … is weird.

private void nextPage() {
  page = page.next();
  setPage(page);
}

private void previousPage() {
  page = page.previous();
  setPage(page);
}

Is setPage() unable to do its job? I would expect something more like…

public SomethingConstructor() {
    setPage(Page.INTRO);
}

private void nextPage() {
    setPage(page.next());
}

private void previousPage() {
    setPage(page.previous());
}

There isn't anything in the Page to indicate that FINISHED.next() returns itself. The only way to test is write if (page.next() == page) …, which is cumbersome. There should be hasNext() and hasPrevious() methods. I'd write it this way:

public static enum Page {
    INTRO,
    PUBLIC_PROFILE,
    PRIVATE_PROFILE,
    SOCIAL_ACCOUNTS,
    LOOPS,
    INVITE,
    FINISHED;

    public boolean hasNext() {
        return this != FINISHED;
    }

    public Page next() throws NoSuchElementException {
        if (!hasNext()) {
            throw new NoSuchElementException();
        }
        return values()[ordinal() + 1];
    }

    …
}

private void nextPage() {
    if (page.hasNext()) {
        setPage(page.next());
    }
}

…
\$\endgroup\$

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.