[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
RE: [stellation-res] To what extent should literals be named?
|
> >-----Original Message-----
> >From: stellation-res-admin@xxxxxxxxxxxxxxx
> >[mailto:stellation-res-admin@xxxxxxxxxxxxxxx]On Behalf Of Dave Shields
> >Sent: September 3, 2002 11:20 AM
> >To: stellation-res@xxxxxxxxxxxxxxx
> >Subject: [stellation-res] To what extent should literals be named?
> >
> >
> >
> >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.TR
> >UE_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
To me the two biggest advantages to naming literals are the ease of changing
a literal value and the ability to find all uses of the concept behind a
literal using a browser. I have had to do a lot of code maintenance over the
years and evolved the practice I suggested to make my life, and that of
others, easier.
I agree that from a readibility point of view there is often little
difference between the two forms.
Regards
Jonathan Gossage
Personal Email
jgossage@xxxxxxxx
Business Email
jonathan@xxxxxxxxxxxxxx
