Нужна критика php-кода

Xeon

Проверенные
Сообщения
143
Реакции
13
Баллы
8,115
Доброго времени суток, нужна ваша оценка моего первого скрипта....
Я не кодер 5 летним стажем, а новичок с недельным, так что прошу умников со словами "говнокодер" проходить мимо...
Вот сам скрипт:
Ps. Есть много повторов в коде, не успел заменить...
 
Последнее редактирование:
Бегло гляну. Там много всякого, чего стоило бы исправить. Из основного:
  1. Лучше стоит использовать PDO или же встроенный класс XenForo для подключения к бд. Это я функцию. Дело в том, что все функции mysql_* уже являются устаревшими даже в уже устаревающем PHP 5.5
  2. лучше вынести в отдельную переменную/константу. Чтобы не приходилось ползать по всему коду, выправляя пути к движку форума.
Дальше углубляться в код, откровенно говоря, лениво.
 
  1. mysql_* как уже выше заметили использовать сейчас категорически нельзя.
  2. - для такого числа ветвлений логичнее использовать case.
  3. - функция для генерации ключа конечно хороша, но удалите неоднозначные символы хотя бы - O и 0, I и 1 и т.п. Если предусмотрена возможность передачи ключа кому-то, то за отсутствие похожих символов люди вам потом спасибо скажут.
  4. Писать в таблицы движка напрямую, тем более в xf_users - дурной тон, у движка есть DataWriter для этого дела, использовать лучше его. Раз вы данные сессии получаете нормально, то можно было бы и писать по человечески.
  5. - кодировку для чего принудительно ставите? Так делать не положено вообще, база может быть в отличной от указанной.
  6. - не советую использовать ?> для закрытия php-кода, можно не закрывать, интерпретатор разберется. Если в файле нет мешанины как в данном случае, то отказ от него поможет избежать пачки проблем с непечатными символами, BOM и т.п.
  7. - вообще файл крайне запутанный, но вот конкретно эти две переменные вы конечно получаете, но фильтровать кто будет? Они там ниже в md5 пакуются и так далее, но особо это ничего не меняет, лучше заранее отфильтровать на опасные символы то, что вы от пользователя получили, потому что потом если логика работы изменится - сразу и не вспомните.
  8. Все про тот же файл - у вас излишне сложная структура if/else, переделайте хотя бы базовую часть на case, код будет проще значительно восприниматься и получится некоторого дублирования подключения к базе, например, избежать.
  9. - мешанина html/php - худшее, что только может быть. Так как вы этот класс по идее вызываете на отдельной странице движка, то перенесите хотя бы CSS в его же шаблоны.
PS. Тут скрытие реферрера бьет ссылки на конкретные строки кода, копируйте ссылки вручную.
 
Всем спасибо огромное, многое для себя уяснил!
 
Доброго времени суток, нужна ваша оценка моего первого скрипта....
Я не кодер 5 летним стажем, а новичок с недельным, так что прошу умников со словами "говнокодер" проходить мимо...
Вот сам скрипт:
Ps. Есть много повторов в коде, не успел заменить...
В некоторых местах можно использовать switch().
Например тут -
Названия некоторых функций плохо говорят о том, что они делают.
Например:
function items($name) {
function getItems($name) { (я просто не вникал что делает, вижу селект какой-то)
В принципе, код достаточно легко читался (лично для меня), никаких "спагетти-кода" не видел, что есть хорошо.
Но некоторые штуки лучше отделить отдельными отступами, что бы например не промочиться с горизонтальным скоролом.
А так круто, молодец, неужели это все за неделю практики php?
 
  • Мне нравится
Реакции: Xeon
Захотелось покодить, и решил пофиксить магаз...
  1. mysql_* как уже выше заметили использовать сейчас категорически нельзя.
  2. - для такого числа ветвлений логичнее использовать case.
  3. - функция для генерации ключа конечно хороша, но удалите неоднозначные символы хотя бы - O и 0, I и 1 и т.п. Если предусмотрена возможность передачи ключа кому-то, то за отсутствие похожих символов люди вам потом спасибо скажут.
  4. Писать в таблицы движка напрямую, тем более в xf_users - дурной тон, у движка есть DataWriter для этого дела, использовать лучше его. Раз вы данные сессии получаете нормально, то можно было бы и писать по человечески.
  5. - кодировку для чего принудительно ставите? Так делать не положено вообще, база может быть в отличной от указанной.
  6. - не советую использовать ?> для закрытия php-кода, можно не закрывать, интерпретатор разберется. Если в файле нет мешанины как в данном случае, то отказ от него поможет избежать пачки проблем с непечатными символами, BOM и т.п.
  7. - вообще файл крайне запутанный, но вот конкретно эти две переменные вы конечно получаете, но фильтровать кто будет? Они там ниже в md5 пакуются и так далее, но особо это ничего не меняет, лучше заранее отфильтровать на опасные символы то, что вы от пользователя получили, потому что потом если логика работы изменится - сразу и не вспомните.
  8. Все про тот же файл - у вас излишне сложная структура if/else, переделайте хотя бы базовую часть на case, код будет проще значительно восприниматься и получится некоторого дублирования подключения к базе, например, избежать.
  9. - мешанина html/php - худшее, что только может быть. Так как вы этот класс по идее вызываете на отдельной странице движка, то перенесите хотя бы CSS в его же шаблоны.
PS. Тут скрытие реферрера бьет ссылки на конкретные строки кода, копируйте ссылки вручную.
Спасибо, что все конкретно расписали, а сейчас по пунктам:
1. Сделал... Сразу хотел исправить, потому, что в логи спамило из-за этого...
2. Поправил... Действительно выглядит более логично...
3. Ключ передается в цифровом виде, так что таких проблем быть не должно, да и ко всему этому, мне, как перфекционисту, сложно так сделать.)))
4. Тот кусок с сессией я использовал именно для частичного использование с ксеней... Подразумевалось, что скрипт может использоваться не только с Xenforo...
5. Вроде были проблемы с русскими именами... Хотя не уверен, закомментировал, но возможности проверить не было...
6. Поправил...
7. Не сильно разбираюсь как обезопасить всевозможные перехваты ключей и тд, и пытался сделать хоть что-то более защищенное со стороны логики, только не знаю как работает часть кода с сессией (которая от xenforo), а точнее, боюсь что можно как-то сессию подставить...
8. --/--
9. Выкладывал файл просто для примера... Самого тошнит от него)))
 
Современный облачный хостинг провайдер | Aéza
Назад
Сверху Снизу