Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HW05. Магазин книг. Громов Павел #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PaGr0m
Copy link
Owner

@PaGr0m PaGr0m commented Mar 24, 2020

No description provided.

Copy link

@ottergottaott ottergottaott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Пожалуй, не очень хорошо, что многие сущности подвешены и ни с какими другими не связаны никаким отношением.
  • CustomerService и PayService не просто используются CustomerController (как локальная переменная или каким-то другим способом). Кажется, что CustomerController агрегирует их. Мб, тут даже лучше подойдет более сильная связь -- композиция. Аналогично для других сущностей.
  • Когда будете поправлять предыдущий пункт, учтите, что дублировать ассоциации и поля не стоит. На ассоциациях вполне можно писать имя поля, посмотрите примеры.
  • Не совсем понятно почему OAuthChecker агрегирует в себе все контроллеры. Из диаграммы можно предположить, что львиная часть логики Либо название не отражает суть, либо что-то напутано.
  • Почему-то у Вас не список желаемого (wishlist), а что-то про мытье (washlist)
  • Вы уверены, что можно добавить книгу в список желаемого конкретного пользователя, не зная о нем ничего? Я про то, что CustomerController никак не может в addBookToWashList узнать ничего о пользователе. Мб, все-таки есть либо отношения между контроллерами, либо как-то данные должны передаваться. Вообще, это замечание распространяется на всю работу: хорошо, что используете существующие паттерны, не очень хорошо, что из-за этого многие детали скрываются и все слишком упрощается.
  • Вы точно уверены, что хранить пользовательские пароли в виде строки -- хорошая идея?)
  • Мне непонятно как из OAuthChecker можно осуществить поиск по каталогу или по всей базе книг.
  • Только id недостаточно для добавления отзыва к книге (CatalogService)
  • Я не нашел PaymentMethod. Ок, предполагаю, что это какой-то enum: одного значения enum недостаточно, чтобы полностью описать метод оплаты. Например, при оплате банковской картой может потребоваться много информации.
  • Напоминаю, что должна быть возможность размещения администраторами редакторских отзывов. Одного status: Boolean для реализации этого точно недостаточно

@PaGr0m
Copy link
Owner Author

PaGr0m commented May 31, 2020

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

Мне показалось, что неудобно будет, если от каждой Entity проводить связь к контроллерам, сервиса, репозиториям. Поэтому оставил комментарий про это. Не знаю можно ли так вообще делать, но Юрий на практиках говорил, что иногда используют комментарии, чтобы указать какие-либо моменты


CustomerService и PayService не просто используются CustomerController (как локальная переменная или каким-то другим способом). Кажется, что CustomerController агрегирует их. Мб, тут даже лучше подойдет более сильная связь -- композиция. Аналогично для других сущностей.
Когда будете поправлять предыдущий пункт, учтите, что дублировать ассоциации и поля не стоит. На ассоциациях вполне можно писать имя поля, посмотрите примеры.

Всегда не понимал тонкую разницу между агрегацией и композицией, так как они очень похожи. Если я правильно понял (нагуглил), то это верно?

Агрегация это тип отношений когда один объект является частью другого. Агрегация образует слабую связь между объектами. Все зависимые классы инициализируются вне основного объекта.
Композиция это тип отношений при котором один объект может принадлежать только другому объекту и никому другому.


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

В общем, идея была такая, OAuth проверяет у запроса, легален ли он. То есть: когда совершается запрос, то вместе с ним передается token. И если token валидный, то запрос проходит, если нет, то "доступ запрещен". По итогу, это происходит между тем, когда посылается запрос и он приходит контроллеру. Поэтому ничего умнее как поставить между контроллерами и интерфейсом API я не придумал. По заданию сказано, что при входе пароль должен сверяться с тем паролем, который в базе. Вот и думал тут это делать. Кажется это абсолютно неверно?

Сейчас сделал так: есть специальный контроллер, который принимает данные, проверяет, может ли выполнить данный запрос (проверяет логин/пароль пользователя). А после передает выполнение одному из контроллеров.


Почему-то у Вас не список желаемого (wishlist), а что-то про мытье (washlist)

))))


Вы уверены, что можно добавить книгу в список желаемого конкретного пользователя, не зная о нем ничего? Я про то, что CustomerController никак не может в addBookToWashList узнать ничего о пользователе. Мб, все-таки есть либо отношения между контроллерами, либо как-то данные должны передаваться. Вообще, это замечание распространяется на всю работу: хорошо, что используете существующие паттерны, не очень хорошо, что из-за этого многие детали скрываются и все слишком упрощается.

Идея была в следующем: все необходимые данные приходят в запросе (как в REST), поэтому доставать пользователя из базы не было смысла, так как нам приходит его id и по id можно добавить в базу. Немного переделал, добавил к сервису агрегацию с репозиторием пользователей.


Вы точно уверены, что хранить пользовательские пароли в виде строки -- хорошая идея?)

Исправил как массив символов char [] - это же правильный выбор ?)


Только id недостаточно для добавления отзыва к книге (CatalogService)

Скорее всего Вы имели в виду BookService. Согласен, тут вообще не указал ничего адекватного :с


Я не нашел PaymentMethod. Ок, предполагаю, что это какой-то enum: одного значения enum недостаточно, чтобы полностью описать метод оплаты. Например, при оплате банковской картой может потребоваться много информации.

Добавил!


Напоминаю, что должна быть возможность размещения администраторами редакторских отзывов. Одного status: Boolean для реализации этого точно недостаточно

Добавил Enum для статуса и соответсвующие операции.

@ottergottaott
Copy link

CustomerService и PayService не просто используются CustomerController (как локальная переменная или каким-то другим способом). Кажется, что CustomerController агрегирует их. Мб, тут даже лучше подойдет более сильная связь -- композиция. Аналогично для других сущностей.
Когда будете поправлять предыдущий пункт, учтите, что дублировать ассоциации и поля не стоит. На ассоциациях вполне можно писать имя поля, посмотрите примеры.

Всегда не понимал тонкую разницу между агрегацией и композицией, так как они очень похожи. Если я правильно понял (нагуглил), то это верно?

Агрегация это тип отношений когда один объект является частью другого. Агрегация образует слабую связь между объектами. Все зависимые классы инициализируются вне основного объекта.
Композиция это тип отношений при котором один объект может принадлежать только другому объекту и никому другому.

Да, агрегация -- это отношение "содержит", композиция -- это отношение "состоит из".

Вы точно уверены, что хранить пользовательские пароли в виде строки -- хорошая идея?)

Исправил как массив символов char [] - это же правильный выбор ?)

Это скорее общее замечание про то, что не стоит хранить пароли в виде строк и лучше хранить их в захешированном виде.

Со всем остальным более менее согалсен, исправления засчитываю

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants