Правильно ли составлена функция php, mysql?

Question

[kamtm]

Добрый день!

Только изучаю программирование, поэтому, лучше уж спрошу, чем буду думать, что все работает, значит, все правильно.

Функция выводит данные из таблицы, при этом можно указать кол-во данных для вывода.

Соответственно в скрипте просто пишу show_banner(5);

Сам код:

function show_banner($kolvo) {
global $link;
if (!$q = mysqli_query($link, "SELECT banner_id,banner_title,banner_text,banner_img FROM banner LIMIT $kolvo")) echo 'Ошибка загрузки баннеров';
else 
if (mysqli_num_rows($q) > 0) {
while ($row = mysqli_fetch_assoc($q)) 
echo "
<div class='last_banner'>
<div class='banner_item'>
<img src='".$row['banner_img']."' alt='".$row['banner_title']."'>
<a href='/click/banner_id".$row['banner_id']."' target='_blank'>".$row['banner_title']."</a>
".$row['banner_text']."
</div>
</div>
";
}
 mysqli_free_result($q); 
}

Спасибо.

Answer 1

[Сергей Протько]

Не совсем понял что вы хотите выяснить, просто запустите код. Так что просто пару советов:

1) global $link; - старайтесь не использовать глобальные состояния. Коль уж пишите процедурный код - пусть он будет по настоящему процедурным:
function show_banner($kolvo, $link) {

2) старайтесь не делать вывод в функции которая у вас берет данные из базы - пусть она возвращает только данные (либо ничего в случае ошибки) а за вывод пусть отвечает другая функция.

$banners = get_banners($count, $link);
show_banners($banners);

Comments

[kamtm]

Спасибо. Меня как раз такие моменты интересовали. Стараюсь читая с 0 книги по php, тут же практиковаться, но порой сомнения берут, правильно ли.
А момент с mysqli_free_result($q); , верно указан? То есть именно после while я очищаю память?

[Сергей Протько]

@kamtm да, чистить память нужно после того как вы уже все что нужно прочитали. Иначе будет ошибка.

[kamtm]

@Fesor спасибо за ответ. Единственное немного не понял ваш второй комментарий. То есть, я могу в функции присвоить переменным значения базы, например $title = row['title'] вы это имели ввиду? Тогда немного непонятно, как вторую функцию реализовать.

[Сергей Протько]

у вас первая функция возвращает данные в том виде, с которым вам приятнее будет работать в представлении, например в виде массива данных. Таким образом вы отделяете логику от представления, и при изменении шаблона не нужно будет менять функцию, которая будет лесть в базу. И наоборот, если вы вдруг решите перевести все на PDO вам не нужно будет менять шаблоны. А еще вы можете вдруг решить вынести шаблоны в файлы и так как все будет инкапсулировано в функцию show_banners то менять придется только ее, весь остальной код от этого не пострадает.

Конечно в контексте вашего примера это все может казаться бессмысленным, так как у вас очень простая задача. Но все же в реальной жизни вам придется писать намного более сложные штуки.

[kamtm]

@Fesor, спасибо еще раз за развернутый ответ. В итоге переписал код, следуя вашим советам.

Answer 2

[Nick Murzin]

Сразу приучайте себя правильно именовать переменные.
kolvo - показатель плохого тона и "поповщины", используйте number, num , amount, amt, quantity, qt, qty

Comments

[kamtm]

Спасибо за дельный совет! Видимо такие истины приходят с опытом или как-то интуитивно? В книгах редко встретишь подобные советы. Я несколько лет программирую с большими перерывами, что-то типа досок объявлений, гостевых получалось делать. Но сейчас хочу как-то лучше чтоли писать.

[Сергей Протько]

@kamtm переменные должны быть названы так, что бы вгзлянув на название можно было сразу определить зачем она там нужна. Есть даже шутка, что в WEB разработке есть только две сложные вещи: именование переменных и инвалидация кэша.

Answer 3

[Виталий Желтяков]

Используйте PDO.

Вот Вам на заметку. Думаю разберётесь сами что с этим делать.

//-------------------------- Класс работы с базой ----------------------------//
class DBPDO {
   // Используем статическую переменную для сохранения значений внутри класса
	static private $database = NULL;
	
	// Конструктор выполняет соединение с БД возращает объект БД или false
	private function __construct(){
		try {
   		// Сразу задаём кодировку соединения
			self::$database = new PDO('mysql:host=localhost; dbname=basename', 'user', 'password', array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES \'UTF8\''));
   		// Устанавливаем уровень показа ошибок базы
   		self::$database->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
		} catch (PDOException $e){
			$error = $e->getMessage();
     		writelog('sql_error', date("y.m.d H:m:s")."\t".$error);
   		return false;
		}
	}
	
	// Статическая функция синглтон для получения объекта ДБ
	static public function dbconnect() {
		if (self::$database == NULL) { new DBPDO(); }
		return self::$database;
	}
	
	// Функция выполнения запроса 
   static public function sql_query($query) {
   	try {
         $return = self::dbconnect()->query($query)->fetchAll(PDO::FETCH_ASSOC);
         if ($return === false) return false;
         if ((!isset($return[1]))&&(isset($return[0]))) $return = $return[0];
      } 
      catch (PDOException $e) { 
         $error = $e->getMessage();
     		writelog('sql_error', date("y.m.d H:m:s")."\t".$error);
   		return false;
      }

   	return $return;
   }	

   // Запрос без выборки
	static public function exec($query) {
      try { 
         return self::dbconnect()->exec($query);
      } 
      catch (PDOException $e) { 
         $error = $e->getMessage();
     		writelog('sql_error', date("y.m.d H:m:s")."\t".$error);
   		return false;
      }
	}
	
   // Экранирование спецсимволов
	static public function quote($str) {
      return self::dbconnect()->quote($str);
	}

	// Получение id последней изменённой строки
	static public function lastInsertId() {
      return self::dbconnect()->lastInsertId();
	}
}
//----------------------------------------------------------------------------//

Comments

[kamtm]

Спасибо, но пока слишком сложно для меня. Наверное, лучше начать с основ, чтобы потом уже более уверенно смотреть на ваш код.

[FanatPHP]

Очередная бессмысленная и дырявая обертка над ПДО.

Answer 4

[FanatPHP]

Функция составлена отвратительно.
Две самых главных проблемы - вывод и работа с БД.

1) Нужно всегда разделять логику приложения и логику отображения.
Во-первых, как правильно написал Fesor, никакого вывода в функциях быть не должно.
Во-вторых, и в остальном коде работа с БД и вывод должны быть всегда разделены. Сначала работает код, который получает данные из базы, и только потом начинается вывод, желательно в отдельном файле-шаблоне.

2) Несмотря на модную буковку i названиях функций, сам подход остался прежним, и представляет собой классический говнокод из прошлого века.
Во-первых, любые данные всегда должны подставляться в запрос только через плейсхолдеры. новичкам это традиционно сложно понять, поэтому рекомендуется просто запомнить
Во-вторых, работать с БД надо не напрямую, а через враппер. Самый примитивный враппер - это PDO. Оно совсем не такое страшное, как написал каментатор выше, и код на самом деле получается короче, чем через mysqli

function get_banners($limit) {
  global $link;

  $sql = "SELECT banner_id,banner_title,banner_text,banner_img FROM banner LIMIT ?";
  $stm = $link->prepare($sql);
  $stm->execute([$limit]);
  return $stm->fetchAll();
}

или (с осмысленным враппером):

function get_banners($limit) {
  $sql = "SELECT banner_id,banner_title,banner_text,banner_img FROM banner LIMIT ?";
  return DB::prepare($sql)->execute([$limit])->fetchAll();
}

Вот пример нормального вывода:

<?
$banners = get_banners(5);
?>

<?php foreach ($banners as $row): ?>
<div class='last_banner'>
  <div class='banner_item'>
    <img src='<?=$row['banner_img']?>' alt='<?=$row['banner_title']?>'>
    <a href='/click/banner_id<?=$row['banner_id'].?>' target='_blank'><?=$row['banner_title']?>"</a> 
    <?$row['banner_text']?>
  </div>
</div>
<?php endforeach ?>

Впоследствии весь блок с HTML можно будет вынести в отдельный файл

А вот mysqli_free_result() как раз делать здесь бессмысленно - эта функция будет вызвана автоматически.

Comments

[kamtm]

Спасибо за ответ. Правда мне сейчас так удобнее было, когда все можно править в одном файле. Но, например у меня будет файл banner.tpl, как мне его по уму здесь задействовать? То есть оставив мою функцию с использованием mysqli?

[FanatPHP]

@kamtm Во-первых и в-самых главных, надо различать важность замечаний. Разделение вывода - это хорошо, но не принципиально. А вот использование подстановок в запросах - это примерно в сто раз важнее. И "оставлять мою функцию" можно только если её тоже переписать на использование подстановок. Но тогда код вырастет еще в два раза. По поводу же разделения - без проблем держать всё в одном файле. Главное - логчески разделять код и отображение, а так же никогда не писать HTML как строки в пхп. я сейчас отредактирую свой ответ чтобы показать пример. Впоследствии код вывода можно будет просто вынести в файл banners.tpl.php и тупо за-include-ить его