«phpClub» — архив тем ("тредов"), посвящённых изучению PHP и веб-технологий.
Аноним 2019/05/08 17:00:58  №1395825 1
iFrrf6wk.png (191, 512x512)
512x512
Написал решение к классической задаче с собесов про
> Есть продукты A, B, C, D, E, F, G, H, I, J, K, L, M. Каждый продукт стоит определенную сумму.
> Есть набор правил расчета итоговой суммы:
> 1. Если одновременно выбраны А и B, то их суммарная стоимость уменьшается на 10% (для каждой пары А и B)
> 2. Если одновременно выбраны D и E, то их суммарная стоимость уменьшается на 6% (для каждой пары D и E)
> ...
Хочу, чтобы меня похуесосили. Если вам несложно, конечно ^_^
bit-ly/2Wyh0iX
Аноним 2019/06/13 04:37:22  №1415606 2
Вроде я проверил практически все посты в прошлом треде, и ответил на все вопросы, так что если вы что-то спрашивали, зайдите в старый тред >>1380485 (OP) и проверьте. А если я пропустил, напомните о себе тут.

Заодно я бы хотел разобрать одну интересную задачу из прошлого треда. Есть такая известная задача на ООП про скидки, кто мои задачи про ООП решал, думаю, без проблем справится. Вот тут вот задача: >>1395825 а тут решение анона: https://github.com/nokitakaze/test-programming-task-am-items/

Думаю, прошло много времени, так что прокомментирую решение:

> Правило №4 "стоимость выбранного продукта уменьшается на 5%". Что такое "выбранный продукт"? Покупатель произвольно пальцем тыкнул в любой продукт, и у него цена уменьшилась?

Это товар A из правила.

> Если одновременно выбраны А
Вот он и "выбранный".

Вообще, переставлять правила плохая идея. Да, они могут быть не очень логичными, но в задаче требуется сделать именно как описано. Согласен, впрочем, что формулировка плохая.

https://github.com/nokitakaze/test-programming-task-am-items/blob/master/src/Item.php

Тут странный отступ в 4 пробела. Он не нужен.

> protected $_type;
Не надо использовать подчеркивание в именах свойств. Это было в PHP4.

> @var ItemType[]|ItemType[][]|null
> protected $_only_includes = null;

Вот такой зоопарк типов - это плохо. Тебе при работе с полем придется всегда делать проверку на тип данных, ее легко забыть, итд. Это будет плохой, запутанный код.

И вообще, тут ты имхо переусложнил код. Ты пытался сделать так, чтобы у правил было как можно больше общего кода, чтобы достаточно было передать для разных правил разные массивы, но это лишь привело к переусложнению логики. То есть, думаю, что из базового класса Rule можно вообще убрать эти поля exclude/inlcude.

Тебе не надо было в базовом классе делать поля exclude/include. Они не применимы ко всем видам скидок, и значит, им незачем быть в базовом классе. Также, не стоит полагаться на идею "класс-наследник должен переопределить этот массив", так как она никак не задокументирована и не проверяется. Если ты хочешь что-то требовать от класса-наследника, то используй абстрактные методы.

Также, не стоило объединять правила 1 и 4 в один класс. Так как там другая логика. Или если и объединять, то без потери читабельности кода.

Также, посмотри на объем кода в https://github.com/nokitakaze/test-programming-task-am-items/blob/master/src/Rules/TotalCostSelectedItemsRule.php и сравни с тем, как это правило описано в одном предложении в задаче. Этот код стоит хотя бы разбить на методы с понятными названиями, сейчас он вообще не читаем.

Ну например, один из вариантов - это сделать так:

// Сортируем товары по типу
foreach ($types as $type) {
$itemsByType[$typeName] = $this->pickItemsByType($items, $type);
}

// Определяем число групп
$itemsCount = array_map('count', $itemsByType);
$pairsCount = min($itemsCount);

// Проходимся по группам товаров и даем скидку за каждую
for ($i = 0; $i < $pairsCount; $i++) {
$group = $this->selectGroup($itemsByType, $i);
$this->addDiscountForPair($result, $pair);
}

Вот, основная логика кода занимает небольшой объем и гораздо читабельнее. У тебя же код плохо читабелен, а разделение на функции (calculateIteration) не помогает его понять.

Что касается формата ответа, то думаю, стоило сделать объект, который бы хранил: вид скидки, размер скидки, к чему применяется, за какие товары дана. Тогда мы можем в итоге вывести информацию о примененных скидках. Твой же класс RuleCalculationResponse этого не позволяет.

И еще, ты сделал метод Rule#calculate(), который принимает массив товаров и за один проход ищет все скидки. Но можно было еще сделать по-другому: он принимает массив товаров и возвращает первую найденную скидку. Соответственно, мы вызываем его в цикле, пока все скидки этого типа не найдутся. Плюс в том, что это упрощает код правила. Нам не нужно искать все скидки, достаточно найти одну. Для правила 1, например, код будет совсем простой:

function applyDiscount(array $items)
{
$group = [];
foreach ($this->types as $type) {
$item = $this->findItemByType($items, $type);
if (!$item) {
return;
}
$group[] = $item;
}

// Применить скидку к $group
return new Discount(....);
}

Хотя этот код менее эффективен, но он более читабелен. А если встроить в корзину функцию поиска по типу и оптимизировать ее, то может быть даже более эффективен, чем твой код.

https://github.com/nokitakaze/test-programming-task-am-items/blob/master/src/Rules/RuleGroup.php

В этом классе, наверно, логично было сделать метод вроде applyGroup() или как-то так. А то класс не сильно отличается от просто массива.

https://github.com/nokitakaze/test-programming-task-am-items/blob/master/src/Rules/RuleSet.php

Не очень понятно, зачем этот класс.


https://github.com/nokitakaze/test-programming-task-am-items/blob/master/test/CalculatorTest.php

Тест сложный. Его тяжело прочесть, и понять, а правильный он или нет? В случае падения - непонятно, что именно не так. Обычно тесты пишут для проверки отдельных требований. В твоем случае, можно сделать такие тесты, начиная с тестирования отдельных скидок:

- тест, что скидка на группу товаров применяется, когда она есть
- что скидка применяется несколько раз, если групп несколько
- что она не применяется, если товаров нет
- тесты на другие виды скидок ....
- тест на то, что 2 разных скидки могут применяться к одной корзине
- тест на то, что товар, участвующий в одной скидке, не используется в другой

То есть тестируем отдельные простые требования простыми тестами. То, что у тебя - сложновато выглядит, и это скорее приемочные, а не юнит-тесты. Я бы предпочел набор простых тестов.

И не надо в одном тесте тестировать разные объекты (testprimitives). Надо было сделать 2 отдельных теста.


Ответы: >>1415839
Аноним 2019/06/13 12:36:37  №1415839 3