Minecraft query tools

This is my first major PHP project and I haven't gotten much feedback. I wanted to learn how to use PHP and I wanted to learn about Minecraft's status protocol, so I did what was obvious at the time.

File listing

The code is also on GitHub.

mcstat.php

Contains class MinecraftStatus and is also a stand-alone CLI tool.

```
#!/usr/bin/env php
hostname = $hostname;
$this->port = $port;
}

public function ping()
{
$newStats = $this->serverListPing($this->hostname, $this->port);
$this->stats[microtime()] = array(
'stats' => $newStats,
'method' => 'Server List Ping',
'hostname' => $this->hostname,
'port' => $this->port
);

return $newStats;
}

public function query($fullQuery=true)
{
if ($fullQuery) {
$newStats = $this->fullQuery($this->hostname, $this->port);
$this->stats[microtime()] = array(
'stats' => $newStats,
'method' => 'Full Query',
'hostname' => $this->hostname,
'port' => $this->port
);
} else {
$newStats = $this->basicQuery($this->hostname, $this->port);
$this->stats[microtime()] = array(
'stats' => $newStats,
'method' => 'Basic Query',
'hostname' => $this->hostname,
'port' => $this->port
);
}

return $newStats;
}

/*
================
Server Li

Foreword

Bearing in mind that this post was created more than 4 years ago, you likely have changed the code and learned more about PHP features - perhaps you don't even use/maintain this code anymore. I honestly have never played Minecraft, nor had any reason to monitor server statuses. But I wanted to give some feedback to this question.

Feedback

The code looks pretty sophisticated. I honestly haven't used pack() before. That is neat that the class MinecraftStatus can function as a stand-alone CLI tool.

Suggestions

Utilize member variable instead of passing

The methods MinecraftStatus::ping() and MinecraftStatus::query() make calls to private methods like serverListPing(), basicQuery() and fullQuery(), which all appear to be private methods. Instead of passing the hostname and portnumber to those methods, the methods can utilize the member variables using $this->. That way, the member variables don't need to be passed around.

Mapping Character to colors

The switch statement in MC_parseMotdColors() could be replaced by a lookup in an array.

define('COLOR_MAPPING', [
    '0' => '000000',
    '1' => '0000aa',
    //2-9, a-e...
    'f' => 'ffffff',
    'r' => 'ffffff'
]);

Then lookup the value of $character using array_key_exists():

$color = false; //default value
if (array_key_exists($character, COLOR_MAPPING)) {
    $color = '#' . COLOR_MAPPING[$character];
}

The same is true for the switch statement within MinecraftStatus::fullQuery() - though instead of using define(), a class constant could be created using const:

const RESPONSE_ITEM_KEY_MAPPING = [
    'numplayers' => 'player_count',
    'maxplayers' => 'player_max',
    'hostname' => 'motd',
    'hostip' => 'ip',
    'hostport' => 'port',
];

and then that constant can be accessed using self::RESPONSE_ITEM_KEY_MAPPING.

$key = $item; //default value
if (array_key_exists($item, self::RESPONSE_ITEM_KEY_MAPPING)) {
    $key = self::RESPONSE_ITEM_KEY_MAPPING[$item];
}

Alternatives to fwrite(), fread(), fclose()

I am not sure if it would work but you could consider using cURL wrapper functions or file_get_contents - that way the parsing of individual characters could possibly be eliminated.

Reading characters in getStrings()

I haven't tested it but in theory the (outer) while loop could be simplified to a for loop:

for ($nulsProcessed = 0; $nulsProcessed < $count; $nulsProcessed++) {
    while ($c != chr(0)) {
        $s .= $c;
        $c = fread($fp, 1);
    }
    $strings[] = $s;
    unset($c);
    unset($s);
})

And also the nested while could also be rewritten as a for loop:

while($c = fread($fp, 1); $c != char(0); $c = fread($fp, 1)) {
    $s .= $c;
}

StackExchange Code Review Q#42405, answer score: 3