Keep abstractions at same level

During a code review I noticed that the developer had added an equals and hashcode method to an existing object.  We were not comparing these objects anywhere so we didn’t need the equals defined.

This caused me to dig a little further to understand why he added the equality code.  After a short conversation, it turned out that he added the equality methods in order to test code in another class.

Here is what his code was doing (methods changed to protect the innocent):

public class Stuff {
    private Jsonifier jsonifier;
    private Dao dao;
    public String getSomethingAndConvert() {
        List<Stuff> things = dao.getStuff();
        List<ConvertedStuff> convert = new ArrayList<ConvertedStuff>();
        for(Stuff thing: things) {
            converted.add(new ConvertedStuff(thing));
        }
        return jsonifier.jsonify(converted);
    }
}

When testing this method, since he was creating a bunch of new objects, he needed to have equals and hashcode defined on the ConvertedStuff object.  I suggested that he change it to this instead:

public class Stuff {
    private Jsonifier jsonifier;
    private Dao dao;
    private Converter converter;
    public String getSomething() {
        List<Stuff> things = dao.getStuff();
        List<ConvertedStuff> converted = converter.convert(things);
        return jsonifier.jsonify(converted);
    }
}

public class Converter {
    public List<ConvertedStuff> convert(List<Stuff> stuffToConvert) {
        List<ConvertedStuff> converted = new ArrayList();
        for(Stuff thing: stuffToConvert) {
            converted.add(new ConvertedStuff(thing));
        }
        return converted;
     }
}

Now the test for the method getSomething() in the Stuff class we can easily test without needing an equals and hashcode.  We can simply mock out the converter.convert method and see that the method is working correctly.

It seems we have just moved our problem to a different class.  Don’t we still need to have an equals and hashcode method on ConvertedStuff?  Well, no.

public void testConvert() {
    Stuff thing1 = mock(Stuff.class);
    thing1.setName("name");
    List<Stuff> stuffToConvert = Arrays.asList(thing1);

    List<ConvertedStuff> converted = converter.convert(stuffToConvert);

    assertEquals(1, converted.size());
    assertEquals("name", converted.get(0).getName());
}

We could have just written our test around the stuff.getSomethingAndConvert method like we see above.  That would have allowed us to get rid of the equals and hashcode. However, the issue I saw wasn’t really about the equality methods.  That is simply what tipped me off to a possible larger design issue.

We were doing too many things in the original method and we had multiple levels of abstraction in one method.  With the refactor, we are keeping the methods at the same level of abstraction.  This makes reading the code much easier.  We don’t have to look at the details of how we convert stuff to converted stuff, we just know that we do it.  If we need the details, we know exactly where to go.

 

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s