Пример рефакторинга мелких недочётов в методе формирования параметров запроса


Исходный код

protected function _buildParamsUrl($additional = array()) {
    $url = array();
    $params = $this->_request->getParams();
    $params = array_merge($params);
    foreach ($params as $var => $val) {
        if (in_array($var,$this->_neededParams) && is_scalar($val)) {
            $url[$var] = $val;
        }
    }
    
    $url = array_merge($url,$additional);
    
    return $url;
}

Что не так в исходном коде

  1. Метод _buildParamsUrl возвращает массив параметров, а не URL, поэтому я бы назвал его просто _buildParams.
  2. Сейчас у метода две ответственности: очистить массив параметров от ненужных и смёржить его с переданным извне массивом дополнительных параметров. Также метод заключает в себе знание о том, откуда берётся исходный список параметров. Желательно эти знания и ответственности как-то разделить.
  3. Первый вызов функции array_merge выглядит загадочно: возможно, его забыли убрать после очередной модификации метода.
  4. Названия переменных $var и $val для меня выглядят неубедительно: нет полной ясности, что они значат.
  5. Функция array_filter существует в PHP начиная с версии 4, безымянные функции с версии 5.3 — лучше пользоваться их комбинацией для фильтрации массивов, это гораздо прогрессивнее. Я всегда за использование паттерна MapReduce вместо циклов.

Вариант рефакторинга исходного кода

protected function _buildParams($dditional = array()) {
    $needed_params = $this->_filterNeededParams($this->_request->getParams());
    return array_merge($needed_params, $additional);
}

protected function _filterNeededParams($params) {
    return array_filter($params, function ($param_value, $param_name) {
        return in_array($param_name, $this->_neededParams) && is_scalar($param_value);
    }, ARRAY_FILTER_USE_BOTH);
}

Можно пойти ещё дальше и поделить фильтрацию needed params и скалярных значений, но понять необходимость такого шага можно только исходя из требований предметной области.