What is wrong with this code #02

Given that the functionality of the method toGenericObject must be preserved; there is something obviously wrong in this code, can you find it?

If so, can you think of an easy solution?

[sourcecode language=”java”]

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;
}

}
[/sourcecode]

11 Comments

  1. GenericObjectFactory genericObjectFactory = GenericObjectFactory.getInstance()

    AAAAHHH!!

    When are you gonna switch to Ruby already? πŸ˜€

    • Do you really think i approve this code? πŸ˜‰ and since when has a language has anything to do with dependency management? πŸ™‚

      I would love to do ruby some time.

  2. I just find it too verbose. Don’t try Ruby, you’ll never go back πŸ˜€

    • Who knows one day I might try it. πŸ™‚ I’ll let you know.

      • Btw, if you thought ” too verbose” was the answer of thus post, then please look again. πŸ™‚

  3. Doing this setField(‘myField’, myField) for all class variables results in that genericObject’s class variables refer to the same objects as myObject, myField.clone() should be user to construct the New genericObject.

    • On the other hand, if you do not want to clone the myObject to a generic object, but rather treat the myObject as a generic object, first thing that comes to mind is that GenericObject should be the parentclass of MyObject.

      • Both good points Bart. Funny how many things can be found in such a small piece of code.

  4. The whole idea of creating an object with “generic” fields is bothering me but the first thing I would do is extract the following code in a seperate methode so you can mock it when testing:

    GenericObjectFactory genericObjectFactory = GenericObjectFactory.getInstance();

    to something like this:

    public GenericObjectFactory getGenericObjectFactory() {
    return GenericObjectFactory.getInstance();
    }

    The other thing I would do is to make the following code explicit:

    GenericObject genericObject = genericObjectFactory.create(“myObject”);
    genericObject.setField(“myField”, myField);

    It would like something like this for the readability:

    GenericObject genericObject – genericObjectFactory.createMyObjectWithMyField(myField);

    • The getInstance of the GenericObjectFactory is a pain yes. Depending on how invasive you want/can be in the method signature there would be more options then extracting to a getGenericObjectFactory method.

      Also, there are more fields on this object. So making explicit is a good way to go I think, but “withMyField” would not suffice as there are other fields as well.

      Nice input Arjan πŸ™‚

Leave a Reply

Your email address will not be published. Required fields are marked *