Поспешные действия, дублирующийся код


Код

$result = mysql_query("SELECT * FROM `Wallpapers` WHERE `Title` = '".$Title."'");
$row = mysql_fetch_assoc($result);
$wID=$row['ID'];
$Wallpaper1=$row['Wallpaper1'];
$Wallpaper2=$row['Wallpaper2'];
$Wallpaper3=$row['Wallpaper3'];
$Wallpaper4=$row['Wallpaper4'];
$Wallpaper5=$row['Wallpaper5'];

if ( $_SESSION['userid'] )
{ 
	if ($Wallpaper1 != NULL) {
		$size='';
		$size=getimagesize("images/wallpapers/".$Wallpaper1);
		$dHTML .= '<a class="preview" href="'.$image_path.'show/'.$Title.'/?w=1" style="text-decoration: underline;" onClick="alerts('.$wID.'); countIp();">'.$size[0].'x'.$size[1].'</a> ';
	}
	if ($Wallpaper2 != NULL) {
		$size='';
		$size=getimagesize("images/wallpapers/".$Wallpaper2);
		$dHTML .= '<a class="preview" href="'.$image_path.'show/'.$Title.'/?w=2" style="text-decoration: underline;" onClick="alerts('.$wID.'); countIp();">'.$size[0].'x'.$size[1].'</a> ';
	}
	if ($Wallpaper3 != NULL) {
		$size='';
		$size=getimagesize("images/wallpapers/".$Wallpaper3);
		$dHTML .= '<a class="preview" href="'.$image_path.'show/'.$Title.'/?w=3" style="text-decoration: underline;" onClick="alerts('.$wID.'); countIp();">'.$size[0].'x'.$size[1].'</a> ';
	}
	if ($Wallpaper4 != NULL) {
		$size='';
		$size=getimagesize("images/wallpapers/".$Wallpaper4);
		$dHTML .= '<a class="preview" href="'.$image_path.'show/'.$Title.'/?w=4" style="text-decoration: underline;" onClick="alerts('.$wID.'); countIp();">'.$size[0].'x'.$size[1].'</a> ';
	}
	if ($Wallpaper5 != NULL) {
		$size='';
		$size=getimagesize("images/wallpapers/".$Wallpaper5);
		$dHTML .= '<a class="preview" href="'.$image_path.'show/'.$Title.'/?w=5" style="text-decoration: underline;" onClick="alerts('.$wID.'); countIp();">'.$size[0].'x'.$size[1].'</a> ';
	}
}

Что не так с этим кодом

  1. Если судить чисто по приведённому фрагменту кода (не знаю, что там ещё есть дальше), то запрос к базе выполняется слишком рано. Сначала мы должны убедиться, что его результат понадобится (то есть проверить $_SESSION['userid']), а потом только собственно делать запрос.
  2. Запрос извлекает из базы все строки, подходящие под заданное условие, хотя в обработку потом идёт только одна строка. В данном случае это вряд ли повлияет (скорее всего, в таблице присутствует только одна строка с указанным заголовком), но в другой ситуации такая привычка может сыграть злую шутку: запросы могут выполняться заметное время, и страницы сайта начнут тормозить.
  3. Слишком много дублирующегося кода. В таких случаях всегда нужно создавать функцию. Здесь нарушен принцип единственной реализации: код для реализации формирования ссылки повторен многократно.
  4. Какая-то магия с $size = ''; $size=getimagesize(). Такие вещи ни в коем случае нельзя оставлять в коде — с ними нужно разбираться и устранять, чтобы не вызывать потом вопросов. А если уже без магии совсем никак — то нужно писать к этому месту комментарий: что именно не работает и для чего так было сделано.
  5. Инлайновые стили задавать нежелательно — лучше объявить новый класс.
  6. Инлайновые обработчики событий тоже в общем случае являются плохой практикой.

Исправленная и улучшенная версия кода

function makeWallpaperLink($row, $number) 
{
    global$image_path, $Title;
    $wallpaper = $row["Wallpaper$row"];
    if ( $wallpaper ) {
        $size = getimagesize("images/wallpapers/".$wallpaper);
        return'<a class="preview wallpaper" href="'.
        	$image_path.'show/'.$Title.'/?w='.$number.
        	'" data-id=".$row['ID'].">'.
        	$size[0].'x'.$size[1].'</a> ';
    }
    return'';
}

if ( $_SESSION['userid'] )
{
    $result = mysql_query("SELECT * FROM `Wallpapers` WHERE `Title` = '".$Title."' LIMIT 1");
    $row = mysql_fetch_assoc($result);
    for ($i = 1; $i <= 5; $i++) {
        $dHTML .= makeWallpaperLink($row, $i);
    }
}
.preview.wallpaper {
    text-decoration: underline;
}
document.addEventListener('click', function (event) {
    if (event.target.classList.has('wallpaper')) {
        alerts(event.target.dataset.id);
        countIp();
    }
});

Теория

  • Принцип единственной реализации