views:

518

answers:

8

We are in the process of refactoring some code. There is a feature that we have developed in one project that we would like to now use in other projects. We are extracting the foundation of this feature and making it a full-fledged project which can then be imported by its current project and others. This effort has been relatively straight-forward but we have one headache.

When the framework in question was originally developed, we chose to keep a variety of constant values defined as static fields in a single class. Over time this list of static members grew. The class is used in very many places in our code. In our current refactoring, we will be elevating some of the members of this class to our new framework, but leaving others in place. Our headache is in extracting the foundation members of this class to be used in our new project, and more specifically, how we should address those extracted members in our existing code.

We know that we can have our existing Constants class subclass this new project's Constants class and it would inherit all of the parent's static members. This would allow us to effect the change without touching the code that uses these members to change the class name on the static reference. However, the tight coupling inherent in this choice doesn't feel right.

before:

public class ConstantsA {
  public static final String CONSTANT1 = "constant.1";
  public static final String CONSTANT2 = "constant.2";
  public static final String CONSTANT3 = "constant.3";
}

after:

public class ConstantsA  extends ConstantsB { 
  public static final String CONSTANT1 = "constant.1";
}

public class ConstantsB {
  public static final String CONSTANT2 = "constant.2";
  public static final String CONSTANT3 = "constant.3";
}

In our existing code branch, all of the above would be accessible in this manner:

ConstantsA.CONSTANT2

I would like to solicit arguments about whether this is 'acceptable' and/or what the best practices are.

A: 

I've seen this done by putting the static final String on an interface, so that you can 'implement' it and not have to worry about what to do when you need a different base class. It's just as accessible that way.

In general though, enums are pretty good at what you are trying to do, and may get rid of the "I'm not sure" feeling you are experiencing, as that's the intention of enums.

joeslice
when you implement an interface, you make all of that interface's members a part of your class' permanent API -- take a look at the Swing classes to see just why this is a bad idea
kdgregory
@kdgregory - You don't have to implement it. Just use it in the same way as the class of constants since they are all static values.
Robin
That's correct. However, I was responding to the poster, who explicitly said "implement".
kdgregory
+2  A: 

I don't like it, but it's probably the best you can do right now.

The right answer would be to break up the constants into coherent groups, fixing the code breaks as you go along. In C#, I'd use enums.

Steven Sudit
A: 

I think what you are doing is fine. Yes, the classes are tightly-coupled, but that is kind of the point -- you want to be able to reference only a single class to see all of your project-wide constants.

You do have to be diligent to ensure that ConstantsB contains only constants that are generalizable amongst all your projects, and ConstantsA contains only project-specific constants. If, later on, you realize that there is a constant in ConstantsB that you seem to be overriding in your subclasses a lot, then that's an indication it should've never been put in ConstantsB in the first place.

alanlcode
A: 

I think what you've got is a good first step. The next step is to gradually replace all references to ConstantsA.CONSTANT2 and ConstantsA.CONSTANT3 with ConstantsB.CONSTANT2 and ConstantsB.CONSTANT3 until you can remove the extends.

Most IDEs can be configured to show a warning if you refer to a superclass constant via a subclass, and I'd guess static analysis tools like FindBugs can do it, too.

One idea that might be slightly cleaner:

  1. make all the constants classes interfaces
  2. move all the constants out of ConstantsA and call it something like LegacyConstants
  3. have LegacyConstants extend all the other, modular Constants interfaces
  4. deprecate LegacyConstants

The goal would be not to have any inheritance between the Constants interfaces. LegacyConstants would be the only place there's any inheritance, it wouldn't declare any constants of its own, and when it's no longer used -- when every class that did use it instead refers to the proper Constants interface -- you've finished refactoring.

David Moles
A: 

When you extract your constants, have the old class reference the constant defined in the new class. There's really no need to create an inheritance relationship here.

kdgregory
+7  A: 
  • A class with only static fields is a code smell. It's not a class.

  • Some people use interfaces, so they can implement it to use the constants more easily. But an interface should be used only to model a behaviour of a class. (http://pmd.sourceforge.net/rules/design.html#AvoidConstantsInterface) Using static imports from Java 5 removes the need for simple constant usage at all.

  • Are your constants really Strings, or just used as Strings. If they are different options for some type (so called enumerations), you should used typesafe enumerations, using enum in Java 5 or the Enum provided by Commons Lang. Of course, converting your code to use enums might be a little work.

  • You should at least split the constants to groups of related constants in files with proper business name. Moving the final members is easy in IDE and will update all usages.

  • If you can afford it, convert them to enums then. (Think about using about a script to do that, often it's possible.) Class hierarchies are only usefull, if there is a relation between the constants/enums. You can keep the Strings if you have to but still think about them as entities, then extends might make sense for some (describing is-a relation). First enums can be simple classes made by yourself if serializing is not a problem. Enums are always favourable due to their type safe nature and the extra name showing intend or business/domain specific things.

  • If the constants are really String constants use a Properies or ResourceBundle, which can be configured by plain text files. Again you can script the refactoring using the constant names as resource bundle keys and generate both files automatically.

Peter Kofler
Re your last bullet: one would still have strings all over the place. I don't see why properties.get("SYMBOL")is better than Constants.SYMBOL
Wouter Lievens
you are right, but it depends on the usage of the constant I would say. If it's a text for display, it belongs in a bundle. And then we are good developers and but the bundle names into constant classes - hurra recursion
Peter Kofler
thanks for your response. I agree with the interface comment (I didnt even consider it). The question you are asking is: what are these constants used for? We have a variety of uses - some are String values for display (ex: table column headers), some are constant values in primitives, still others are constant keys for use in Maps.
akf
of course it's not easy to say what to use when. But I think the technical separation is a good start. So display texts to their own file (or to bundles, but then the bundle keys have to go somewhere as Wouter Lievens pointed out. Keys in maps could be enums, if all keys are numerated. But propably it's special keys for some meaning, so put them together etc. As usual the problem is more in the domain than in the code (what belongs where)...
Peter Kofler
+1  A: 

I could be wrong, but I don't think we need constants at all. It just means that you can't change the value of the constants and you probably should.

Berlin Brown
+1 too true. I'm currently implementing a capabilities model for some software I'm writing, and instead of hardcoding MaxSize and MaxCount, I'm parameterizing them through interfaces. Now my limit is going to be whatever integer datatype I choose.
Chris Kaminski
A: 

Peter Kofler has already discussed how you might wish to better organize constants. I'll share how to automate the transition:

The eclipse "Inline" refactoring can automatically replace constants by their defintion, saving you from having to hunt down and change each usage manually. So you'd simply change the code to:

public class ConstantsA  { 
  public static final String CONSTANT1 = "constant.1";
  public static final String CONSTANT2 = ConstantsB.CONSTANTFOO;
  public static final String CONSTANT3 = ConstantsB.CONSTANTBAR;
}

public class ConstantsB {
  public static final String CONSTANTFOO = "constant.2";
  public static final String CONSTANTBAR = "constant.3";
}

... and then have eclipse inline COONSTANT2 and CONSTANT3 (while all affected projects are checked out, if you can't do that, look into refactoring scripts), and you're done.

meriton