A lot of things can be found in the snippet at “What is wrong with this code #02”. Here it is again:
public MyObject {
private String myField;
... (imagine multiple fields) ...
public GenericObject toGenericObject() {
GenericObjectFactory genericObjectFactory = GenericObjectFactory.getInstance();
GenericObject genericObject = genericObjectFactory.create("myObject");
genericObject.setField("myField", myField);
// imagine multiple lines using genericObject.setField("field", field)
return genericObject;
}
}
Already many things are pointed out in the comments of the initial post.
One thing that immediately struck me is this line:
GenericObjectFactory genericObjectFactory = GenericObjectFactory.getInstance();
Don’t do this.
GenericObjectFactory implies with the getInstance to be a Singleton. Even though it is a singleton, never retrieve and instance inside your method like that.
Even though there “should be only one instance” of the GenericObjectFactory, it still is a dependency for serializing MyObject. Never hide your dependencies. It makes your code hide its intent, it’s hard to decouple, test, refactor, in short hard to maintain. The code should clearly communicate its intent and usage(s).
In my opinion, there are three (well four sort-off) options to deal with this:
1. Deliver GenericObjectFactory as a parameter (invasive in method signature, but automated refactoring tools (i.e. IntelliJ) handle this perfectly):
public GenericObject toGenericObject(GenericObjectFactory genericObjectFactory ) {
GenericObject genericObject = genericObjectFactory.create("myObject");
2. Inject GenericObjectFactory as a property of MyObject (invasive in constructor)
public MyObject {
private final GenericObjectFactory genericObjectFactory;
public MyObject(final GenericObjectFactory genericObjectFactory) {
this.genericObjectFactory = genericObjectFactory;
}
private String myField;
... (imagine multiple fields) ...
public GenericObject toGenericObject() {
GenericObject genericObject = genericObjectFactory.create("myObject");
2B. If you find option two too invasive, add the default constructor again but let it call the new constructor (makes point 2 less invasive, remove this default constructor after period of time).
public MyObject {
private final GenericObjectFactory genericObjectFactory;
public MyObject(final GenericObjectFactory genericObjectFactory) {
this.genericObjectFactory = genericObjectFactory;
}
// add this default constructor
public MyObject() {
this(GenericObjectFactory.getInstance());
}
private String myField;
... (imagine multiple fields) ...
public GenericObject toGenericObject() {
GenericObject genericObject = genericObjectFactory.create("myObject");
3. Introduce protected getInstance method in MyObject (least invasive, increases testability)
public MyObject {
... snip ...
protected GenericObjectFactory getInstance() {
return GenericObjectFactory.getInstance();
}
public GenericObject toGenericObject() {
GenericObjectFactory genericObjectFactory = getInstance();
GenericObject genericObject = genericObjectFactory.create("myObject");
...
}
This is sorted by my personal preference. Although option one and option two trade places from time to time. The third option is less favorable, as you still do not expose the need for dependencies, but it does make it more testable. It is also the least invasive way to refactor, which is sometimes beneficial in legacy projects with lots of untested code.
To conclude, never hide your dependencies, if you ever do see some class using getInstance, please, refactor it. It will make your, and your fellow-programmers, life a lot easier.
Thank you!