Choice of Smell

I have a problem that puts me into the position to choose between to stinky options I don’t like and I don’t see a third.

I tend to divide up my project into packages. Most always I have some package called data. In that package I usually put the classes that contain the data my software works with. For example the data a customer enters or the stuff that is stored in the database or the stuff I retrieve from the database. With that type of structure I ran into a design issue today.

CodeSmell #1: Cyclic Dependencies

I try to not get a lot of cyclic dependencies between packages. I consider that code smell (as do others but don’t press me for links right now I want to go home!). For example the data classes should not have dependencies to other packages (unless we’re talking about subpackages), because nearly every other package depends on the data classes and cycles cannot be avoided if the data classes have outside dependencies. This kind of approach might not be the brightest idea, it’s just the way I have done it up to now.

CodeSmell #2: Switch Statements

Another code smell I try to avoid like the plague are switch statements. Often enough though, I have to take some data specific Action. It is tempting to use a Factory to produce that Action and switch over some kind of identifier (enums, classes, ints and more) to get the specific Action needed.

import project.data.Protocol;
class ActionFactory{
  public Action getAction(Protocol protocol) {
        switch(protocol) {
            TCP:  new TcpAction();
            ICMP: new IcmpAction();
            default: throw new AssertionError("Stupid Switch!");
        }
    }
}

One could also add the Action directly to the data class. I opted for a FactoryMethod variant (hopefully at least resembling the original pattern?)

import project.rmi.actions.*;
public enum Protocol {
    TCP {  public Action getAction() {  return new TcpAction();   }},
    ICMP{  public Action getAction() { return new IcmpAction();  }};
    public abstract Action getAction();
}

But it’s not so simple – even though the enum does look simple (IMHO).

Problem

Now I have the FactoryMethod added to my data classes or in this case to the enum elements. It still looks very much like the Factory class except that the switch statement is missing and Eclipse does not yet support abstract methods in Enums, arghl! But the Enum is part of the data package while the action being returned by the FactoryMethod is part of a completely different package: even worse it is part of a package of classes that will communicate via rmi with the rest of my application. So now I have a ‘remote’ dependency in my data classes.

The enum is inherently part of the data while the action is definitely not. So what do I do? Go back to the switch statement? Or is there a better solution I just don’t see right now? Is the complete design with the data package rubbish?

PS: and no I don’t consider a mess of if-else statements an alternative solution. It is the equivalent of a switch as far as error proneness goes.

11 Replies to “Choice of Smell”

  1. Two short comments before I leave:

    – I don’t consider the switch statement to be a code smell

    – If I got it right, you are trying to seperate data from functionality. In my opinion, this is a good approach. But be aware, that this clashes with the pure doctrine of object orientation, where the data and its operations are bundled together

    – I don’t like the idea that an enum has operation. Enums are in my opinion sub-ranges of basic types, e.g. integers. Therfore, they should not have operations. But this aversion may come from the fact that I am a C++ programmer.

    So yes, I would go back to the switch statement.

  2. Ok so the switch is kind of a favorite among C++ programmers it seems 🙂
    I’ll probably go back there. I had the discussion with my boss before where I asserted that it wasn’t possible to get rid of the switch. When I noticed I could get rid of it I tried. With a not so pretty solution.

    About the enums with/out operations well I think enums in java are something completely different than enums in C/++. That’s a topic for another post (or mor time on my hands). I really like the way you can attach behaviour to constants or variables with anonymous classes or stick methods on enums. I think enums as they are now developed out of the “java typesafe enumeration pattern” (as mentionned in Effective Java for example).

    As for the object orientation paradigm that will keep objects with their operations, I guess I am often not quite adhering to that codex. Anyway in this case though that is not even the problem because the dependency is across system/vm boundaries. Of course I can deploy both classes in both systems/vms but it’s still not a pretty sight.

  3. I have to admit that method 2 is, well, really slick. It does cool things with enums that are really anonymous inner classes and whatnot. It looks cool. On the other hand, it took me a minute to really get it, and I also looked up a spec. The switch was intutively clear in about 3 seconds, and readability is a prime concern when writing code.

    The enum debate, since in this instance it is largely a matter of taste. In Java, enums are always objects, and though this may not neccessarily be a good thing, it is a fact of life. So you may as well use the functionality that is offered.

    Still, it seems that you are sacrificing some of the flexibility of a really factory method with your scheme. You cannot simply call a method with a simple parameter that creates your object, you first have to create an object (the enum) that does it for you. Things like object pooling or flexible creation rules may be possible; I guess it’d soon get very messy, though.

    Most importantly, I don’t see any reason *not* to use the switch statement. I don’t consider it a bad code smell. And even if you did (because it can often be replaced by polymorphism and so on…), a factory method would be a perfectly good application of it. The worst thing you can say about it is that you may to break – but hey, we’ll get over it. At least it’s clean, efficient and readable.

    (Note from my newfound C# skills: C# does not allow fallthrough switch statements and treats missing breaks as an error. ..)

  4. Pingback: /dev/blog
  5. Pingback: daily de(v)lusions
  6. Pingback: daily delusions
  7. Would this even work? I’m new to enums and all Tiger new features, and I don’t have a java 5 compiler at hand to test if this even compiles but if it did work it would be a less ugly version (although I do think the switch wasn’t that bad). So here it goes:

    import project.rmi.actions.*;
    public enum Protocol
    {
    TCP (TcpAction.class),
    ICMP(IcmpAction.class);

    private Class actionClass;
    Protocol(Class<T extends Action> action)
    {
    actionClass = action;
    }
    public Action getAction()
    {
    // Needs to handle two exceptions
    return actionClass.newInstance();
    }
    }

    I’m assuming that your classes have zero-arg constructors and that you don’t mind handling the exceptions involved in the newInstance() method. I think this solution has its own ugliness but so do the other two. Please critizise with no mercy 🙂 I need to learn!

  8. I corrected the html 🙂

    And I checked: with Eclipse is does not compile. It doesn’t like the “T extends Action” part but my eclipse version does not support all 5.0 features yet. I don’t have time to look up the specs on that right now. But I can add one more fact: my constructors do have one parameter. Well some even have two params. So it cannot quite work that way.

    More on that later. I am no fan of reflection. It is going the complicated, error-prone way when the alternative isn’t that bad, as I posted here I went back to the switch.

  9. Thanks for the quick corrections! Not blogged that much yet 😉
    I’ll try to think over the subject and lok at the specs to see if it should compile. Thanks for your attention as well, cheers

  10. Pingback: daily delusions

Comments are closed.