2

I inherited some code that I have spent some time reviewing to get a better handle on its design.

There is one class that I came across that I have an idea for refactoring, but I am wondering if it I could get an opinion on whether or not it would make the design clearer.

The class basically contains 4 properties:

public class MyObj {
   private String foo;
   private String bar;
   private TypeOneObj baz;
   private TypeTwoObj fuz;

   public MyObj(String foo, String bar, TypeOneObj baz){
      this.foo = foo;
      this.bar = bar;
      this.baz = baz;
   }

   public MyObj(String foo, String bar, TypeTwoObj fuz){
      this.foo = foo;
      this.bar = bar;
      this.fuz = fuz;
   }    

}

The MyObj class has constructors for creating instances with either a TypeOneObj or a TypeTwoObj, but not both.

Each constructor includes a foo and a bar.

With the way that the class is constructed now, it gives the impression that there is a scenario underwhich you could have both baz and fuz data present, but that isn't the case.

My idea for refactoring the class would be to make MyObj abstract, and then creates sub types, something like:

public abstract class MyObj {
   private String foo;
   private String bar;

   public MyObj(String foo, String bar){
      this.foo = foo;
      this.bar = bar;
   }
}


public class TypeOneBackedMyObj extends MyObj{
   private TypeOneObj baz;

   public TypeOneBackedMyObj(String foo, String bar, TypeOneObj baz){
      super(foo, bar);
      this.baz = baz;
   }
   ...
   ...
}

public class TypeTwoBackedMyObj extends MyObj{
   private TypeTwoObj fuz;

   public TypeOneBackedMyObj(String foo, String bar, TypeTwoObj fuz){
      super(foo, bar);
      this.fuz = fuz;
   }
   ...
   ...
}

I would also add some abstract methods to the MyObj class to delegate to the 2 implementations for whatever public operations are needed that involve the baz/fuz

From a high level, would this type of refactoring be beneficial?

2
  • 13
    What's notably missing from this explanation is how you intend to use this object. You can't judge an implementation if you don't know what it's supposed to do.
    – Flater
    Dec 4, 2023 at 5:06
  • Whatever "improvement" you decide upon, make baz and fuz final, so that it is clear that one can't add or change one later.
    – user949300
    Dec 8, 2023 at 18:01

5 Answers 5

4

This...

My idea for refactoring the class would be to make MyObj abstract, and then creates sub types

...or generic solution...

public class MyObj<T> {
   private T type;
   private String foo;
   private String bar;

   public MyObj(T type, String foo, String bar){
      this.type = type;
      this.foo = foo;
      this.bar = bar;
   }
}

...or without polymorphism, that shouldn't be used for inheritance, have TypeOneBackedMyObj and TypeTwoBackedMyObj with a field, or a property, of type MyObj...

public class MyObj {
   private String foo;
   private String bar;

   public MyObj(String foo, String bar) {
      this.foo = foo;
      this.bar = bar;
   }
}

public class TypeOneBackedMyObj {
   
   private MyObj myObj;

   public TypeOneBackedMyObj(myObj) {
      this.myObj = myObj;
   }
   ...
   ...
}

public class TypeTwoBackedMyObj {

   private MyObj myObj;

   public TypeOneBackedMyObj(MyObj myObj) {
      this.myObj = myObj;
   }
   ...
   ...
}

...or with polymophism the TypeOneBackedMyObj and TypeTwoBackedMyObj having a field, or a property, of type MyObj implement an interface.

The details strain the solution space.

4

There are a number of ways you can go here and as flater notes, you haven't explained how this object should be used, but here's an additional option. I surmise that there are a lot of if checks throughout the code to see whether TypeOneObj or TypeTwoObj is in play. If so, this might be an approach to consider.

First and most simply, if you can modify the code for TypeOneObj and TypeTwoObj, consider creating an interface that they both implement that contains the functions that are needed relative to MyObj to call. These functions can be aliases for existing functions on those types.

If you cannot modify these classes, you can use this same approach with composition. For example:

interface TypeObj {
    String getSna();
    void doMuv();

    static TypeObj from(TypeOneObj one) {
        return new TypeObj() {
            String getSna() {return one.snaas()}
            void doMuv() {one.muuvie()};
        };
    }

    static TypeObj from(TypeTwoObj two) {
        return new TypeObj() {
            String getSna() {return two.truux()}
            void doMuv() {/* nothing here */};
        };
    }
}

public class MyObj {
   private String foo;
   private String bar;
   private TypeObj typ;

   public MyObj(String foo, String bar, TypeOneObj baz){
      this.foo = foo;
      this.bar = bar;
      this.typ = TypeObj.from(baz);
   }

   public MyObj(String foo, String bar, TypeTwoObj fuz){
      this.foo = foo;
      this.bar = bar;
      this.typ = TypeObj.from(fuz);
   }    
}
3

Objects should not be constructed around data, but around behaviour.

New objects should only be created if it represents a different behavior. Note that subclassing is expensive in terms of complexity and cognitive load, and should be avoided if at all possible. Having some common data is definitely not a reason to do it.

So the question is: Does your object behave in a different way depending on which constructor is invoked, do these two modes have something in common, is there a general behaviour that could be extracted? Etc.

2

You wrote

The MyObj class has constructors for creating instances with either a TypeOneObj or a TypeTwoObj, but not both.

...

With the way that the class is constructed now, it gives the impression that there is a scenario under which you could have both baz and fuz data present, but that isn't the case.

I think this is missing the point, these two statements are actually contradictional.

Your current design does exactly the opposite: a user of the class who looks at the available constructors sees clearly that only a TypeOneObj or a TypeTwoObj can be passed, but not both. So users of the class have no reason to assume both types of objects are present at the same time.

If you have maintainers of the class in mind: those can be easily adressed by a comment in the code that only baz or fuz will be initialized. Unit tests which test each new functionality with objects of both kinds will make sure that comment won't get overlooked, without complicating things by extra subclasses.

But the real issue is a different one: once an object x of type MyObj is constructed and passed around, it may not be obvious whether x was initialized with Type1Obj or Type2Obj. This may or may not be a problem:

  • when MyObj only has methods which work for both types, and hence abstract the internal state category (Type1Obj or Type2Obj) away from the user, then there is no need to distinguish between MyObj objects of both types from the outside.

  • when MyObj offers certain public methods which work only when initialized with a Type1Obj, and others which work only when initialized with a Type2Obj, then your proposed design with two derivations start to make sense. Otherwise, it would be overdesign.

Sometimes it is sufficient to implement a boolean property in MyObj which allows a user to check the type of former initialization at run time. That's particular an option when you need this check only once or twice in the code.

Since you left out any information on how MyObj will be used in your code and most of any other useful context, I can only guess around what kind of design will be most suitable for your case, so decide for yourself.

3
  • I don't disagree enough to downvote, and you make valid points. However, I'd argue that my solution eliminates the need for comments (which often are overlooked) and unit tests (which take effort and sometimes fall behind)
    – user949300
    Dec 7, 2023 at 20:16
  • @user949300: think of your and the OPs solution both compiled to binaries, where only the signature is visible to the user of a class, but not the implementation. In the OPs design, it will be clear that only one kind of initialization is possible, at compile time - no comment needed. For your solution, the user of a class needs a clear comment in the API documentation that they are only allowed to pass one of both objects. So I would say your solution introduces the need for a comment, where the OPs solution does not have this need.
    – Doc Brown
    Dec 7, 2023 at 21:22
  • Given that OP presented source code, not binary, and is confused by the subtleties of the existing constructors, let's agree to disagree on this one. :-)
    – user949300
    Dec 8, 2023 at 18:03
0

@user437602 presents an elegant, but possibly overkill, solution.

Here's another: A clear, explicit constructor

public MyObj(String foo, String bar, TypeOneObj type1, TypeTwoObj type2){
  
  if ((type1 != null) && (type2 != null))
    throw new IllegalArgumentException("Cannot have both a TypeOneObj  and a TypeTwoObj");

  // question: should you check if both type1 and type2 are null?

  this.foo = foo;
  this.bar = bar;
  this.type1= type1;
  this.type2 = type2;
}

This makes the workings very clear to another programmer.

3
  • 2
    I don't think this is an improvement. Introducing a constructor which allows to pass both types of objects simultanously (and throwing an exception when used wrongly at run time) is definitely more error prone than the current design of MyObj where the two constructors already prevent wrong initialization at compile time. Maybe there is some place in the OPs code where one could wrongly assume both members being initialized, and there testing the values and throwing an exception is fine, but intentionally introducing such a constructor is IMHO not a good idea.
    – Doc Brown
    Dec 6, 2023 at 16:07
  • @DocBrown Thanks for explaining your downvote - I get your points and somewhat agree. However, see my comment on your answer.
    – user949300
    Dec 7, 2023 at 20:16
  • Would work fine as a private constructor
    – Basilevs
    Dec 7, 2023 at 23:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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