[stellation-res] To what extent should literals be named?

[Dave Shields <shields@xxxxxxxxxxxxxx>]

In his workspace code audit, Jonathan Gossage wrote:

  The second point concerns the use of literal constants. While these
  are very commononly used they represent bugs-in-waiting, especially
  with open source projects. There are two major problems with them:
 
  a) They become a maintenance nightmare whenever it is necessary to
  change the value of a literal. This is made worse where the same
  literal may be used for several purposes. Does the value 0 here mean
  the same as the value 0 there for example.

  b) It is not possible to use browsers to find where those literals
  that have meaning in a specific context are used.

  <start rant>
  My own policy has been to never use literal values for anything except
  things like strings in debug and log messages. Everything else is made
  a static constant in the class where the literals are relevant, or
  occasionally in classes of their own.
  <end of rant/>

Many of his suggested changes are to follow this policy, assigning
names to many of the string literals currently used in the code.

Current policy is to name literals rarely, and to hopefully write code
so that the context in which a literal is used indicates what the
literal is naming. An example of existing usage can be found in
repos/artifact/Artifact.java

    /**
     * Names the artifact types.
     */
    public interface Type {
        /**
         * Note that the type names used here must be the same that are
         * used to enter artifact types in the repository. In particular
         * they must be in the same case.
         */

        /**
         * A compound artifact has type "Compound".
         */
        public static final String COMPOUND = "Compound";

This is done since the name of a Compound artifact is used throughout
the repository and workspace code, and in accessing the database.

Jonathan favors a view closer to the other end of the spectrum -- to
name every literal except those used in debug and error messages.

Here are some examples taken from current code to indicate what effect
the proposed change would have. The question is how much would these
changes improve readability?

From the main loop of Svc:

        else if (command.equals("clean")) {
            new Change(context).command();
        }
        else if (command.equals("checkout")) {
            new Checkout(context).command();
        }

would become

        public Interface Command {
            public static final String CLEAN = "clean";
                ...
        }
        ...
        else if (command.equals("SVC.Command.CLEAN)) {
            new Change(context).command();
        }
        else if (command.equals(SVC.Command.CHECKOUT)) {
            new Checkout(context).command();
        }

and related code in Change

        else if(commandName.equals("mv"))
            mv();
        else if (commandName.equals("rm"))
            rm();

would become

        else if(commandName.equals(SVC.Command.MOVE))
            mv();
        else if (commandName.equals(SVC.Command.REMOVE))
            rm();

I find that in the existing code it is clear that "clean" and
"checkout" are command names.

From LogEntry, current

    public String getAction() {
        return getAttribute("action");
    }

would become

    public String getAction() {
        return getAttribute(LogEntry.Item.ACTION);
    }

Here the use of "action" is actually an implementation detail, as the
caller of the method getAction() doesn't know that the string "action"
has a special meaning -- it's part of the implementation.

From Checkout:

        boolean force = getOptions().get("force") != null;

would become

        boolean force = getOptions().get(SVC.Options.FORCE) != null;

Here the use of 'getOptions()' indicates that "force" is an option
name, so is a name really needed.

From Test:

      if (getOptions().get("verbose","").equals("true")) {

would become

      if(getOptions().get(SVC.Options.VERBOSE,"").equals(SVC.Options.TRUE_VALUE)) {

I find it clear from context that "verbose" is an option value, and
"true" is a possible option value.

From Fork:       

            context.getWorkspace().setState("0", projectSignature);

would become

            context.getWorkspace().setState(Workspace.STATE_ZERO,projectSignature);

The Javadoc for setState indicates the first parameter is a workspace
state:

    /**
     * Saves the signature associated with a state.
     *
     * @param state the state to which the signature is to be assigned.
     * @param signature signature for the specified state
     */
    public void setState(String state, String signature) {

so it's clear that "0" in this context represents a workspace state.

Re Jonathan's point

  b) It is not possible to use browsers to find where those literals
  that have meaning in a specific context are used.

I tend to use 'fgrep' for this task in Unix. For example here is
result of fgrep '"checkout"' *.java in Workspace:

  Checkout.java:        getWorkspace().setAttribute("checkout", checkoutTime);
  Checkout.java:                      .add("action","checkout")
  List.java:                String value =  getWorkspace().getAttribute("checkout");
  LogManager.java:        if (action.equals("checkout") || action.equals("checkin")) {
  LogManager.java:            if (command.equals("co")) command = "checkout";
  LogManager.java:            else if (selection.equals("co")) selection = "checkout";
  Svc.java:        else if (command.equals("checkout")) {
  Svc.java:        .add("checkout")
  Svc.java:            if (commandName.equals("co")) commandName = "checkout"; // 'co' and 'configure' clash
  Svc.java:            if (command.equals("co"))  command = "checkout";

I find no trouble from this seeing what each instance of "checkout"
refers to, but I'm probably biased since I wrote the code.

My own view is to avoid naming literals except where there is good
reason to do so, but I would like to hear what others have to say
about this.

dave

-- 
Dave Shields, IBM Research, shields@xxxxxxxxxxxxxx.