| Re: [stellation-res] To what extent should literals be named? |
On Tue, Sep 03, 2002 at 11:19:49AM -0400, Dave Shields wrote:
>
> 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();
> }
Close, but still not perfect: think about I18N...
florin
>
> 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.
> _______________________________________________
> stellation-res mailing list
> stellation-res@xxxxxxxxxxxxxxx
> http://dev.eclipse.org/mailman/listinfo/stellation-res
>
--
"If it's not broken, let's fix it till it is."
41A9 2BDE 8E11 F1C5 87A6 03EE 34B3 E075 3B90 DFE4
Attachment:
pgp8q_Fuptt6A.pgp
Description: PGP signature