0

Suppose I have a class

public class MyMainClass {
   SomeGlobalStateObject someGlobalStateObjectInstance;

   public MyMainClass(SomrDep1 someDep1, SomeDep2 someDep2){
      this(someDep1); // call the other constructor so that I can initialize, the base layer is the same
      someGlobalStateObjectInstance.setGlobalVariable("someVar", someDep2); // I have overriden setGlobalVariable for "someVar", seems like a smell.
   }

   public MyMainClass(SomeDep1 someDep1){
      someGlobalStateObjectInstance = new someGlobalStateObjectInstance();
      someGlobalStateObjectInstance.setGlobalVariable("someVar", null); // The null in the second param is a key part of my problem. Assume this must be done, I cant remove this line for this constructor.
      // do lots of other things with someGlobalStateObjectInstance.
   }

}

The above is the current design for this class. You can assume that I must create someGlobalStateObjectInstance in the constructor and it cant be passed as a dependency itself.

Here is my main problem now. As you see, I first called someGlobalStateObjectInstance.setGlobalVariable("someVar", null); in the second constructor, and then in the first one I override again with someGlobalStateObjectInstance.setGlobalVariable("someVar", someDep2);.

Is this a code smell? It is unnecessarily setting a variable and then overriding in the first constructor. I am wondering how to best tackle this to avoid all code smells.

jjot0
  • 209
  • 2
  • 6
  • Why can't you delete `setGlobalVariable("someVar", null);`? – Sweeper Apr 14 '20 at 11:42
  • @Sweeper, it has to be there. – jjot0 Apr 14 '20 at 11:43
  • 1
    I'm asking _why_? Maybe you should redesign your `SomeGlobalStateObject` so that that line can be removed? If you can't change `SomeGlobalStateObject`, then it can't be helped, can it? – Sweeper Apr 14 '20 at 11:44
  • In your second constructor, what are you doing with `someDep1`? – Lino Apr 14 '20 at 11:44
  • @Sweeper, yeah the problem is the codebase I am working in, I can't redesign that component and its a blackbox. so that line just cant be moved. – jjot0 Apr 14 '20 at 11:49
  • I was considering making another `prrivate void` method with everything other than that `setGlobalVariable("someVar")` and then calling the private method from both consturctors and then setting `setGlobalVariable("someVar")` individuall. – jjot0 Apr 14 '20 at 11:50
  • 1
    Shouldn't `SomeGlobalStateObject` be injected via the constructor and shouldn't it be configured (setting "someVar") outside of the constructor? – Joakim Danielson Apr 14 '20 at 11:50
  • 2
    can't you reverse the call order of the constructors? i.e. could you write the second constructor as `public MyMainClass(SomeDep1 someDep1){ this(someDep1,null); }` and write all constructor code in the two args constructor? – Thomas Kläger Apr 14 '20 at 11:51
  • Having a nullable in the constructor doesn't seem ideal, which is why I think the recommendation is to overload instead isn't it? @ThomasKläger – jjot0 Apr 14 '20 at 11:52
  • @JoakimDanielson, yeah it should in the ideal world. the codebase I am wondering in though, thats not possible. I mentioned this above – jjot0 Apr 14 '20 at 11:53
  • @jjot0 If you don't want to allow null to be passed inside from the outside you could add a `private` third constructor to which the other two `public` ones delegate: [Demo](https://ideone.com/lDBxPw) – Lino Apr 14 '20 at 12:01
  • 1
    Well, [best practice to overload constructors](https://stackoverflow.com/questions/1182153/constructor-overloading-in-java-best-practice) suggests to follow the approach offered by @thomas-kläger, but if you have some backward compatibility reasons to set null to `someVar` initially, then it's unlikely to change anything about it. – Nowhere Man Apr 14 '20 at 12:05

1 Answers1

0

Change the way then,

public MyMainClass(SomrDep1 someDep1, SomeDep2 someDep2){
   //this(someDep1);
   someGlobalStateObjectInstance = new someGlobalStateObjectInstance();
   someGlobalStateObjectInstance.setGlobalVariable("someVar", someDep2);
}

public MyMainClass(SomeDep1 someDep1){
   this(someDep1, null);
}