Thursday 6 December 2012

Code Smells - The Lying Interface

I came across a code smell today that I hadn't seen / heard before. I've christened it the "Lying Interface". At work, we've been trying to refactor some code into a shared service accessible by WCF. To do this, we need to understand the interfaces and then develop the appropriate protocol to model this.

I've been faced with some interfaces like this:

public interface IPoller {   
     void Poll();
  }

This doesn't look too bad. The Poll method presumably calls every so often to poll something. Looking at the constructor then shows something a bit hidden.

public class Poller : IPoller {
    private IPollListener m_Listener;
  
    public Poller(IPollListener listener) { 
       m_Listener = listener;
    }

    public void Poll() {
      // run some code and generate some results
      m_Listener(results);
    }
  }

So despite the poll method being simple, it's deceptive. It actually relies on a constructor argument being passed in and invokes the results upon it. The interface is really lying, it's not showing the true responsibilities of the class. Perhaps another name for this is "hidden dependency"?

My opinion is that the interface should be self-describing, it should say what the class does without saying how. In this case, the hidden dependency on the IPollListener class is not expressed properly. I ended up refactoring the code to be more self-describing and using an event instead, roughly the new code ends up looking like this.

public interface IPoller {
     void Poll();
 
     event EventHandler<PollingEventArgs> Polled
   }

   public class PollingEventArgs : EventArgs {}

It's still not perfect, without a comment I have no way of knowing from the method signatures what events are triggered, but I think it's better than it was. This has the neat side effect of breaking the dependency on Poller on IPollListener and switching over to an Observer pattern.

I'm still not sure what the right name for this smell is, maybe it's more to do with hidden dependencies, but it's a start!

2 comments:

  1. The whole name of the interface is incorrect imho. Its not a poller at all. It reacts to events this way while a Poller monitors a target itself eg it's an actor instead of a reactor. When it discovers a change it should send an event itself.
    So yes the interface is lying but better its not the lying here that is the case its the naming.

    ReplyDelete
  2. This is true. Perhaps it's a bad example. The thing I found interesting was that there was a hidden dependency that wasn't shown on the interface, but was vitally important to the behaviour of the class.

    ReplyDelete