Выдача по RCON

[Anarchist_YT]

Форумчане, проанализируйте пожалуйста мой код для связи сайт с сервером по RCON и скажите насколько я рукожоп?

<?php
class Rcon {
private $host;
private $port;
private $password;
private $timeout;
private $socket;
private $authorized;
private $last_response;

const PACKET_AUTHORIZE = 5;
const PACKET_COMMAND = 6;
const SERVERDATA_AUTH = 3;
const SERVERDATA_AUTH_RESPONSE = 2;
const SERVERDATA_EXECCOMMAND = 2;
const SERVERDATA_RESPONSE_VALUE = 0;

public function __construct($server, $timeout = 3) {
global $config;
$this->host = $config['servers'][$server]['host'];
$this->port = $config['servers'][$server]['port'];
$this->password = $config['servers'][$server]['password'];
$this->timeout = $timeout;
}

public function get_response() {
return $this->last_response;
}

public function connect() {
$this->socket = fsockopen($this->host, $this->port, $errno, $errstr, $this->timeout);
if (!$this->socket)
{$this->last_response = $errstr;
return false;}
stream_set_timeout($this->socket, 3, 0);
$auth = $this->authorize();
if ($auth)
{return true;}
return false;
}

public function disconnect() {
if ($this->socket) {
fclose($this->socket);
}
}

public function is_connected() {
return $this->authorized;
}

public function send_command($command) {
if (!$this->is_connected()) return false;
$this->write_packet(Rcon::pACKET_COMMAND, Rcon::SERVERDATA_EXECCOMMAND, $command);
$response_packet = $this->read_packet();
if ($response_packet['id'] == Rcon::pACKET_COMMAND)
{if ($response_packet['type'] == Rcon::SERVERDATA_RESPONSE_VALUE)
{$this->last_response = $response_packet['body'];
return $response_packet['body'];}}
return false;
}

private function authorize() {
$this->write_packet(Rcon::pACKET_AUTHORIZE, Rcon::SERVERDATA_AUTH, $this->password);
$response_packet = $this->read_packet();

if ($response_packet['type'] == Rcon::SERVERDATA_AUTH_RESPONSE) {
if ($response_packet['id'] == Rcon::pACKET_AUTHORIZE) {
$this->authorized = true;
return true;
}
}

$this->disconnect();
return false;
}

private function write_packet($packet_id, $packet_type, $packet_body) {
$packet = pack("VV", $packet_id, $packet_type);
$packet = $packet . $packet_body . "\x00";
$packet = $packet . "\x00";
$packet_size = strlen($packet);
$packet = pack("V", $packet_size) . $packet;
fwrite($this->socket, $packet, strlen($packet));
}

private function read_packet() {
$size_data = fread($this->socket, 4);
$size_pack = unpack("V1size", $size_data);
$size = $size_pack['size'];
$packet_data = fread($this->socket, $size);
$packet_pack = unpack("V1id/V1type/a*body", $packet_data);
return $packet_pack;
}

}

 

[Flaimer]

В принципе, нормальный код.

 

[Mansur]

В целом согласен с комментариям выше, код хороший

Но есть ряд моментов.

Использование глобальных переменных не очень хорошо. Таким образом у тебя класс Rcon зависит от глобальной переменной $config. И если по какой-то причине переменная $config окажется недоступной, то класс попросту не будет работать. (это называется - dependency injection. Подробнее -

Авторизуйтесь для просмотра ссылок.

или реализация от Symfony

Авторизуйтесь для просмотра ссылок.

). К тому же это удобнее для тестирования.

Поэтому я бы рекомендовал вынести $config в аргумент конструктора, да и вообще убрать зависимость от $config, представив только $server.

public function __construct(array $server, $timeout = 3) {

    $this->host = $server['host'];

    $this->port = $server['port'];

    $this->password = $server['password'];

    $this->timeout = $timeout;

}

//...

$rcon = new Rcon($config['lobby']);

//или же напрямую задать параметры сервера

$rcon = new Rcon([
    'host' => '127.0.0.1',
    'port' => '666',
    'password' => 'qwerty'
])ж

Можно пойти дальше и $config обьявить в виде класса, добавить неймспейсы и тд. Но не думаю что это необходимо. (Если интересно, то -

Авторизуйтесь для просмотра ссылок.

,

Авторизуйтесь для просмотра ссылок.

,

Авторизуйтесь для просмотра ссылок.

)


Еще можно подчистить за работой Rcon класса и добавить деструктор, но не уверен что это стоит сделать.

public function __destruct()
{
    $this->host = $this->port = $this->password = $this->timeout = null;
}

Ну и пару фиксов и советов

//Конструкция if может принимать несколько условий (www.tradebenefit.ru/uslovie-if-i-logicheskie-operatori-v-php)
if ($response_packet['id'] == Rcon::PACKET_COMMAND && $response_packet['type'] == Rcon::SERVERDATA_RESPONSE_VALUE) {
    $this->last_response = $response_packet['body'];
    return $response_packet['body'];
}
return false;

//Можно сразу передать $auth, учитывая что он может быть только boolean
//if ($auth) return true;
//return false;
return $auth;

//Если if имеет лишь одно действие при выполнении условия, то можно немного сократить конструкцию
if ($this->socket) fclose($this->socket);

//Конкатенацию строк можно проводить через оператор присваивания .= (www.php.net/manual/ru/language.operators.assignment.php)
$packet .= $packet_body . "\x00"; //равносильно $packet = $packet . $packet_body . "\x00";
// $packet .= "\x00"; Только я немного не понял почему нужно дважды добавить "\x00"