Ниже показан пример плохого кода с комментариями, которые объясняют, почему не следует так делать.
<?php
// начиная с php 7.1, в первой строке должен быть включен строгий режим: declare(strict_types=1);
// Не надо оставлять подобную информацию, git сделает это за вас.
/**
* Created by PhpStorm.
* User: Some.Name
* Date: 15.11.2017
* Time: 11:45
*/
// возможность менять значения константам разрешается только в файле конфигурации
defined('ORDER_NUMBER_LENGTH') || define('ORDER_NUMBER_LENGTH', 8);
// каждому классу-сервису необходимо создавать описание (нужно написать о том, чем занимается данный
// сервис с точки зрения бизнес-логики)
/**
* Class OrderAttachLisQuery - это просто бесполезная информация (удалите строку, а лучше сразу
* настройте PhpStorm, чтобы он не добавлял эту строку)
* @package My\Syper\OrderEmailSender - это просто бесполезная информация (удалите ее, а лучше сразу
* настройте PhpStorm, чтобы он не добавлял эту строку)
*/
class OrderEmailSender// если в проекте классам-сервисам принято добавлять суфикс Service, то это нужно
// делать, даже если имя класса самодостаточно
{
// имена констант названы нечитабельно, несмотря на то, что автор писал их подразумевая апостроф
// в притяжательных существительных, поэтому, в нашей команде принято не подразумевать апостроф
// в именах существительных, другими словами нужно использовать предлог of
const SERVICES_GROUPS = 1;// Группы услуг - читабельный вариант: GROUPS_OF_SERVICES
const SERVICES_ANALOG = 2;// Аналоги услуг - читабельный вариант: ANALOGS_OF_SERVICES
const SERVICES_GROUP_1С = 3;// Группа услуг из 1С - читабельный вариант: GROUP_OF_SERVICES_FROM_1C
public $email;// нельзя делать публичные переменные для неизменяемых объектов (не забывайте про это)
protected $db;// если переменная используются только в текущем классе, то ее следует сделать
// максимально защищенной (private)
private $userName;// забыли создать docblock https://docs.phpdoc.org/guides/types.html
// забыли создать docblock
public function __construct($userName)// :th (type hinting) - забыли объявить тип входящих данных,
// в php7 можно было бы написать string $userName
{
$this->db = \Yii::app()->db;// зависимости должны приходить извне
$this->user = new User($userName);// но переиспользуемые объекты могут порождаться в классах-сервсах
$this->userName = $userName;// скорее всего тут что-то не так, т.к. в сервис должны инжектится
// только зависимости (продолжение ниже)
}
// забыли добавить docblock, в котором обязательно должен быть комментарий на русском, описывающий
// работу метода с точки зрения бизнес-логики таким образом не нужно описывать в комментарии то,
// что происходит в методе, а название методу нужно давать с точки зрения бизнес-логики
public function buildTreeAndAddUsers(// обычно функции должны делать что-то одно, тут уже по имени
// ф-и видно, что ф-я делает два действия, возможно проблема
&$groups,// не используйте передачу данных по ссылке, это прямой путь к неявной ошибке
$value,// слишком абстрактное имя переменной, имена должны отражать бизнес-логику (понятней код)
$servNums// слишком короткое имя переменной + отсутсвует th, правильней: array $serviceNumbers
)// начиная с php 7.1, у ф-ии должен быть объявлен возвращаемый тип данных (забыли указать)
{// правильно, согласно psr2 фигурная скобка начала функции должна быть перенесена на новую строку
// В строке ниже забыли сделать проверку на наличие ключа в массиве, это ошибка, так делать
нельзя, входящие в данные всегда нужно проверять, независимо от того, проверялись они ранее (в
др. функции) или нет:
$message = $servNums['my_key'];
// в 99% случаев, требуется выполнять строгую проверку данных, отправив true третьим
// параметром в функцию in_array:
$message .= in_array('myGroupName', $groups) ? 'yes' : 'no';
$this->userName = 'NewName';// сервис не может менять свое состояние (так делать нельзя)
// еще раз, не надо использовать передачу данных по ссылке (неочевидное поведение)
foreach ($servNums as &$servNum){
// ... какой-то код
}
// Функции должны быть лишь одним уровнем абстракции. Второй цикл (while/foreach/etc) говорит
// о втором уровне абстракции, значит на функцию возложено слишком много задач.
// Разбиение функций позволяет многократно использовать код и облегчает тестирование. Тоже
// самое происходит, когда мы видим например foreach вложенный в foreach-е.
foreach ($servNums as $key => $serviceNumber) {
// правильная реализация изменений значений (через ключ элемента, а не по ссылке)
$servNums[$key] = $serviceNumber;
// если в while/foreach/etc > 1 строки, то лучше такой код вынести в отдельный метод,
// это улучшает читаемость и упрощает тестируемость (сказывается на будущей поддержки кода)
// но бывают простые случае, которые не вынесешь и это нормально, например:
$name = $this->findName($key);
if (!empty($name)){
$message .= $name;
}
}
// Использование двойных кавычек. Следует использовать одинарные кавычки, а двойные при
// крайней необходимости, например в SQL запросах:
$dataText = "Data text";
// Использование старого синтаксиса работы с массивами, следуют использовать короткий
// синтаксис: ['Logs text', ['data']]
$toLog = array('Log text', array('data'));
// addWarning и другие методы Monolog-а давно deprecated и теперь уже удалены
https://github.com/Seldaek/monolog/blob/master/src/Monolog/Logger.php
// поэтому, используйте warning + т.к. интерпритатор идет дальше, то возможно это notice:
\Container::getLogger()->addWarning('Some info', $toLog);
// нестрогая проверка, смысла в ней почти нет (представьте, что автор имел ввиду пустую
// строку, а переменная содержит строку из одного пробела)
if (!$value) {
return false;// метод должен возвращать один тип данных, либо выбрасывать эксепшен
}
// так делать нельзя, выполняется приведение к типу без проверки типа, сначала надо проверить
// в нашей команде обычно используют метод Helper-а, например ScalarValidator::getInteger()
$value = (int)$value;
if ($value < 0) {
// сообщение эксепшена не должно быть динамичным (если нужно передать данные выше по
// трейсу, то создайте свой класс эксепшена (например ExtraException), который с помощью
// монолог-процессора, может залогировать динамичные extra-данные
throw new \Exception('Wrong value: ' . $value);
}
// никогда не используйте switch, если тип сверяемых данных может быть разным, потому что
// выполняется нестрогое сравнение
switch ($value) {
case 0:
// например, при $value = 'string'; будет напечатана буква 'a', т.к. произойдет
// неявное приведение строки к числу:
echo 'a';
break;
case 1:
echo 'b';
break;
}// забыли добавить default: throw new \UnexpectedValueException('SomeMethod');
// непонятное условие - таким условиям добавляют комментарии, а в лучшем случае выносят в
// отдельную функцию (особенно, если условие или его часть используется дважды)
if ($value === 2 && count($servNums) > 2) {
return true;
}
$data = "hello";// всего лишь строка, которую ниже будут проверять как массив:
if ($data['user_id']) {// невероятно, но PHP стригерит Warning, но результат будет верным
echo 'yes';// и будет напечатано слово "yes"
}// все потому, что PHP приведет 'user_id' к int = 0, а затем возьмет 1-ый символ строки
$payDetail = new PaymentDetail();
// нет смысла создавать единожды используемую переменную массива, сразу
// формируйте массив в $payDetail->insertDetailForPay([...])
$params = [
'pay_id' => 1,
'order_num' => 2,
];
$payDetail->insertDetailForPay($params);
try {
// тут может быть плохой код, который выбрасывает \Exception в сообщении которого
// записаны данные (ниже неправильная обработка эксепшена)
} catch (\Exception $e) {
echo $e->getMessage();// нельзя отображать сообщение любого эксепшена (в сообщении
// могут быть данные, которые нельзя разглашать), печатать можно только сообщения
// PublicMessageException которые можно безопасно отображать/отдавать "клиенту"
}
$userModel = UserModel::model()->findByPk(123);
// некорректная проверка т.к.: $userModel может существовать но иметь тип отличный от UserModel
if (!isset($userModel)) {
return 'User not found';
}
// правильно: if (!$userModel instanceof UserModel) { + благодаря такой проверки IDE начнет
// автокомплитить переменную $userModel
// все что начинается с \Container::get должно инджектиться через __construct
echo \Container::get('Some\Service')->getUsersAsString();
// когда вы изменяете настройки PHP, всегда пишите рядом подробный комментарий описывающий
// причину изменения дефолтного значения:
set_time_limit(1800);
// не используйте магические функции, если есть возможность обойтись без магии (в 99% случаев
это возможно), лучше написать 50 штук if...elseif...
call_user_func($callback, $parameter); call_user_func_array($callback, $param_arr);
// обычно такие функции применяют, когда ленятся, в результате теряется связанность кода
// если используется один из этих методов, значит нарушается контракт задекларированный
// интерфейсом (а может быть интерфейса вовсе нет и его надо создать)
method_exists($object, $method_name); property_exists($class, $property);
// видите это в коде, значит в этом месте делается костыль.
exit;// не используйте exit или die, используйте единую точку выхода, хотя бы Yii::app()->end();
}
/**
* @return array|null - ПЛОХО: метод возвращает 2 типа данных, дабы исключить множество видов
* проверки вариантов возврата, решено что метод должен возвращать 1 тип данных, а в
* случае ошибки - выбрасывать эксепшен. Исключение: объект (например: сущность/модель), если
* она неполучена из б.д. - возвращаем null
*/
function countDaysBeforeNewYear()
{
$nomerDnya = [];// не именуйте переменные транслитерацией русского, имя должно быть на английском
return $nomerDnya > 365 ? null : $nomerDnya;
}
/**
* Ужасно абстрактное имя метода (функция должна иметь название согласно бизнес-логики)
*
* переданные аргументы имеет неправильный порядок, всегда уникальное значение
* важнее, всегда аргумент имеющий значение по-умолчанию является наименее важным
*
* @return CDbDataReader - ПЛОХО: метод возвращает ссылку на поток данных (stream/resource), а
* должен возвращать данные (из-за этого могут быть проблемы с долгим чтением данных, например
* из таблицы б.д., ведь транзакции должны быть максимально короткими).
*/
function getDataFromDb(\DateTime $date, $email = null, $userId)
{
return $this->db->createCommand('SELECT * FROM table')->query();
}
/**
* @return string
*
* В анотации должны указываться все исключения, которые выбрасывает метод, за исключением тех
* исключений, которые выбрасываются по стеку ниже.
*
* Если метод сам не выбрасывает исключений, а по стеку они явно могут быть выброшены, то не нужно
* писать анотации исключений вообще. Такое решение принято потому что на практике, мы все равно не
* учитываем все разнообразие всех исключений, которые могут быть выброшены, тем более, когда код
* по стеку ниже позже поменяется (нашими силами или в сторонни библиотеках)
* @throws ReflectionException
*/
function getUsername(): string
{
return ...;
}
// Магические методы которые использовать нежелательно
function __toString(){}// использование нужно согласовывать с ревьювером заранее
function __invoke(){}// без обсуждения используют только в проектах где это стандартная практика
// Магические методы, которые использовать запрещается (если все же понадобится что-то из списка
// ниже, то нужно согласовывать их заранее)
function __destruct(){}
function __call(){}
function __callStatic(){}
function __get(){}
function __set(){}
function __isset(){}
function __unset(){}
function __sleep(){}
function __wakeup(){}
function __set_state(){}
function __clone(){}
function __debugInfo()
}Внимание, все выше изложенное не повод спросить, обсудить и предложить свой вариант реализации.