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