Repository navigation
Conversation
EvgrafovMichail
left a comment
EvgrafovMichail
left a comment
There was a problem hiding this comment.
Хорошее и интересное решение. Отдельный плюс - наличие юнит тестов.
Очень обидно снижать вам балл за копипасту
|
|
||
|
|
||
| # ============================================================ | ||
| # Helpers |
There was a problem hiding this comment.
Хелперы для подготовки данных для тестов в pytest обычно оформляются с помощью фикстур:
https://docs.pytest.org/en/stable/explanation/fixtures.html
| @pytest.mark.parametrize( | ||
| ("raw", "expected"), | ||
| [ | ||
| ("12.999", "13.00"), |
There was a problem hiding this comment.
Для оформления тесткейсов лучше использовать pytest.param. Так вы сможете явно сгруппировать данные для тесткейса и связать с ними читаемый ID:
https://docs.pytest.org/en/stable/example/parametrize.html#set-marks-or-test-id-for-individual-parametrized-test
|
|
||
|
|
||
| class TestCreateProduct: | ||
| def test_success_adds_normalized_product(self) -> None: |
There was a problem hiding this comment.
Старайтесь придерживаться паттерна AAA во всех тестах.
|
|
||
|
|
||
| def dict_to_cart_line(line: dict[int, Any]) -> CartLine: | ||
| return tuple(x[1] for x in sorted(line.items())) |
There was a problem hiding this comment.
Забавно, что mypy на это не ругается. Формально тип выходного значения tuple[Any, ...], т.к. тип значений словаря - Any, а dict[int, Any] не дает гарантий, что в словаре будет два объекта. Но т.к. функция служебная, не буду придираться.
А еще ее можно было реализовать без явного индексирования:
| return tuple(x[1] for x in sorted(line.items())) | |
| return tuple(val for _, val in sorted(line.items())) |
| LINE_QUANTITY_INDEX: Final = 1 | ||
|
|
||
|
|
||
| def dict_to_cart_line(line: dict[int, Any]) -> CartLine: |
There was a problem hiding this comment.
Если функция служебная, можно было явно на это указать, назвав ее так:
| def dict_to_cart_line(line: dict[int, Any]) -> CartLine: | |
| def _dict_to_cart_line(line: dict[int, Any]) -> CartLine: |
|
|
||
|
|
||
| def find_cart_line(cart: list[CartLine], product_id: int) -> CartLine | None: | ||
| line = [x for x in cart if x[LINE_PRODUCT_ID_INDEX] == product_id] |
There was a problem hiding this comment.
Стоило ли использовать для реализации этой функции списковое включение? Так вы гарантированно обойдете все элементы корзины. При использовании явного итерирования в цикле for вы можете выйти из цикла сразу после обнаружения нужной позиции и сэкономить на этом ресурсы.
| from .utils import normalize_price | ||
|
|
||
|
|
||
| def dict_to_product(record: dict[int, Any]) -> Product: |
There was a problem hiding this comment.
Это один в один дублирование функции dict_to_cart_line. Можно было определить одну служебную функцию в utils.py: convert_dict_to_tuple. Тело функции было бы таким же, а тип возвращаемого значения: tuple[Any, ...]. А вызывающая сторона сама бы выводила нужный тип с помощью функции typing.cast
| storage, product_id, (product[NAME_INDEX], product[PRICE_INDEX], new_quantity) | ||
| ) | ||
|
|
||
| dict_line = { |
There was a problem hiding this comment.
Я бы унес создание словаря в функцию dict_to_cart_line. А dict_to_cart_line переименовал бы в create_cart_line. Тогда эта функция выглядела бы так:
def create_cart_line(
product_id: int,
quantity: int,
) -> CartLine:
cart_line_dict = {
LINE_PRODUCT_ID_INDEX: product_id,
LINE_QUANTITY_INDEX: quantity,
}
return tuple(val for _, val in sorted(cart_line_dict.items()))За счет этого вы бы избежали дублирования кода создания словаря, и логику создания позиции в корзине инкапсулировали бы полностью. Одни плюсы. А так, вынужден снизить вам балл за копипасту и нарушение критерия S1
| if fields[0] in [x[NAME_INDEX] for x in storage]: | ||
| print(f"product name {fields[0]} is already taken") | ||
| return None | ||
| dict_record = { |
| """ | ||
| # TODO: реализуйте функцию | ||
| return (0, "", Decimal(0), 0) | ||
| record = [x for x in storage if x[PRODUCT_ID_INDEX] == product_id] |
No description provided.