Unit Testing and the Law of Demeter

17Jan/104

Unit Testing and the Law of Demeter

I was writing some code today and not using Test-Driven development. The reason was that I did not have a good understanding of what I was writing, so I decided to write some of the guts before writing the tests. In the process of writing the guts, I recognized that I was paying very close attention to how I was going to later test each of the methods I was writing. I was paying especially close attention to the Law of Demeter. The idea behind the Law of Demeter is to keep units of code distinct from one another. So how did this relate to my code? To put it simply, my business logic methods did not use get methods.

Assume we have a person class. The class constructor takes the persons full name and we have a getter and setter for the full name.

<?php
class Person
{
    protected $fullName;
 
    public function __construct($fullName)
    {
        $this->setFullName($fullName);
    }
 
    public function setFullName($fullName)
    {
        $this->_fullName = $fullName;
    }
 
    public function getFullName()
    {
        return $this->_fullName;
    }
}

There is also a method that parses apart the persons full name into separate parts. The naive approach:

    public function parseFullName()
    {
        $nameParts = explode(' ', $this->getFullName());
        ...
    }

This code works fine, but think about how we will write a test for this function. We have to first set the full name using a setter so the parseFullName method can retrieve that value with a getter. This violates one of the principle rules of unit testing: testing individual units of code. If there is a bug in the getter or setter functions, they may inadvertently affect my test. This is a very real issue when using the magic __get and __set methods. It also means more work to setup your tests because you have to keep in mind an order of operations when testing.

The better approach:

    public function parseFullName($fullName)
    {
        $nameParts = explode(' ', $fullName);
        ...
    }

Notice how the parseFullName function is being told what to parse rather than asking what to parse. This subtle change now allows us to truly test this individual unit of code with the least amount of outside environment interaction.

Tagged as: php, testing, unittest Leave a comment

http://www.milesj.me Miles Johnson

Nice post and easily understandable when it comes to unit testing. However, it seems this theory won’t really apply to applications that aren’t specifically low/loose coupled (Like most large web applications).

Also, when using parseFullName() in actuality, just seems it would be a lot longer to write. I guess it depends on the context of how you will be using it in your app.

$name = $this->parseFullName($this->getFullName());
Herman Radtke

The parseFullName method was a simple example, not something I was actually working on. It is obviously much more complicated than just splitting on spaces and thus, facilitates the need for unit testing this method.

I think the above idea applies best to methods that do some business logic like calculate or parse something. Even if the method is part of a large, highly coupled class, we can still tell the method what to parse/calculate/etc rather than let it ask. But there is obviously a trade off here: now something must know what to pass into that method. No problem If there is a service layer, but as you mention, most people just slap code in a controller and move on.

Maybe I will write another post showing how a service layer facilitates good design, which implicitly helps unit testing.
http://www.johnkleijn.nl John Kleijn

I am leaning toward disagreement about the parse method. Passing the subject of the parsing really doesn’t constitute a stateful interface in its current form (the method might as well have been defined as static). But lets say the parsing is a more involves process, that does require certain instance variables. Wouldn’t it then be more appropriate to “encapsulate the concept that varies”, and create a “PersonNameParser”? In fact, I think that would be more appropriate in any case, unless you intend the parsed name to be the default output (merged with “getFullName”).

My point is that I think it defeats the purpose of a domain object to request operations on the state of that same object. Actually I think you’ve done the exact opposite of Tell Don’t Ask. Tell Don’t Ask doesn’t mean “pass arguments instead of using internal state”, it means “decisions based entirely upon the state of one object should be made ‘inside’ the object itself”. And in trying to improve testability you actually violated that and broke encapsulation. Because the decision of what to use an argument to explode() has effectively been delegated to the client.

Sure, one should try to keep to the Law of Demeter to avoid “train wrecks” and improve cohesion on a package or application level, but I don’t think that extends to methods used within the same class to fetch internal state.

On a side note I think you have very nice blog here. I came across it when looking for info about Thrift for a HBase adapter I’m planning to write, but found a few other interesting topics as well. They have given me some ideas for my own blog as well.
Herman Radtke

Looking at this post again, you are correct. This was just contrived example, but it definitely violates some OO principles. I can’t remember exactly what I was working on when I decided to post this, but it makes me want to review it and make sure I did not make the same mistake there.

Thanks for keeping me honest!