No Open Blockers

The exceptions lecture of the semester is one of the trickier ones I deal with. Part of the issue is that I try to cram it into a single class, a strategy that has backfired on me twice now. Another difficulty is the fact that while there is quite a bit of technical knowledge as far as throwing and catching exceptions, I could easily spend double that time covering design and best practices.

I took a bug today that covers the error messages shown to a user when they input an invalid channel name. Currently, we simply say “Invalid channel name” without any indication of why… was it too short, did it contain invalid characters, was it already in use, etc. Our name validation is correct, but from a user experience stand point, it could use a little love (this was exacerbated by the multi-org addition where a channel name could be taken without you being able to see that data otherwise).

There are a number of different validations we run against the name. A good sampling of them is found in the following snippet:

1
2
3
4
5

if (cname == null || 
    !Pattern.compile(CHANNEL_NAME_REGEX).matcher(cname).find() ||
    cname.length() < 6) {
        throw new InvalidChannelNameException();
    }

Again, the logic is fine. Any of the circumstances in the if clause represent an invalid name. But now we have the need to carry more information to better indicate why it failed.

I started by editing the exception class itself. The constructor now accepts two parameters. The first is the channel name; if we’re saying the exception represents an invalid channel name then it makes sense it should indicate exactly what was invalid. This also saves the UI some work in remembering what was specified that caused the error.

The second is an enum (defined as an inner class of the exception class itself) that describes the reason for the failure. The enum (currently – I’m kinda writing this as I work on it) looks like the following:

1
2
3
4
5
6

public enum Reason {
    REGEX_FAILS,
    TOO_SHORT,
    IS_MISSING,
    NAME_IN_USE
}

I’m a big fan of the enum for something like this. It’s clunky to make subclasses of InvalidChannelNameException just to indicate the different possible reasons for failure, and frankly a series of instanceof checks to later use that information would just be ugly. A similar if/else structure would be needed if I used a series of String constants stored in the exception’s message attribute.

Some camps would argue against the inner enum (is that the way to say an enum defined as an inner class?). Their reasons would center around the usage syntax. I actually feel the opposite way; the syntax I have to use since it’s defined as an inner enum reads really well in my opinion:

1
2
3
4

if (cname == null) {
    throw new InvalidChannelNameException(cname,
        InvalidChannelNameException.Reason.IS_MISSING);
}

Also realize that using an enum lets IntelliJ only show me the enum values when I get to that parameter. There’s no need to hunt around for the possible constants.

Once the extra information was in there, it was just a matter of getting the UI to display a custom message based on what actually happened, cleanly using a switch statement since I’m using enums.

1
2
3
4
5
6
7
8
9
10
11
12

catch (InvalidChannelNameException ferengi) {
    switch (ferengi.getReason()) {
        case IS_MISSING:
            errors.add(ActionMessages.GLOBAL_MESSAGE,
              new ActionMessage("edit.channel.invalidchannelname.missing"));
            break;
 
        case REGEX_FAILS:
            errors.add(ActionMessages.GLOBAL_MESSAGE,
              new ActionMessage("edit.channel.invalidchannelname.regex"));
            break;
[snip]

Again, IntelliJ can auto-complete here, this time only showing the possible values (i.e. the enum values) each time I start a new case clause.