Код
$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> ';
}
}
Что не так с этим кодом
- Если судить чисто по приведённому фрагменту кода (не знаю, что там ещё есть дальше), то запрос к базе выполняется слишком рано. Сначала мы должны убедиться, что его результат понадобится (то есть проверить
$_SESSION['userid']
), а потом только собственно делать запрос. - Запрос извлекает из базы все строки, подходящие под заданное условие, хотя в обработку потом идёт только одна строка. В данном случае это вряд ли повлияет (скорее всего, в таблице присутствует только одна строка с указанным заголовком), но в другой ситуации такая привычка может сыграть злую шутку: запросы могут выполняться заметное время, и страницы сайта начнут тормозить.
- Слишком много дублирующегося кода. В таких случаях всегда нужно создавать функцию. Здесь нарушен принцип единственной реализации: код для реализации формирования ссылки повторен многократно.
- Какая-то магия с
$size = ''; $size=getimagesize()
. Такие вещи ни в коем случае нельзя оставлять в коде — с ними нужно разбираться и устранять, чтобы не вызывать потом вопросов. А если уже без магии совсем никак — то нужно писать к этому месту комментарий: что именно не работает и для чего так было сделано. - Инлайновые стили задавать нежелательно — лучше объявить новый класс.
- Инлайновые обработчики событий тоже в общем случае являются плохой практикой.
Исправленная и улучшенная версия кода
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();
}
});
Теория
- Принцип единственной реализации