Что неправильного в моем коде?

Question

[Valentine5]

Здравствуйте!

Да днях написал тестовое. Вот, что получилось: https://drive.google.com/drive/folders/1mqeDM1vxQT...
Не дописал, попросил еще немного времени. Мне его не дали, сказали, что в целом код недостаточно профессионально написан. Подскажите, пожалуйста, что я не так делаю?


Задача была: загружать через форму файл json со списком команд, а потом сводить эти команды в турнируную таблицу с условиями (гость, хозяин), круг 1, круг 2 и еще 5-6 условий, которые влияют на список. Сколько раз должны встречаться между собой.

Файл json содержит следующие данные:

{
"teams": [
{ "id": 1, "title": "Ливерпуль" },
{ "id": 2, "title": "Челси" },
{ "id": 3, "title": "Тоттенхэм Хотспур" },
{ "id": 4, "title": "Арсенал" },
{ "id": 5, "title": "Манчестер Юнайтед" },
{ "id": 6, "title": "Эвертон" },
{ "id": 7, "title": "Лестер Сити" },
{ "id": 8, "title": "Вест Хэм Юнайтед" },
{ "id": 9, "title": "Уотфорд" },
{ "id": 10, "title": "Борнмут" },
{ "id": 11, "title": "Бернли" },
{ "id": 12, "title": "Саутгемптон" },
{ "id": 13, "title": "Брайтон энд Хоув Альбион" },
{ "id": 14, "title": "Норвич Сити" },
{ "id": 15, "title": "Шеффилд Юнайтед" },
{ "id": 16, "title": "Фулхэм" },
{ "id": 17, "title": "Сток Сити" },
{ "id": 18, "title": "Мидлсбро" },
{ "id": 19, "title": "Суонси Сити" },
{ "id": 20, "title": "Дерби Каунти" }
]
}

Comments

[kalapanga]

PHP-шный код я оценить не могу. Но если бы мне программа выдала как на скриншоте, что в первом круге Ливерпуль играет одновременно с тремя командами (в том числе и сам с собой!), я бы наверное тоже дальше смотреть не стал.
Возможно, не понравилось именно то, что вариант ещё слишком сырой.

Answer 1

[Сергей delphinpro]

Вы получили простое задание, но это не значит, что код нужно написать абы как, лишь бы результат был =). Элементарные вещи стоило бы использовать,как то: автозагрузка классов, шаблонизация.

0. Код на облачном диске. Почему не гитхаб? Это же удобнее (и плюс в карму=).
1. Инклуд файлов, вместо автозагрузки классов. Написать автозагрузчик - дело пяти минут. Даже одна минута, если использовать composer.
2. Разделение мух и котлет. Код отдельно, html-разметка отдельно. Простейший php-шаблонизатор - это тоже пять-десять минут.
Вот так вообще нельзя писать: echo '<tr><th scope="col">Первый круг</th></tr>';
3. Разделение обязанностей. У вас один класс и файлы загружает, и читает загруженные, и турнирную таблицу рассчитывает и рендерит ее.
4. range(0, 19); Почему 20 элементов? Почему не 30? Откуда это число взялось? Его не должно быть в коде.
5.

$probablePairsArr[] = range(0, 19);
$probablePairsArr = $probablePairsArr[0];

Интересный финт. В чём его сакральный смысл - не ясно. Да и использование локальной переменной здесь не требуется. Можно сразу создавать массив в свойстве класса.

Наверное всё. Больше там ничего нет.

Comments

[Valentine5]

Сергей delphinpro А я думал, у меня разделен код и html (пункт 2 в вашем фидбэке). У меня в table только создается объект, который динамически формирует html. Ну и header c помощью php прикрепляется. А как правильно это сделать? index.php же все равно откроет table.php и html нужно в нем "рисовать" с помощью PHP, исходя из конкретных, переданных данных. Подскажите, пожалуйста, как это делать более профессионально...

[Ипатьев]

Valentine5, разделён - это когда СНАЧАЛА логика, а ПОТОМ html. А не как у вас - сначала HTML, потом прямо из него инклюдим логику, потом из логики выводим HTML, потом снова HTML.

Логика она на то и логика, что не должна ничего выводить сама. а только готовить сырые данные.
И только после того как данные подготвлены, они доджны выводиться.

А если этот скрипт должен будет возвращать json, а не html? При разделении логики и отображения, логику менять не придётся - только вывод. А у вас все перемешано

[Valentine5]

Сергей delphinpro А зачем непременно автозагрузчик, насколько я понял, он нужен для того, чтобы избежать большого количества include(), но в этом проекте данное пока не предвидится, поэтому зачем добавлять что-то избыточное. А если предположить, что у среды, где тестировать будут нет Composer?

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

[Сергей delphinpro]

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

Другое дело, если вы пишете небольшую утилитку для личного пользования. Вот там действительно любые навороты излишни.

[Valentine5]

Сергей delphinpro Огромное спасибо, что уделяете мне время, но если бы я добавил ту же автозагрузку, то кто-то бы сказал, что я принял не рациональное решение, потратил время на то, что пока не надо. Или чаще так не подумают?