What Do You Care Most About When Reviewing Someone Else’s Code?

 My current expected answer is something less vague.  I don’t want you to just tell me that you want to see properly indented code; I want you to tell me something much more specific.

Take a look at the example code below.  This is well indented code, no large comment blocks and clear documentation explaining what I’m trying to accomplish.  This is taken from an article I wrote a long, long time ago called How to create a socket server in PHP (March 2^nd, 2009!).

// Set time limit to indefinite execution
set_time_limit (0);
// Set the ip and port we will listen on
$address = 'localhost';
$port = 10000;
$max_clients = 10;
// Array that will hold client information
$client = Array();
// Create a TCP Stream socket
$sock = socket_create(AF_INET, SOCK_STREAM, 0);
// Bind the socket to an address/port
socket_bind($sock, $address, $port) or die('Could not bind to address');
// Start listening for connections
socket_listen($sock);
echo "Waiting for connections...\r\n";
// Loop continuously
while (true) {
    // Setup clients listen socket for reading
    $read[0] = $sock;
    for ($i = 0; $i < $max_clients; $i++) {
        if (isset($client[$i]['sock']))
            $read[$i + 1] = $client[$i]['sock'];
    }
    // Set up a blocking call to socket_select()
    if (socket_select($read, $write = NULL, $except = NULL, $tv_sec = 5) < 1)
        continue;
    /* if a new connection is being made add it to the client array */
    if (in_array($sock, $read)) {
        for ($i = 0; $i < $max_clients; $i++) {
            if (empty($client[$i]['sock'])) {
                $client[$i]['sock'] = socket_accept($sock);
                echo "New client connected $i\r\n";
                break;
            }
            elseif ($i == $max_clients - 1)
                echo "Too many clients...\r\n";
        }
    } // end if in_array    
    // If a client is trying to write - handle it now
    for ($i = 0; $i < $max_clients; $i++) { // for each client
        if (isset($client[$i]['sock'])) {
            if (in_array($client[$i]['sock'], $read)) {
                $input = socket_read($client[$i]['sock'], 1024);
                if ($input == null) {
                    echo "Client disconnecting $i\r\n";
                    // Zero length string meaning disconnected
                    unset($client[$i]);
                } else {
                    echo "New input received $i\r\n";
                    // send it to the other clients
                    for ($j = 0; $j < $max_clients; $j++) {
                        if (isset($client[$j]['sock']) && $j != $i) {
                            echo "Writing '$input' to client $j\r\n";
                            socket_write($client[$j]['sock'], $input, strlen($input));
                        }
                    }
                    if ($input == 'exit') {
                        // requested disconnect
                        socket_close($client[$i]['sock']);
                    }
                }
            } else {
                echo "Client disconnected $i\r\n";
                // Close the socket
                socket_close($client[$i]['sock']);
                unset($client[$i]);
            }
        }
    }
} // end while
// Close the master sockets
socket_close($sock);

Sorry for the long code block, but I think this really helps illustrate the point.  In theory, this code would satisfy the “generic” answer.  I’ve indented my code correctly, lots of nice short and clear comments, and no large comment blocks.

As I stated earlier, past Jamie would have been ok with the generic answer; however, if I shoved this code block to you in an interview and ask you “is this what you mean?”.  Hopefully at this point, the interviewee might clue into that I’m setting them up for a trap.  Because if you’ve answered yes – yikes – that is some ugly code and I should know because I wrote it!

Whereas, if the answer is no, we can start digging into some specifics as to why.

First and foremost, this is a single code block, not a single function for this 78 lines of monstrosity.  Secondly, I can (now) clearly see a great case to put this in a class and split up the functionality a lot.

Let’s rewrite the code with this in mind.  While I’m at it, let’s remove some useless lines of comments that can be addressed through naming conventions; especially the stupid comments stating that it’s the end of the while or the if.  Man am I embarrassed by this code!

serverAddress = $serverAddress;
        $this->serverPort = $serverPort;
        $this->maxClients = $maxClients;
    }
}
class SocketServer {
    var $socketConfiguration;
    var $connectedClients = Array();
    var $listeningSockets = Array();
    var $socketServer;
    function __construct(SocketConfiguration $socketConfiguration = null) {
        if (null == $socketConfiguration) {
            $socketConfiguration = new SocketConfiguration();
        }
        $this->socketConfiguration = $socketConfiguration;
    }
    function __destruct() {
        $this->CloseSocket();
    }
    function CreateSocket() {
        $this->socketServer = socket_create(AF_INET, SOCK_STREAM, 0);
    }
    function BindSocket() {
        socket_bind($this->socketServer, $this->socketConfiguration->serverAddress, $this->socketConfiguration->serverPort) or die('Could not bind to address');
    }
    function ListenForConnections() {
        socket_listen($this->socketServer);
    }
    function InitializeSocketServer() {
        $this->CreateSocket();
        $this->BindSocket();
        $this->ListenForConnections();
    }
    function CloseSocket() {
        socket_close($this->socketServer);
    }
    function Execute() {
        while (true) {
            $this->PopulateListeningSockets();
            // Set up a blocking call to socket_select()
            if (socket_select($this->listeningSockets, $write = NULL, $except = NULL, $tv_sec = 5) < 1)                 continue;             $this->CheckForNewClientConnections();
            $this->CheckIfAnyClientsAreWriting($this->listeningSockets);
        }
    }
    function PopulateListeningSockets() {
        $this->listeningSockets[0] = $this->socketServer;
        for ($client = 0; $client < $this->socketConfiguration->maxClients; $client++) {
            if ($this->IsClientConnected($client))
                $this->listeningSockets[$client + 1] = $this->connectedClients[$client]['sock'];
        }
    }
    function CheckForNewClientConnections() {
        if (in_array($this->socketServer, $this->listeningSockets)) {
            for ($client = 0; $client < $this->socketConfiguration->maxClients; $client++) {
                if (empty($this->connectedClients[$client]['sock'])) {
                    $this->connectedClients[$client]['sock'] = socket_accept($this->socketServer);
                    echo "New client connected $client\r\n";
                    break;
                }
                elseif ($client == $this->socketConfiguration->maxClients - 1)
                    echo "Too many clients...\r\n";
            }
        }
    }
    function CheckIfAnyClientsAreWriting() {
        for ($client = 0; $client < $this->socketConfiguration->maxClients; $client++) {
            if ($this->IsClientConnected($client)) {
                if (in_array($this->connectedClients[$client]['sock'], $this->listeningSockets)) {
                    $input = socket_read($this->connectedClients[$client]['sock'], 1024);
                    if ($input == null) {
                        echo "Client disconnecting $client\r\n";
                        $this->ClearClientConnected($client);
                    } else {
                        echo "New input received $client\r\n";
                        $this->WriteToAllClients($client, $input);
                        if ($input == 'exit') {
                            $this->DisconnectClient($client);
                        }
                    }
                } else {
                    echo "Client disconnected $client\r\n";
                    $this->DisconnectClient($client);
                    $this->ClearClientConnected($client);
                }
            }
        }
    }
    function WriteToAllClients($currentClient, $message) {
        for ($client = 0; $client < $this->socketConfiguration->maxClients; $client++) {
            if ($this->IsClientConnected($client) && !$this->IsClientTheSame($client, $currentClient)) {
                echo "Writing '$message' to client $client\r\n";
                socket_write($this->connectedClients[$client]['sock'], $message, strlen($message));
            }
        }
    }
    function IsClientConnected($client) {
        return isset($this->connectedClients[$client]['sock']);
    }
    function IsClientTheSame($client1, $client2) {
        return $client1 == $client2;
    }
    function DisconnectClient($position) {
        socket_close($this->connectedClients[$position]['sock']);
    }
    function ClearClientConnected($position) {
        unset($this->connectedClients[$position]);
    }
}

Yikes!  This is even more code.  But that’s ok because it’s more readable code.

Please note, this code is untested, so if it doesn’t work, please let me know and I can make a correction as it’s just an example.

Let’s now look at how to use this code and get a socket server up and running:

$socketServer = new SocketServer();
$socketServer->InitializeSocketServer();
$socketServer->Execute();

I must say, this is much easier to provide to someone along with the one big SocketServer class – that hopefully shouldn’t require much editing – apart from adding new functionality!

Going back to the original purpose of the article, I would love to zoom in on just the endless while loop in both scenarios.  First, the single block of code:

// Loop continuously
while (true) {
    // Setup clients listen socket for reading
    $read[0] = $sock;
    for ($i = 0; $i < $max_clients; $i++) {
        if (isset($client[$i]['sock']))
            $read[$i + 1] = $client[$i]['sock'];
    }
    // Set up a blocking call to socket_select()
    if (socket_select($read, $write = NULL, $except = NULL, $tv_sec = 5) < 1)
        continue;
    /* if a new connection is being made add it to the client array */
    if (in_array($sock, $read)) {
        for ($i = 0; $i < $max_clients; $i++) {
            if (empty($client[$i]['sock'])) {
                $client[$i]['sock'] = socket_accept($sock);
                echo "New client connected $i\r\n";
                break;
            }
            elseif ($i == $max_clients - 1)
                echo "Too many clients...\r\n";
        }
    } // end if in_array    
    // If a client is trying to write - handle it now
    for ($i = 0; $i < $max_clients; $i++) { // for each client
        if (isset($client[$i]['sock'])) {
            if (in_array($client[$i]['sock'], $read)) {
                $input = socket_read($client[$i]['sock'], 1024);
                if ($input == null) {
                    echo "Client disconnecting $i\r\n";
                    // Zero length string meaning disconnected
                    unset($client[$i]);
                } else {
                    echo "New input received $i\r\n";
                    // send it to the other clients
                    for ($j = 0; $j < $max_clients; $j++) {
                        if (isset($client[$j]['sock']) && $j != $i) {
                            echo "Writing '$input' to client $j\r\n";
                            socket_write($client[$j]['sock'], $input, strlen($input));
                        }
                    }
                    if ($input == 'exit') {
                        // requested disconnect
                        socket_close($client[$i]['sock']);
                    }
                }
            } else {
                echo "Client disconnected $i\r\n";
                // Close the socket
                socket_close($client[$i]['sock']);
                unset($client[$i]);
            }
        }
    }
} // end while

Versus the cleaned up Execute method in the SocketServer class:

    function Execute() {
        while (true) {
            $this->PopulateListeningSockets();
            // Set up a blocking call to socket_select()
            if (socket_select($this->listeningSockets, $write = NULL, $except = NULL, $tv_sec = 5) < 1)                 continue;             $this->CheckForNewClientConnections();
            $this->CheckIfAnyClientsAreWriting($this->listeningSockets);
        }
    }

This is the end goal of how I would like the interviewee to attempt and guide the conversation.  Do not simply tell me the generic of nicely indented code, basic commenting, etc…  Dig right down into exactly what you want to see.

You want to see things like DRY (don’t repeat yourself), SRP (single responsibility pattern), good naming conventions, etc…  More importantly, do not just spit this buzz words or acronyms to me, tell me in detail what you mean by these things.

Summary

What used to be an “open-ended” question that I would skim over in less than a minute has now become – in my opinion – something that I can spend hours debating back-and-forth and really gaining a solid grasp of how this developer thinks and writes code; more importantly whether they care about the code they are writing!

The above refactoring example is not even nearly complete, but hopefully the point is being driven home in what I’m trying to extract when asking the question “What do you care most about when reviewing someone else’s code?”.

Published on Jun 10, 2013

Tags: Optimization | php | socket server | PHP | Rants