Исходный код
switch($action){
default:
case 'main':
{
if($request_do == 'adtb'){
$sql->Query("SELECT * FROM utb WHERE `siteid` = $request_site AND `uid` = ".$_SESSION['id'].";");
if($sql->size_of_result == 0){
$sql->Query("INSERT INTO utb VALUES(null,".$_SESSION['id'].",$request_site,'$request_url');");
}
}
if($request_do == 'del' && isset($request_id)){
$sql->Query("SELECT * FROM utb WHERE `id` = $request_id AND `uid` = ".$_SESSION['id'].";");
if($sql->size_of_result != 0){
$sql->Query("DELETE FROM utb WHERE `id` = $request_id;");
}
}
if($request_do == 'save' && is_array($request_tbdata)){
foreach($request_tbdataas$k=>$v){
$sql->Query("UPDATE utb SET `url` = '$v' WHERE `id` = $k;");
}
}
$sql->Query("SELECT `id`, `domen`, `sitename` FROM sites;");
$sdata = $sql->GetAssoc();
$sql->Query("SELECT *,(SELECT `domen` FROM sites WHERE `id` = utb.id) as `site` FROM utb WHERE `uid` = ".$_SESSION['id'].";");
$udata = $sql->GetAssoc();
$sql->Query("SELECT `id`, `domen` FROM sites WHERE `id` NOT IN(SELECT `siteid` FROM utb WHERE `uid` = ".$_SESSION['id']." );");
$data = $sql->GetAssoc();
$smarty->assign('sdata',$sdata);
$smarty->assign('data',$data);
$smarty->assign('udata',$udata);
$smarty->assign('id',$_SESSION['id']);
$smarty->display('links.tpl');
break;
}
}Что не так в исходном коде
Архитектура контроллера
Легко видеть, что это код контроллера для отрабатки простейших CRUD-запросов. Структурно все четыре запроса оформлены в виде единого фрагмента кода. При этом каждая из трёх операций по изменению данных заканчивается отображением этих данных на веб-странице.
В вебе стандартным является подход, когда добавление, удаление, изменение и отображение — это четыре отдельные операции, которые максимально изолированы друг от друга. Технически это выглядит как четыре отдельных веб-запроса, причём изменяющие запросы работают через HTTP-методы, отличные от GET, а после выполнения соответствующей операции с данными делают редирект на нужную страницу, уже через метод GET.
Раз действия разделены логически, то логично будет их раскидать по отдельным методам, объединённые в классы соответственно контроллеру. Такой подход тоже широко применяется в различных фреймворках.
Код, который определяет, какой контроллер будет обрабатывать поступивший веб-запрос, и каким экшеном, называется роутером, и он ничего не знает о том, как будет обрабатывать запрос.
Запросы к БД при отображении данных
Для меня выглядит сомнительной необходимость во втором и третьем запросах. Они оба, причём довольно запутанных способом, извлекают всю ту же информацию, что и первый запрос, только поделенную на два сегмента.
Получается, что эти два запроса предоставляют не саму информацию, а способ разделения уже имеющейся информации на сегменты.
Для меня более логичным выглядит вариант с получением нужных строк по отдельности из каждой таблицы, а потом комбинирования этой информации во всех нужных сочетаниях.
Названия
Некоторые названия предельно лаконичны: таблицв utb, переменные $data, $sdata, $udata, $k, $v. Нужно много всего знать про остальной код, чтобы видеть межжду ними разницу. Чтобы помнить разницу между data, sdata, udata, нужно довольно сильно напрягаться.
Для меня важно, чтобы названия переменных точно называли то, что в них хранится. Например, в переменной $request_site хранится не сам сайт, а его идентификатор, а в переменной $request_url — не просто URL, а URL сайта, который нужно добавить. Поэтому более подходящие названия: $site_id и $site_url. В переменной $request_id, опять же, находится не просто айди, а айди сайта, то есть снова $site_id.
Экранирование входных данных для избежания SQL-инъекций
Оно отсутствует.
Оптимальность алгоритмов добавления и удаления записей
Нет необходимости проверять наличие записи перед добавлением — стоит воспользоваться средством игнорировать ошибку при добавлении дублирующейся записи. Такая возможность есть и в MySQL, и в PostgreSQL.
Нет необходимости проверять существование записи перед удалением: если нужной записи нет — просто ничего не будет удалено.
Вариант рефакторинга исходного кода
class MainController extends WebController
{
public function add() {
$site_id = $sql->EscapeInteger($this->params['site_id']);
$site_url = $sql->EscapeString($this->params['site_url']);
$sql->Query("INSERT IGNORE INTO utb VALUES(null,".$_SESSION['id'].",$request_site_id,'$request_url');");
$this->redirectToAction('show');
}
public function delete() {
$site_id = $sql->EscapeInteger($this->params['site_id']);
$sql->Query("DELETE FROM utb WHERE `id` = $site_id;");
$this->redirectToAction('show');
}
public function update() {
$urls = $this->params['urls'];
if (is_array($urls)) {
foreach($urlsas$url_id => $url){
$url_id = $sql->EscapeInteger($url_id);
$url = $sql->EscapeString($url);
$sql->Query("UPDATE utb SET `url` = '$url' WHERE `id` = $url_id;");
}
}
$this->redirectToAction('show');
}
public function show() {
$sql->Query("SELECT `id`, `domen`, `sitename` FROM sites;");
$sites = $sql->GetAssoc();
$sql->Query("SELECT `siteid` FROM utb WHERE `uid` = {$_SESSION['user_id']}");
$bookmark_site_ids = $sql->GetColumn();
list ($user_sites, $other_sites) = $this->splitSites($sites, $bookmark_ids);
$smarty->assign('all_sites', $sites);
$smarty->assign('user_sites', $user_sites);
$smarty->assign('other_sites', $other_sites);
$smarty->assign('user_id', $_SESSION['user_id']);
$smarty->display('links.tpl');
}
private function splitSites($all_sites, $user_bookmark_site_ids) {
$bookmark_index = array_fill_keys($user_bookmark_site_ids, true);
$user_sites = array_filter($all_sites, function ($site) use ($bookmark_index) { returnarray_key_exists($site['id'], $bookmark_index); });
$other_sites = array_filter($all_sites, function ($site) use ($bookmark_index) { return !array_key_exists($site['id'], $bookmark_index); });
return [$user_sites, $other_sites];
}
}