Using nested public classes to organize constants

Question

I'm working on an application with many constants. At the last code review it came up that the constants are too scattered and should all be organized into a single "master" constants file. The disagreement is about how to organize them. The majority feel that using the constant name should be good enough, but this will lead to code that looks like this:

public static final String CREDITCARD_ACTION_SUBMITDATA = "6767";
public static final String CREDITCARD_UIFIELDID_CARDHOLDER_NAME = "3959854";
public static final String CREDITCARD_UIFIELDID_EXPIRY_MONTH = "3524";
public static final String CREDITCARD_UIFIELDID_ACCOUNT_ID = "3524";
...
public static final String BANKPAYMENT_UIFIELDID_ACCOUNT_ID = "9987";

I find this type of naming convention to be cumbersome. I thought it might be easier to use public nested class, and have something like this:

public class IntegrationSystemConstants
{
    public class CreditCard
    {
        public static final String UI_EXPIRY_MONTH = "3524";
        public static final String UI_ACCOUNT_ID = "3524";
        ...
    }
    public class BankAccount
    {
        public static final String UI_ACCOUNT_ID = "9987";
        ...
    }
}

This idea wasn't well received because it was "too complicated" (I didn't get much detail as to why this might be too complicated). I think this creates a better division between groups of related constants and the auto-complete makes it easier to find these as well. I've never seen this done though, so I'm wondering if this is an accepted practice or if there's better reasons that it shouldn't be done.

Answer 1

I don't agree with either of the two proposals.

Constants should be in their pertinent classes, not in an all-constant class in either of the two forms proposed.

There shouldn't be constants-only classes/interfaces.

A class CreditCard (not an internal class) should exist. This class/interface has methods relative to credits cards as well as the constants UI_EXPIRY_MONTH and UI_ACCOUNT_ID.

There should exist a class/interface BankAccount (not an internal class). This class/interface has methods relative to bank accounts as well as constant UI_ACCOUNT_ID.

For example in the Java API every class/interface has its constants. They are not in a Constants class/interface with hundreds of constants, either grouped into inner classes or not.

For example, these constants pertinent to result sets are in the interface ResultSet:

static int  CLOSE_CURSORS_AT_COMMIT 
static int  CONCUR_READ_ONLY 
static int  CONCUR_UPDATABLE 
static int  FETCH_FORWARD 
static int  FETCH_REVERSE 
static int  FETCH_UNKNOWN 
static int  HOLD_CURSORS_OVER_COMMIT 
static int  TYPE_FORWARD_ONLY 
static int  TYPE_SCROLL_INSENSITIVE 
static int  TYPE_SCROLL_SENSITIVE 

This interface has method signatures relative to result sets, and implementing classes implement that behavior. They are not mere constant-holders.

These constants pertinent to UI actions are in the interface javax.swing.Action:

static String   ACCELERATOR_KEY 
static String   ACTION_COMMAND_KEY 
static String   DEFAULT 
static String   LONG_DESCRIPTION 
static String   MNEMONIC_KEY 
static String   NAME 
static String   SHORT_DESCRIPTION 
static String   SMALL_ICON 

Implementing classes have behavior relative to UI actions, they are not mere constant holders.

I know at least one "constants" interface (SwingConstants) with no methods but it doesn't have hundreds of constants, just a few, and they are all related to directions and positions of UI elements:

static int  BOTTOM 
static int  CENTER 
static int  EAST 
static int  HORIZONTAL 
static int  LEADING 
static int  LEFT 
static int  NEXT 
static int  NORTH 
static int  NORTH_EAST 
static int  NORTH_WEST 
static int  PREVIOUS 
static int  RIGHT 
static int  SOUTH 
static int  SOUTH_EAST 
static int  SOUTH_WEST 
static int  TOP 
static int  TRAILING 
static int  VERTICAL 
static int  WEST 

I think constants only classes are not good OO design.

Answer 2

it came up that the constants are too scattered and should all be organized into a single "master" constants file.

This is the 100% wrong solution to the problem of constants being "hard to find". It's a fundamental error (unfortunately all too commonly perpetrated by programmers) to group things by technical criteria like being constants, enums or interfaces.

Except for layer architectures, things should be grouped by their domain, and constants all the more so since they are very low-level elements. Constants should be part of the classes or interfaces that use them as part of their API. If you can't find a natural place for them to live, you need to clean up your API design.

Additionally, a single huge constants file kills development speed in Java because constants are inlined into the classes that use them at compile time, so any change forces a recompilation of all those files and all that depend on them. If you have one file with constants that are used everywhere, you largely lose the benefit of incremental compilation: watch Eclipse lock up for 15 minutes as it recompiles your entire project for every change to that file. And since that file contains all constants used anywhere, you'll be changing it quite often. Yes, I am speaking from personal experience.

Answer 3

You are right. Your suggestion allows a programmer to import static Constants.BankAccount.* and eliminate countless repetitions of 'BANKACCOUNT_'. Concatenation is a poor substitute for actual scoping. That said, I don't hold out a lot of hope for you to change the team culture. They have a notion of proper practices, and aren't likely to change even if you show them 100 experts that say they are wrong.

I'm also wondering why you have a single 'Constants' file at all. This is very bad practice, unless these constants are all related somehow and are very stable. More typically it becomes a kind of kitchen-sink class that changes constantly and is depended on by everything.

Answer 4

Both approaches work, and that's what's important at the end of the day.

That said, your approach is common enough to be considered a pattern, or more accurately a good enough improvement over the constant interface anti-pattern. I don't see what's complicated about it, but that's a discussion you should have with your team.

Answer 5

One of the challenges with the long list of constants is finding the "correct" constant to be using. Invariably, someone tacks a constant in at the end of the line instead of adding it to the group it belonged to.

Which brings up another point to discuss with your team - there already is a de facto categorization of the constants through their names. So it really shouldn't be a big shift for them from current state to what you're proposing. Using auto-complete makes constant look-ups even easier.

If anything, your suggestion helps discourage using the "wrong" constant even though it might fit in a particular situation. By wrapping the constants within a representative class, it will encourage the developers to think through if they should be using it. For example, if I pull CreditCard.SomeConstant for general usage, I really should wonder why there isn't a General.SomeConstant for that situation instead.

I've been on projects with monster amounts of constants, and I would have preferred to have the method you're suggesting. It's cleaner, more direct, and it helps avoid inadvertent usage.

Answer 6

I do this occasionally (in C#), particularly if I need to program against / parse a well-known and fixed XML structure or something similarly nested and string heavy...for instance a custom config block parser.

I build out a nested class structure matching the hierarchical structure of the XML w/ the class names corresponding to the elements containing a const string "ElementName" property and a similar property corresponding to each Attribute (if any).

It makes it very easy to program against the corresponding Xml.