Is it bad practice to have executable code in my channel state object?

Question

I have a CommunicationStateMachine object that's attached to my channels. It keeps track of who's turn it is to speak, how far along the conversation the server and client are, things like the client ID, all bytes read to date, the ByteBuffer etc.

My previous version of this server did some checks in the Selector thread (bad idea, I know), like checking if the message was complete, and checking if the client was a valid one.
To that end I had the methods that did those checks declared in the CommunicationStateMachine.class and the Selector thread called those methods on every OP_READ pass.

Now I'm removing all that work and putting the responsibility on the worker threads, and in order to keep things a bit cleaner I've decided to use the State design pattern. I've made a class called Processor.class which contains abstract methods. Depending on what mode the server is in (reading/writing text or reading/writing hex commands) I've created classes that extend Processor.class and implement it's methods. The result of the JSON mode processor so far is this:

public class JsonMode extends Processor {

    @Override
    public void processBytes(CommunicationStateMachine commsState,
            DataDAO database) {

        boolean validHeaderFormat = commsState.isValidClientHeaderFormat();

        if (validHeaderFormat) {

            boolean messageComplete = commsState.checkJsonMessageIntegraty();
            log.debug("JsonProcessor: messageComplete: " + messageComplete);
            String deviceId = commsState.getDeviceId();
            FirmwareFile file = database
                    .checkScheduledFirmwareUpgrade(deviceId);
            if (file != null) {
                commsState.setScheduledUpdate(true);
                log.debug("This is a scheduled update for " + deviceId
                        + " for file: " + file);
            } else {
                commsState.setScheduledUpdate(false);
                log.info("Device "
                        + deviceId
                        + ": Not scheduled for an update, scheduledUpgrade returned null");
            }
        }
    }

    @Override
    public void prepareWriteBytes(CommunicationStateMachine commsState,
            DataDAO database) {
        // TODO Auto-generated method stub

    }

}

So my thinking here, is that the methods in the commsState object that are called here, such as commsState.isValidClientHeaderFormat() and commsState.checkJsonMessageIntegraty() should be removed from the CommunicationStateMachine object and placed in the JSONMode.class processor, meaning I will only call get and set methods from commsState and keep all processing outside that class.
This is the way to go right? My thinking is otherwise I'll either start adding callable code for the other modes to the CommunicationStateMachine.class and it will get more unwieldy, or I'll have JSONMode code in the CommunicationStateMachine.class and other Modes codes outside, which would definitely be wrong.

Any other thoughts you might have on this process would be greatly appreciated too.

Answer 1

I agree that you should pull this functionality out of CommunicationStateMachine. Your state machine should only expose the methods necessarily to make changes in state and to query the current state. Your CommunicationStateMachine class should also be responsible for enforcing the rules of your state machine, i.e. ensuring that only valid state transitions happen. This logic is typically hidden inside the class and invoked when an attempt is made to change state.

In general, the approach you've suggested (the State design pattern) seems like a sensible choice based on your description of your requirements.

Minor note: you've got a small typo on checkJsonMessageIntegraty, which should be Integrity at the end. It may also be more intuitive if your class is called JsonProcessor to reflect its purpose.