A cyclical dependancy anti-pattern: Identity Theft

Anti-patterns:

In software engineering, an anti-pattern (or antipattern) is a design pattern that appears obvious but is ineffective or far from optimal in practice.


Or as I like to think of it, the type of code that makes you want to rip your eyes out, like in that movie Event Horizon…

Identity Theft

Here’s a pattern that I have seen many times in a previous codebase, which I believe suffers from some design flaws:

Description

FooListing is a repository-type object that holds a list of Foo objects, which it populates from the database using its populate() function. The Foo object also has a populate() method, but this method instantiates a FooListing, and uses the FooListing populate function with its key – so now the FooListing contains the desired Foo object.
Our original Foo object now contains a reference to the Foo object that it wants to be.  So it does what anyone would do, steals its identity and hides the body.
Foo.populate() looks something like this:

public void populate() {
  FooListing listing = new FooListing()
  // Set up FooListing
  FooListing.setSearchKey(id);
  if (listing.size > 0) {
      Foo foo = listing.get(0);
      this.setBar(foo.getBar);
      // continue to copy over properties
      // ...
  } else {
      LOG.warn("Foo not found for key: " + id);
  }
}

Why this is bad?

Firstly, apart from the questional morality, it is duplicated code. FooListing already has the capability to create and populate a Foo object from the database. Two locations for this code means twice the maintenance if something changes, (more than) twice the possibility of bugs, etc.
Foo and FooListing have become tightly coupled and dependant on each other under this design – there is a cyclic dependancy which is a code smell, and may cause headaches when writing unit tests.
There is also a waste of resoures creating the uneccesary FooListing and Foo objects inside of Foo.populate(), at least some of that could be avoided by client code accessing instances of Foo via FooListing.populate
It also doesn’t make sense that a data access object like Foo should be concerned with information about how it is created. Foo should act more like a bean, or an immutable class.
Another dangerous aspect of this design is the implication that client code accessing Foo cannot be certain that Foo has been instantiated correctly. If the client code has a faulty key for Foo that does not exist in the database, when it creates new Foo(key) there is no way to know that Foo.populate() has failed to find the correct value, and instead they are left with a faulty Foo instance which was not what they requested.

Solution

The best solution for this isolated pattern is to completely remove (or deprecate) the Foo.populate() method, and replace calls to it with FooListing instances.
If FooListing fails to find a matching Foo, the client code should realise this when FooListing returns them a null object. The client code can handle and recover from this case in context.
Implementing a getFirstResult() function in FooListing could be beneficial if there are many cases where the code with otherwise be calling get(0).
We could also simplify the calling code so that retrieving a result is a one-line operation – i.e. get() calls populate() if the list has not already been populated.

public final class FooListing {
  private List<Foo> _listing;
  private int _id;

  public FooListing {
  }

  public FooListing(int id) {
    _searchId = id;
  }

  public void populate() {
    _listing = new ArrayList<Foo>();
    // Query the database and add to _listing
  }

  public Foo get(int index) {
    if (_listing == null) {
      populate();
    }
    if (_listing.isEmpty() || index >= _listing.size()) {
      return null;
    } else {
      return _listing.get(index);
    }
  }

  public Foo getFirstResult() {
    return this.get(0);
  }

  public List<Foo> getList() {
    return _listing;
  }

  public void setSearchId(int id) {
    _id = id;
  }
}

And the client code would only need to call

Foo foo = new FooListing(id).getFirstResult();

References:

1. http://en.wikipedia.org/wiki/Anti-patterns