How thin is too thin?

I recently had a conversation with a coworker about where the logic belonged in a piece of code.  I felt that his dao* (data access object) layer had too much logic.  I suggested that the objects that were communicating with the database should only have the queries needed to get the data from the database, and any business logic should be in a different layer (likely a service layer). My colleague said that he had done that before and he thought that made the dao layer too thin.

I let the comment slide, and let the code slide (though I am regretting that decision).  But, that got me to wondering if there is such a thing as a layer being “too thin”.

tomatoes
This, by the way, is probably too thin.

The only example I can think of where I might say that a layer was too thin was if it were just a wrapper around another object and it wasn’t really serving any purpose, it just called through to the object it wraps.  However, if the wrapper was serving a purpose (like wrapping a utility object that was all static methods so we can mock it, or wrapping a third party library that we want to encapsulate to make it easy to change out), then I wouldn’t have an issue.

In my mind, I try to think of each layer as a different level of abstraction.  Our controllers have the highest level of abstraction.  Reading the code there helps you understand the high level tasks we need to do (like, get some data, set some values and return some data).

The next level down is the service layer.  This contains business logic  It would contain the code that sets values, or does some calculations that we need to return to the user through the controller.  This layer is more detailed, but will often use other classes to assist.

The last level is the dao layer.  This is where we are getting our data, either from a database or file, or magic 8-ball.  This has nitty-gritty details of how to get the data, but contains NO logic for converting the data in any way.  It can contain queries that allow us to bring back only the data we are interested in.  Those queries can do things for us like sorting our data, etc.  But, the dao layer is simply in charge of executing the queries and returning the data to the service layer.

Do you have any examples where you have seen a layer and thought it was too thin?

*A data access object is any object that is used for accessing data, such as code to talk to a database, or read from a file.  It is a commonly used Design Pattern.  Look forward to a future post about this.

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.

 

Empathy

This is a rather heavy article I just read, but I thought it was extremely important that people read and share.  It talks about our behaviors online.  It talks about the abuse that some people get (especially women and minorities) just for doing their jobs.  There is a video part of the way through the post which is really hard to watch, but is emotionally impactful.

Please read and share.  We need more empathy in this world.

http://blog.codinghorror.com/they-have-to-be-monsters/