Skip to content

Instantly share code, notes, and snippets.

@Rhincodon
Created March 19, 2020 21:05
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save Rhincodon/ba1c9b072df910b57e09f3e72385b5cc to your computer and use it in GitHub Desktop.
Save Rhincodon/ba1c9b072df910b57e09f3e72385b5cc to your computer and use it in GitHub Desktop.
t
по ui:
1. выпадашка с Sign Out должна закрываться и при клике вне своей области, не только при клике на аватар
2. Модальное Add Site не закрывает ни по Esc (хотя есть tip про Esc), ни по клику вне модальном, только по крестику.
3. данные в колонке LAST CHECK никак не обновляются при check/check uptime. Кнопки действий должны игнорировать run_interval_in_minutes и любые другие ограничения, т.е запускать проверку в force режиме, иначе в них нет смысла
4. уведомления которые вылазят при нажатии check кнопок работают только если залогинится и потом обновить страницу. Т.е сразу после логина если кликнуть на них то не показываются уведомления
5. если находясь на второй странице удалить сайт то будет такой артефакт https://monosnap.com/direct/ciPXYKA9VMn921QulgzIul9OBaU2tF . Тоже самое будет если на этой странице добавить сайт. "Showing 10 to 20 of 13 results" — должно быть "Showing 10 to 13 of 13 results"
6. интерфейс не отзывчивый — нет ни одного лоадера, при клике на Check непонятно кликнулась она или нет, т.к интерфейс никак не реагирует; кнопки должны дизеблится при клике пока не прийдет ответ.
7. Использование сокетов это стильно и молодёжно, но в пределах задачи они добавляют сложности начиная с кода, заканчивая инфраструктурой, соответственно в них особо нет смысла.
по коду:
1. есть DTO, но:
а) у них странный нейминг, т.к по названию не понятно что содержит объект и не всегда это существительное. Т.е например AuthenticateDTO содержит $email и $password, он должен называться Credentails или AuthenticationCredentials или UserCredentials. Тоже с RegisterDTO.
б) не совсем понятно почему Form Request отвечают за создание DTO и в них есть метод getDTO(). Смысл DTO чтобы скрыть от сервисов Http слой и передавать в них DTO, но получается что сами DTO создаются в Http слое. DTO должен или сам себя создавать или должен быть DTOFactory
в) DTO почему-то лежат в папке с сервисами + ещё один DTO в jobs
г) можно было использовать https://github.com/spatie/data-transfer-object , но можно было бы вобще без DTO обойтись учитывая сложность задачи
2. MonitorController: как-то выборочно использован MonitorService, для index/store он используется, а для destroy/check уже почему-то нет. Получается частичная привязка бизнес-логики к Http
3. Реизобретена пагинация, хотя в laravel есть дефолтная пагинация со всем необходимым с поддержкой Resources
4. Не совсем ясно зачем дефолтный AuthenticatesUsers::login был заменён на кастомный, в кастомном как-то странно сервис отвечает только за половину аутентификации, за вторую часть отвечает уже контроллер. Ну и в дефолтном есть поддержка guards, remember, attempts.
5. Можно было не использовать Resources (User/Monitor/PaginatedMonitors), т.к они нужны только если нам нужно скрыть/модифицировать что-то в ответе. Но в приложении нет таких требований (и судя по содержимому ресурсов там особо ничего и не модифицируется, непонятная строка (string)$this->url — разве url итак не всегда строка?)), можно было соответственно отдавать модели через eloquent без доп. кастомизаций
6. В целом структура проекта необосновано усложнена (сокеты, dto в странном виде, нет чёткого разделения ответственности), причем без четкой архитектуры, есть только частичные намёки на DDD.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment