Прошу конструктивной критики на мой код на PHP

Приветствую! Так сложилось, что повышаю навыки программирования я полностью самостоятельно, и ввиду специфики работы код мой никто не проверяет. Поэтому прошу раскритиковать меня, чтобы я мог разобрать ошибки и поработать над ними. Я понимаю, что чтение чужого, мягко говоря, не очень хорошего кода отнимает много времени, но все же надеюсь, что найдутся доброжелатели, которые укажут мне на мои ошибки и повысят качество моего кода.
Давно задумывался, уместен ли данный вопрос на данном ресурсе, прочитав вопрос все-таки решился опубликовать.
Некая система, которую я пишу сейчас.
Небольшое тестовое задание, которое я недавно делал.
  • Вопрос задан
  • 2729 просмотров
Пригласить эксперта
Ответы на вопрос 9
Vas3K
@Vas3K
Помню несколько лет назад ко мне тоже обратился, назовем его Геннадий, который «набирал крутых фрилансеров в новую крутую студию» и, видимо чтобы я доказал свою «крутость», выслал задание (больше похожее на реальное ТЗ), по которому надо было реализовать полноценный справочник по недвижимости со всем функционалом, в письме приложив ссылку на полугигабайнтый дамп не то CSV, не то SQL базы. Притом я должен был написать его сразу и за 12 часов, каждые 3 часа отчитываясь ему лично о проделанной работе.

Когда Геннадий получил отказ, он очень сильно обиделся, обозвав меня всякими словами, типа «некомпетентный», «ленивый», «упускающий свой шанс» и.т.д. на чем мы и распрощались.

Просто забейте и больше никогда не ведитесь на «тестовые задания» больше, чем в 100 строк, а тем более включающие в себя разработку готового продукта. Через пару лет будете вот так же рассказывать потомкам :)

Хотите проверить мой скилл как программиста? 100-200 строк хорошего задания вполне хватит для этого.
Хотите просто посмотреть на качество моего кода и умения как проектировщика больших систем? Добро пожаловать на мой github, где есть пара моих проектов, либо попросите «набросать на бумажке» что-то еще.

Почему-то на любом собеседовании в любую крупную компанию спрашивают либо накидать простейшую реализацию, либо дают на дом написать небольшой кусочек, а всем этим проходимцам-Геннадиям так сразу подавай «Интернет-магазин» или «Справочник недвижимости» с полным функционалом.
Ответ написан
Vir
@Vir
Программист
if( defined('DEBUG') && DEBUG )

    $logger->printrLog();

Вот так, точно делать не надо. Очень прошу.
Ответ написан
sdevalex
@sdevalex
1. Глобальные переменные — это плохо… о том, почему плохо написано куча всего.
$db     = Db::initDb($dbConf['host'], $dbConf['user'], $dbConf['pass'], $dbConf['db'] );
$db->query('SET NAMES UTF8');
$page   = UserPage::initPage();
$logger = Logger::initLogger();
$widget = WidgetLoader::initWidgetLoader();
$tpl    = Template::initTemplate(TEMPLATES_PATH);


2. Комментарии к методам класса пиши возле метода, а не в описании класса.
Ответ написан
ilyaplot
@ilyaplot
PHP программист
implode как раз для удаления костылей типа $sql = substr($sql, 0, strlen($sql) — 2);
Захабрил Вас :) Раньше у меня код был точно таким же
Ответ написан
wartur
@wartur
— Пользуйтесь константой DIRECTORY_SEPARATOR.

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

— Db::initDb — а зачем в этом случае пользоваться фабрикой??? Мы же объект создаем, то есть должно быть new DB(); чисто по идее ООП. И кстати, если нет совершенно особой функциональности у ваших классов, то не надо вообще оборачивать стандартные функции. У меня например класс базы данных абстрагирует физические подключения к СУБД, и переключения между БД в одной СУБД. К нему обращаются разные объекты указывая лишь название «шлюза» (я так это назвал), а класс уже гарантирует что запрос будет произведен по нужной СУБД + БД. Еще раз в вашем случае не вижу смысла использовать фабрику (у меня испольуется фабрика, что бы просто создавать объекты с вшитым «шлюзом»).

Советую делать так вначале инициализации:
// директория с системой
define('DIR_ROOT', __DIR__.DIRECTORY_SEPARATOR); // корень ядра.
// директория с кодом движка
define('DIR_CORE', DIR_ROOT.'core'.DIRECTORY_SEPARATOR);
// директория с интерфейсами движка
define('DIR_CORE_INTERFACES', DIR_CORE.'interfaces'.DIRECTORY_SEPARATOR);

========

я могу тут целый пост по моей культуре производства писать, но пожалейте меня ;-) надеюсь помог.

// директория с исключениями движка
define('DIR_CORE_EXCEPTIONS', DIR_CORE.'exceptions'.DIRECTORY_SEPARATOR);

// директория с абстрактными модулями движка
define('DIR_CORE_ABSTRACT', DIR_CORE.'abstract-modules'.DIRECTORY_SEPARATOR);
Ответ написан
Aco
@Aco
Заклинатель кода
Код кишит мёртвым кодом, на подобии
 throw new Exception('Template file '.$templateLink.' not found!');
if( defined('DEBUG') && DEBUG )
  $this->html = 'Template file '.$templateLink.' not found!';
else
  $this->html = '';
return false;

После исключения код никогда не выполнится
Ответ написан
Комментировать
ilyaplot
@ilyaplot
PHP программист
Комментируйте код. Пусть даже для себя. Поможет в будущем.
Ответ написан
По заданию:
У класса ApacheLogAnalyzer слишком много ответственностей и жёстких связей:
— читает данные из лога (пускай через ApacheLogReader, но всё равно);
— анализирует их;
— пишет их в БД (пускай через MysqlLogDataWriter, но всё равно)

При этом нет никакой явной необходимости инстанцировать ApacheLogReader и MysqlLogDataWriter в ApacheLogAnalyzer. Так же не вижу необходимости инстанцировать mysqli в MysqlLogDataWriter, да и вообще лучше PDO, как указали выше.

Можно сделать без сильного рефакторинга что-то вроде
$reader = new ApacheLogReader('access.log'); 
$db = new PDO('mysql:dbname=fl;host=localhost', 'fl', 'fl'); // @todo Обработка ошибок
$writer = new PDOLogDataWriter($db); 
$analyzer = new LogAnalyzer($reader, $writer);
...

Так мы получим более гибкую и легче тестируемую архитектуру практически без накладных расходов и больших изменений. А учитывая специфику (методы ридера и райтера вызываются только в одном месте анализатора), можно сделать
$reader = new ApacheLogReader('access.log'); 
$log_elems = $reader->getLogElems();
$analyzer = new LogAnalyzer($log_elems);
$data = $analyzer->getAnalyzedData();
$db = new PDO('mysql:dbname=fl;host=localhost', 'fl', 'fl'); // @todo Обработка ошибок
$writer = new PDOLogDataWriter($db); 
$writer->write($data);

вообще избавившись от этих зависимостей в анализаторе и сделав его ещё легче тестируемым, но тоже без глобальных изменений

Можно пойти ещё дальше, реализовать кучу интерфейсов и паттернов, и сделать анализатор вообще не зависящий ни от источника данных и их формата, ни от места их записи и формата, ни, даже, от метода анализа, но это уже будет, наверное, «ООП головного мозга» и преждевременное абстрагирование :)
Ответ написан
По поводу тестового задания. Я так думаю, что надо было проанализировать лог и засунуть его в базу.

Вижу проблемы с алгоритмом: считать всё, записать всё(ещё и в один запрос). Сразу пара проблем: при большом логе не будет хватать, при повторном анализе все данные вновь вставятся в лог.
Ответ написан
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Похожие вопросы