2

I'm working with a model that currently looks like this:

class Sign {
   enum Type { A, B }

   Color color;
   String textLine1;
   String textLine2;
   Type type;

   Sign(Type type, Color color, String textLine1, String textLine2) { ... }
}

A sign of type A has a color and 2 lines of text. A sign of type B has a color and only 1 line of text. Therefore they're instantiated like so:

Sign signA = new Sign(A, someColor, "line1", "line2");
Sign signB = new Sign(B, someColor, "line1", null);

When working with these instances, code checks for the type property and reads the appropriate fields.

I think this approach may be flawed as a B sign should not expose properties it doesn't hold (line2). It's also unsightly to be passing null to a bunch of properties it doesn't care about at instantiation time.

I'm wondering if it would be appropriate to use inheritance instead, where a base class would contain the shared fields and the type.

abstract class Sign {
    enum Type { A, B }

    Color color;
    String textLine1;
    Type type;

    Sign(Type type, Color color, String textLine1) { ... }
} 

class SignA extends Sign {
    String textLine2;      

    SignA(Color color, String textLine1, String textLine2) {
        super(A, color, textLine1);
        this.textLine2 = textLine2;
    }
} 

class SignB extends Sign { 
    SignB(Color color, String textLine1) {
        super(B, color, textLine1);
    }
}

Is the above a recommended pattern for modeling models of this type? Note that these models have no behavior and are purely bags of properties.

  • 1
    What are you doing with that enum? That looks like a code smell that you will be doing a bunch of if statements with casting. – Sign Aug 15 '16 at 19:01
  • Today, the enum value is used to know if we should expect a textLine2. So yes, a bunch of if statements. I was thinking that in my proposed approach I'd check the type enum, cast Sign to one of the subtypes and work with that. I hadn't considered the instanceof usage. – siger Aug 15 '16 at 19:59
1

Inheritance is the right way to model this domain, however enumeration is not necessary here. You already have distinctive characteristic of each sign - the type, which can be checked via instanceof or via reflection.

Polymorphism example:

abstract class Sign { 
    abstract void printType();
}
class SignA extends Sign {
    void printType() {
        System.out.println("Sign A");
    }
}
class SignB extends Sign {
    void printType() {
        System.out.println("Sign B");
    }
}
...
Sign sign = ...
sign.printType();

instanceof example:

Sign s = ...
if (s instanceof SignA) { ... } 
else if (s instanceof SignB) { ... } 
else {
   // safety check for future code modifications, in which you add new type 
   // but forget to add a handler
   throw new UnsupportedOperationException("Sign type " 
       + s.getClass().getName() 
       + " not supported");
}

Reflection example (Java 8):

Map<Class<? extends Sign>, Consumer<Sign>> processors = ...;
...
processors.put(SignA.class, s -> System.out.println("Sign A"));
processors.put(SignB.class, s -> System.out.println("Sign B"));
...
Sign sign = ...;
Consumer<Sign> processor = processors.get(sign.getClass());
processor.accept(sign);
1

If these are only "bags of properties", I'd go with the simplest solution:

class SignB {
    Color color;
    String line1;

    SignB(Color color, String line1) { ... }
}

class SignA extends SignB {
    String line2;

    SignA(Color color, String line1, String line2) {
        super(color, line1);
        this.line2 = line2;
    }

A less restrictive approach (you can't enforce 1 or 2 lines) but more generic (you always iterate over the available lines):

class Sign {
    Color color;
    List<String> lines;

    Sign(Color color, String... lines) { ... }
}

Sign signA = new Sign(Color.RED, "hello");
Sign signB = new Sign(Color.BLUE, "hello", "world");

A third approach. However I never used such an approach and I don't know how viable it is:

class Sign {
    Color color;
    String line1;
    Optional<String> line2;

    Sign(Color color, String line1) {
        this(color, line1, null);
    }

    Sign(Color color, String line1, String line2) {
        this.color = color;
        this.line1 = line1;
        this.line2 = Optional.ofNullable(line2);
    }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.