审查他人代码时，您最关心的是什么？
2.73/5 (4投票s)
这篇文章讨论了你在审查别人代码时最关心的事情。
几个月前我写了一篇文章，名为《引人入胜的面试问题》，在几个开放式的技术问题中，我问了一个问题：“在审查别人的代码时，你最关心什么？”
有趣的是，当你往下读几行时，我会补充说，我并没有特别期待什么；只是一些通用的口头禅，大多数人都会说出来。例如，代码缩进正确，没有大段的注释块，用文档来解释复杂的代码块等等。
如果今天你问我对这个问题的期望是什么，我的答案将完全不同！今天，在我写这篇文章的时候，如果你现在问我，让我们看看我的答案是否会改变。
我现在期望的答案是更具体的东西。我不想你只是告诉我你想看到代码缩进正确；我希望你告诉我一些更具体的东西。
看看下面的示例代码。这是缩进良好的代码，没有大段的注释块，并且有清晰的文档解释了我想要完成的事情。
// 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);
抱歉这段代码块很长，但我认为这确实有助于说明问题。理论上，这段代码会满足通用的答案。我已经正确地缩进了我的代码，有很多简洁明了的注释，并且没有大段的注释块。
正如我之前所说，过去的Jamie可能会接受通用的答案；但是，如果我在面试中把这段代码块给你，问你这就是你想要的吗？希望此时，面试者可能会意识到我正在给他们设置陷阱。因为如果你回答“是”，那糟糕了，那是一些难看的代码，我应该知道，因为那是我写的！
然而，如果答案是“否”，我们可以开始深入研究原因的具体细节。
首先，这是一个单一的代码块，而不是一个单一的函数，用于这个78行的庞然大物。其次，我现在可以清楚地看到一个很好的理由将它放在一个类中，并将功能分解成更多部分。
让我们以此为基础重写代码。在重写的同时，让我们删除一些可以通过命名约定来解决的无用注释；特别是那些愚蠢的声明，说明while或if结束的注释。天啊，我为这段代码感到难堪！
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]);
}
}
哎呀！这甚至有更多的代码。但没关系，因为它更易读。
请注意，这段代码未经测试，所以如果它不能工作，请告诉我，我可以进行更正，因为它只是一个例子。
现在让我们看看如何使用这段代码来启动并运行一个套接字服务器。
$socketServer = new SocketServer(); $socketServer->InitializeSocketServer(); $socketServer->Execute();
我必须说，将这段代码以及一个大的SocketServer类提供给别人要容易得多，希望除了添加新功能之外，它不需要太多的编辑！
回到本文的最初目的，我想专注于两个场景中的无尽的while循环。首先，是单个代码块
// 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
与SocketServer类中清理后的Execute方法相比
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);
}
}
这就是我希望面试者尝试和引导对话的最终目标。不要简单地告诉我关于代码缩进良好、基本注释等等的通用答案。直接深入了解你到底想看到什么。
你想看到像DRY（不要重复自己）、SRP（单一职责模式）、良好的命名约定等东西。更重要的是，不要只是向我吐出这些流行语或缩写，告诉我你具体是什么意思。
摘要
过去，这是一个开放式的问题，我会在不到一分钟的时间里略过它，但现在在我看来，我可以花几个小时来来回回地争论，真正地了解这个开发者是如何思考和编写代码的；更重要的是，他们是否关心他们正在编写的代码！
上面的重构示例甚至还不完整，但希望通过这个问题，我试图提取的内容已经变得清楚了。在审查别人的代码时，你最关心什么？