Устранение дублирования реализации и рефакторинг «извлечение функции» при рендеринге картинки

Исходный код

function ShowImage($strImage, $iMaxW=0, $iMaxH=0, $sParams=null, $strImageUrl="", $bPopup=false, $sPopupTitle=false, $iSizeWHTTP=0, $iSizeHHTTP=0)
 {
  global $DOCUMENT_ROOT, $DB;

  if(!($arImgParams = CFile::_GetImgParams($strImage, $iSizeWHTTP, $iSizeHHTTP)))
   return"";

  if($sParams === null || $sParams === false)
   $sParams = ' border="0" ';

  $iMaxW = intval($iMaxW);
  $iMaxH = intval($iMaxH);

  $strImage = htmlspecialchars($arImgParams["SRC"]);
  $intWidth = $arImgParams["WIDTH"];
  $intHeight = $arImgParams["HEIGHT"];
  $strAlt = $arImgParams["ALT"];

  if($sPopupTitle===false)
   $sPopupTitle=GetMessage("FILE_ENLARGE");

  $file_type = GetFileType($strImage);
  switch($file_type):
   case"FLASH":
    $iWidth = $intWidth;
    $iHeight = $intHeight;
    if($iMaxW>0 && $iMaxH>0 && ($intWidth > $iMaxW || $intHeight > $iMaxH))
    {
     $coeff = ($intWidth/$iMaxW > $intHeight/$iMaxH? $intWidth/$iMaxW : $intHeight/$iMaxH);
     $iWidth = intval(roundEx($intHeight/$coeff));
     $iHeight = intval(roundEx($intWidth/$coeff));
    }
    $strReturn = '
     <object
      classid="clsid:D27CDB6E-AE6D-11CF-96B8-444553540000"
      codebase="http://download.macromedia.com/pub/shockwave/cabs/flash/swflash.cab#version=6,0,0,0"
      id="banner"
      WIDTH="'.$iWidth.'"
      HEIGHT="'.$iHeight.'"
      ALIGN="">
       <PARAM NAME="movie" VALUE="'.$strImage.'" />
       <PARAM NAME="quality" VALUE="high" />
       <PARAM NAME="bgcolor" VALUE="#FFFFFF" />
       <embed
        src="'.$strImage.'"
        quality="high"
        bgcolor="#FFFFFF"
        WIDTH="'.$iWidth.'"
        HEIGHT="'.$iHeight.'"
        NAME="banner"
        ALIGN=""
        TYPE="application/x-shockwave-flash"
        PLUGINSPAGE="http://www.macromedia.com/go/getflashplayer">
       </embed>
     </object>
     ';
    return$bPopup? $strReturn : print_url($strImageUrl, $strReturn);

   default:
    $strReturn = "<img src=\"".$strImage."\" ".$sParams." width=\"".$intWidth."\" height=\"".$intHeight."\" alt=\"".htmlspecialchars($strAlt)."\" />";
    if($iMaxW > 0 && $iMaxH > 0) <em>//need to check scale, maybe show actual size in the popup window</em>
    {
     <em>//check for max dimensions exceeding</em>if($intWidth > $iMaxW || $intHeight > $iMaxH)
     {
      $coeff = ($intWidth/$iMaxW > $intHeight/$iMaxH? $intWidth/$iMaxW : $intHeight/$iMaxH);
      $strReturn = "<img src=\"".$strImage."\" ".$sParams." width=\"".intval(roundEx($intWidth/$coeff))."\" height=\"".intval(roundEx($intHeight/$coeff))."\" alt=\"".htmlspecialchars($strAlt)."\" />";

      if($bPopup) //show in JS window
      {
       if(strlen($strImageUrl)>0)
       {
        $strReturn =
         '<a href="'.$strImageUrl.'" title="'.$sPopupTitle.'" target="_blank">'.
         '<img src="'.$strImage.'" '.$sParams.' width="'.intval(roundEx($intWidth/$coeff)).'" height="'.intval(roundEx($intHeight/$coeff)).' alt="'.htmlspecialchars($sPopupTitle).'" />'.
         '</a>';
       }
       else
       {
        CFile::OutputJSImgShw();

        $strReturn =
         "<a title=\"".$sPopupTitle."\" onClick=\"ImgShw('".AddSlashes($strImage)."','".$intWidth."','".$intHeight."', '".AddSlashes(htmlspecialcharsex(htmlspecialcharsex($strAlt)))."'); return false;\" href=\"".$strImage."\" target=\"_blank\">".
         "<img src=\"".$strImage."\" ".$sParams." width=\"".intval(roundEx($intWidth/$coeff))."\" height=\"".intval(roundEx($intHeight/$coeff))."\" /></a>";
       }
      }
     }
    }
    return $bPopup? $strReturn : print_url($strImageUrl, $strReturn);

  endswitch;

  return $bPopup? $strReturn : print_url($strImageUrl, $strReturn);
 }

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

Когда я изучаю этот исходный код, по мере чтения приходится постоянно возвращаться назад, чтобы уточнить какую-то деталь. Условно, мне пришлось это сделать 30 раз.

Я люблю другой исходный код, который легко читается сверху вниз почти без необходимости возвращаться наверх. Почти — это условные два-три раза.

Три возвращения назад против 30 — вот к чему нужно стремиться, и я точно знаю, что такое возможно.

Исчерпывающий анализ исходного кода показал, что в нём кроме смешивания множества ответственностей содержатся также многочисленные дублирования.

Дублирование реализации

Первое дублирование, которое сразу бросается в глаза — вычисление уменьшенных размеров изображения, когда требуется масштабирование. Оно реализовано и для флеша, и для картинок, причём слегка различающимися способами.

Менее очевидное дублирование: четырёхкратно повторенный рендеринг тега img и четырёхкратно же повторенное вычисление ширины и высоты картинки intval(roundEx($intWidth/$coeff)) и intval(roundEx($intHeight/$coeff)).

Может возникнуть возражение, что рендеринг тега дублируется не четырежды, а дважды, а первый и последний рендеринги не являются дублирующими — однако, это не так. В первом рендеринге также упоминается название тега img, и прописываются в нём все те же атрибуты, а то что значения атрибутов иногда отличаются — так это не реализация отличается, а входные параметры реализации. А в последнем пропущен атрибут alt, однако это больше смахивает на багу.

Нарушение принципа единственного знания

Функция ShowImage знает довольно много подробностей: как смасштабировать объект, как рендерить конкретные теги, и в каких ситуациях какая конфигурация тегов нужна.

Прочие недостатки

Не все имена переменных и параметров понятны.
1. Например, переменная $strImage содержит в себе URL картинки, которую нужно отобразить, а параметр $strImageUrl — URL такой же картинки, но в полном размере. Между тем название не отражает факта, что разница между этими величинами — в размере картинки. Названия говорят, что в одном параметре URL, а в другом — не URL.
2. Названия $iSizeWHTTP, $iSizeHHTTP привели меня в тупик, даже прочтение документации не помогло.
3. $sParams — это дополнительные атрибуты тега img, а вовсе никакие не параметры.
4. $strReturn объясняет, что будет сделано с переменной, но не объясняет, что она такое.
Префиксы переменных употребляются вразнобой. В одной и той же функции, даже в списке параметров, можно видеть и однобуквенные префиксы, и такие же трёхбуквенные: $strImage, $sPopupTitle, $iWidth, $intWidth. Это слегка подбешивает мой перфекционизм.
Перезаписывается значение переменной $strReturn. Для меня перезаписывание значений — один из явных запахов плохого кода. Здесь хотя бы не меняется смысл значения при перезаписывании, но всё равно такой структуры алгоритмов следует избегать. При этом инициализация переменной пустым значением с последующей перезапиьсю пустого значения непустым — перезаписью не является.
Перезаписывается значение параметра $strImage. Вот этот случай — большой грех, так как тут меняется смысл значения, хранящегося в переменной.
$sParams, $strAlt и $sPopupTitle вычисляются в общем участке кода, а используются только в ветке картинки, причём $sPopupTitle только в подветке для попапа. Получается, что при рендеринге флеша они вычислятся, но не пригодятся.
Неясно, для чего понадобился самый последний return.
Неясно, для чего понадобилось объявление глобальных переменных в начале функции.
Многократная проверка одних и тех же условий для одних и тех же данных как правило запутывает код. Многократная проверка допустима тех же самых условий только если она касается не связанных между собой данных. В ветке рендеринга картинки дважды проверяется $bPopup (в if и в тернарном операторе ниже), и оба раза проверка влияет на одну и ту же разметку картинки.
К слову, мне не нравится название функции print_url, по смыслу своей работы она должна называться WrapLinkAround.

Как рефакторить исходный код

Метод составления плана для данного примера тоже очень хорошо подходит. Я анализирую весь код и выписываю краткие словесные описания основных действий, которые в нём выполняются:

отрендерить тег object для флеша
отрендерить тег img для картинки
завернуть тег изображения в ссылку
отрендерить собственно ссылку (она разная в разных ситуациях)
вычислить требуемые визуальные размеры изображения

К более мелким действиям можно отнести подготовку дополнительных атрибутов $sParams и подготовку альт-текста для картинки htmlspecialchars($strAlt).

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

function ShowImage($sImageFile, $iMaxWidth=0, $iMaxHeight=0, $sAddAttrs=null, $sFullSizeImageUrl="", $bPopup=false, $sPopupTitle=false, $iSizeWidthHTTP=0, $iSizeHeightHTTP=0)
{
  if (!($aImgParams = CFile::_GetImgParams($ImageFile, $iSizeWidthHTTP, $iSizeHeightHTTP)))
    return"";

  $sImageUrl = $arImgParams["SRC"];
  list($iViewWidth, $iViewHeight) = GetScaledSizes($arImgParams["WIDTH"], $arImgParams["HEIGHT"], $iMaxWidth, $iMaxHeight);

  if (GetFileType($strImage) === 'FLASH')
  {
    $sObjectTag = RenderFlash($sImageUrl, $sViewWidth, $sViewHeight);
    $sPictureHtml = $bPopup ? $sObjectTag : print_url($sFullSizeImageUrl, $sObjectTag);
  }
  else
  {
    $sAlt = $arImgParams["ALT"];
    $sPictureTag = RenderImgTag($sImageUrl, $iViewWidth, $iViewHeight, $sAlt, $sAddAttrs);

    if ($bPopup)
    {
      $sLinkUrl = empty($sFullSizeImageUrl) ? $sImageUrl : $sFullSizeImageUrl;
      $sLinkAttrs = RenderPopupLinkAttrs($sLinkUrl, $iWidth, $iHeight, $sAlt, $sPopupTitle);
      $sPictureHtml = print_url($sLinkUrl, $sPictureTag, $sLinkAttrs);
    }
    else {
      $sPictureHtml = $sPictureTag;
    }
  }

  return $sPictureHtml;
}

function GetScaledSizes($iFileWidth, $iFileHeight, $iMaxWidth, $iMaxHeight)
{
  if ($iMaxWidth > 0 && $iMaxHeight > 0 && ($iMaxWidth < $iFileWidth || $iMaxHeight < $iFileHeight)) 
  {
    $horFactor = $iMaxWidth / $iFileWidth;
    $verFactor = $iMaxHeight / $iFileHeight;
    $factor = min($horFactor, $verFactor);
    return array(intval(roundEx($iFileWidth * $factor)), intval(roundEx($iFileHeight * $factor)));
  }
  return array($iFileWidth, $iFileHeight);
}

function RenderFlash($sFlashUrl, $iWidth, $iHeight, $sFullSizeImageUrl = null, $bPopup = false)
{
  $sFlashUrl = htmlspecialchars($sFlashUrl);
  $sObjectTag = '
     <object
      classid="clsid:D27CDB6E-AE6D-11CF-96B8-444553540000"
      codebase="http://download.macromedia.com/pub/shockwave/cabs/flash/swflash.cab#version=6,0,0,0"
      id="banner"
      WIDTH="'.$iWidth.'"
      HEIGHT="'.$iHeight.'"
      ALIGN="">
       <PARAM NAME="movie" VALUE="'.$sFlashUrl.'" />
       <PARAM NAME="quality" VALUE="high" />
       <PARAM NAME="bgcolor" VALUE="#FFFFFF" />
       <embed
        src="'.$sFlashUrl.'"
        quality="high"
        bgcolor="#FFFFFF"
        WIDTH="'.$iWidth.'"
        HEIGHT="'.$iHeight.'"
        NAME="banner"
        ALIGN=""
        TYPE="application/x-shockwave-flash"
        PLUGINSPAGE="http://www.macromedia.com/go/getflashplayer">
       </embed>
     </object>';
    return $sObjectTag;
}

function RenderImageTag($sImageUrl, $iWidth, $iHeight, $sAlt = null, $sAddAttrs = null)
{
  $sAddAttrs = $sParams === null || $sParams === false ? ' border="0" ' : '';

  $sAltAttr = $sAlt ? ' alt="'.htmlspecialchars($sAlt).'"' : '';
  $sAddAttrs .= $sAltAttr;

  return '<img src="'.$sImageUrl.'" width="'.$iWidth.'" height="'.$iHeight.'" '.$sAddAttrs.' />';
}

function RenderPopupLinkAttrs($sLinkUrl, $iWidth, $iHeight, $sAlt, $sPopupTitle)
{
  $sLinkAttrs = 'target="_blank"';
  $sLinkAttrs .= ' ' . RenderPopupTitleAttr($sPopupTitle);
  $sLinkAttrs .= ' ' . RenderPopupOnClickAttr($sLinkUrl, $iWidth, $iHeight, $sAlt);
  return $sLinkAttr;
}

function RenderPopupTitleAttr($sPopupTitle)
{
  if ($sPopupTitle === false)
    $sPopupTitle = GetMessage("FILE_ENLARGE");
  return 'title="'.$sPopupTitle.'"';
}

function RenderPopupOnClickAttr($sImageUrl, $iWidth, $iHeight, $sAlt)
{
  CFile::OutputJSImgShw();
  $sImageUrlParam = AddSlashes($sImageUrl);
  $sAltParam = AddSlashes(htmlspecialcharsex(htmlspecialcharsex($sAlt)));
  return "onClick=\"ImgShw('".$sImageUrlParam."','".$iWidth."','".$iHeight."','".$sAltParam."'); return false;\"";
}

По ходу рефакторинга я не удержался и значительно упростил ветвления, касающиеся рендеринга картинки. Получившийся у меня алгоритм не повторяет исходный на 100%, зато в нём более ясно обозначены зависимости: в каком случае выполняется оборачивание картинки в ссылку для показа увеличенной версии в попапе, и как формируются атрибуты ссылки.

Один участок остался для меня загадкой — $bPopup ? $sObjectTag : print_url($sFullSizeImageUrl, $sObjectTag): почему в ветвлении используется условие $bPopup, а наличие $sFullSizeImageUrl не проверяется, хотя он может быть пустым.

Думаю, если бы владел полной документаций касательно всех нюансов поведения функции, я смог бы ещё немного упростить финальный код.

Govnokodu.Net