Removing Switch Statement Smell from a Websocket Server

Question

I'm not sure this question belongs here since it's "debatable", but let's give it a try.

After reading lots of examples and questions about it (like this one which seems really similar), I still can't figure out if I should opt for polymorphism to replace a switch.

It's all about a Ratchet WebSocket server which receives JSON messages and executes a routine depending on the message type:

public function onMessage(ConnectionInterface $from, $msg) {
    /*!
    Triggers everytime a message is received by the application.
    Depending on @p $msg type, the application will broadcast @p $msg accordingly.

    @param ConnectionInterface $from 
    This is the socket (client) who sent @p $msg.
    @param string $msg
    A JSON string sent by the client.
    */
    $usermsg = json_decode($msg, true);
    if (isset($usermsg["message"])) {
        $actual_msg = $this->removeHTML($usermsg["message"]);
    }

    switch ($usermsg["type"]) {

        case 'text':
            $this->text($from, $actual_msg, "text");
            break;

        case 'token':
            $this->token($from, $usermsg["im"]);
            break;

        case "ready":
            $this->ready($from);
            break;

        case "action":
            $this->text($from, $actual_msg, "action");
            break;

        case "users":
            $this->whoIsOnline($from);
            break;

        case "listen":
            $this->listen($from);
            break;

        case "end":
            $this->finish($from, $actual_msg);
            break;

        case 'statInit':
            $this->statInit($from);
            break;
    }
}

Thing is, $msg is a string since it's JSON, no object is instantiated for any message that arrives at all. This is why there's no class hierarchy, because messages are not objects. In fact, there is no other class than the actual Server.

In any case, this is the only switch that exists server-side (there's another one in client, but it´s jQuery so it's a different story), so adding new functionality should be adding a case AND a method, not that hard. The project is not gonna grow much, but I'd like it to be easily scaled.

Should I stick with OOP design and create an object for each message that arrives and apply polymorphism? Seems a little bit overwhelming since the server handle chat messages.

score 3 · Accepted Answer · answered Dec 18 '15 at 01:19

On one hand, at the end of the day this works for you and is relatively easy to maintain. Going full OOP may just be an exercise in purity and/or ego.

If you were to take the OOP approach to the next level, if it were me, I'd create a SocketMessage class that accepts a JSON message string in the constructor. I would also create a SocketHandler class with individual handler classes for each message type that extends it. Some rough code of how your method would look afterwards is:

public function onMessage(ConnectionInterface $from, $json) 
{
    $message = new SocketMessage($json);
    return $message->dispatch();
}

The dispatch() call would be part of the SocketMessage class and would determine which handler to direct it to based on the message type. It would know what handlers were available because you would have first registered them with the base SocketHandler class.

// Somewhere that you initialize code
SocketHandler::register('text', Handlers\TextHandler::class);
SocketHandler::register('token', Handlers\TokenHandler::class);
SocketHandler::register('ready', Handlers\ReadyHandler::class);

Registering a handler would just put them in a static array in the class to be accessed later. Inside the SocketMessage::dispatch() function, you would probably do something like this:

class SocketMessage
{
    ...
    public function __construct($json)
    {
        // Parse raw message data
        $this->message = @json_decode($json, true);
        if (empty($this->message)) {
           throw new Exception('SocketMessage expects a valid JSON string.');
        }
    }
    ...
    public function dispatch()
    {
        $type = $this->message['type'];
        $handler = SocketHandler::get($type);

        return $handler->handle($this->message);
    }
    ...
}

I'm not really sure where I am going with all of this. My main point is that you can make it more object oriented, which will result in it being more flexibile and extensible, but that also can bring along more difficulties with maintainability. It's really a matter of preference and project scope (i.e. is it going to be a large project maintained by lots of people, or is it a hobby script you are playing with in your spare time?)

This gives me a good perspective on what to work on next. Since this project is kinda like a "hobby" script, I should leave it at is. Thank you for your insight. — saxarona, Dec 18 '15 at 18:21

Removing Switch Statement Smell from a Websocket Server

1 Answers1