4

I'm having a problem using the state pattern, I don't know how to check if a State is of a certain instance without using instanceOf (because that is considered a bad practice).

TCPConnection holds a TCPState object. Let's say I want to get all the TCPConnections that have the state TCPEstablished. How would I do this? enter image description here

A way would be:

public List<TCPConnection> getAllEstablished() {
  List<TCPConnection> list = new ArrayList<TCPConnection>();

  for(TCPConnection tcp : allConnections) {
      if(tcp.getState().instanceOf(TCPEstablished)) {
          list.add(tcp);
      }
  }

  return list;
}

But this uses instanceOf and I would prefer not to use that. Are there any better ways? Or is my use of instanceOf valid?

jaco0646
  • 15,303
  • 7
  • 59
  • 83
Stanko
  • 4,275
  • 3
  • 23
  • 51
  • 1
    For state management, I would use `enum` rather than classes. – Luiggi Mendoza Apr 23 '15 at 17:50
  • 1
    There is nothing wrong in using `instanceof` operator in your case. However, the usage is syntactically incorrect. `instanceof` is not a method, it's an operator. – Rohit Jain Apr 23 '15 at 17:50
  • @LuiggiMendoza Why would an enum be better? – Stanko Apr 23 '15 at 17:51
  • 1
    Why is instanceOf not good practice? – EDToaster Apr 23 '15 at 17:52
  • 1
    @JClassic: It isn't bad practice it is a code smell. If you need to know what class a variable is you probably have too tight of coupling and need to rethink how you are using that variable. That isn't always the case (serializing being an obvious example) but it is something to keep in mind when using the operator. – Guvante Apr 23 '15 at 17:53
  • @Guvante what if I have something that returns a random `GameState` out of the set of `[Game, Setting, Menu]`, but I want to do an operation based on what State i receive? Since they all extend State, and are received as such? – EDToaster Apr 23 '15 at 18:01
  • 1
    btw, `tcp.getState().instanceOf(TCPEstablished)` breaks the [Law of Demeter](http://en.wikipedia.org/wiki/Law_of_Demeter). – proskor Apr 23 '15 at 18:29
  • @JClassic: Usually you make it a method of the `GameState` but it really depends on the specifics. I usually try to avoid using `instanceof` to make semantic changes as that makes the coupling very very tight. For instance if I added a `SubMenu` that inherits from `Menu` I end up with a lot of unknown behavior that is embedded in various places that call `instanceof` without my knowledge. – Guvante Apr 23 '15 at 21:39

2 Answers2

5

Yes, the use of instanceOf is considered a smell. Furhermore, checking for the type of a state object goes contrary to the idea of the state pattern itself (encapsulate state-dependant behavior in the subtypes such that such checks become unnecessary).

Nevertheless you could technically get rid of the instanceOf by adding another operation to TCPState, e.g. bool isEstablished(), and implement it such that it returns true only on TCPEstablished.

interface TCPState {
    ...
    boolean isEstablished();
}

class TCPEstablished implements TCPState {
    ...
    boolean isEstablished() {
        return true;
    }
}

class TCPClosed implements TCPState {
    ...
    boolean isEstablished() {
        return false;
    }
}

Add the operation to TCPConnection:

class TCPConnection {
    ...
    boolean isEstablished() {
        return this.getState().isEstablished();
    }
}

Then your operation getAllEstablished would look like this:

List<TCPConnection> getAllEstablished() {
    List<TCPConnection> list = new ArrayList<TCPConnection>();

    for(TCPConnection tcp : allConnections) {
        if(tcp.isEstablished()) {
            list.add(tcp);
        }
    }

    return list;
}

instanceOf is gone. But is it worth it?

proskor
  • 1,382
  • 9
  • 21
3

You can make your states an enum

public enum TCPState{
  ESTABLISHED,
  LISTEN,
  CLOSED;

  ..methods go here

}

Since enum values are full fledged objects they can override methods to do state specific behavior. You can then do equals() or even == checks to check state.

for(TCPConnection tcp : allConnections) {
  if(tcp.getState()==TCPState.Established)) {
      list.add(tcp);
  }
}

EDIT

Here's how to make each enum value have a different implementation for a method.

let's say TCPState implements an interface or has an abstract method foo()

public enum TCPState{
  ESTABLISHED,
  LISTEN,
  CLOSED;

 public abstract void foo();

}

You can then implement the method differently for each value like this:

 public enum TCPState{
  ESTABLISHED{
     @Override
     public void foo(){
         System.out.println("established");
     }
  },
  LISTEN{
     @Override
     public void foo(){
         System.out.println("listening");
     }
  },
  CLOSED{
     @Override
     public void foo(){
         System.out.println("closed");
     }
  }
  ;

 public abstract void foo();

}

Alternatively, you can make a base implementation in TCPSTate instead of declaring it abstract and then only use overrides in the values that need to do something different.

dkatzel
  • 31,188
  • 3
  • 63
  • 67