Здравствуйте, maxkar, Вы писали:
M>Здравствуйте, arth, Вы писали:
A>>код — https://github.com/arthells/mtest
M>Ну давайте по порядку. Это то, что я на автомате проверяю на ревью, без определенного порядка.
M>
M> Как уже правильно сказали, в readme не написано, как собирать. Одна-две строчки много времени не займут, а для читающего приятно и не надо вспоминать, какая там у вас культура.
камон. я не для домохозяек туториал пишу.
M> У вас там в бинарниках трояна нет? Это я к тому, что бинарники в репозитории — это нехорошо. Я знаю, что "все так делают". Но это ведь показывает, как вы подходите к выбору инструмента. Если у вас команда везде использует gradle, почему она не устанавливает глобально wrapper и не хранит только настройки в repo? Было бы логично, и можно было бы с причинами в readme описать, почему так сделано. Аналогичный инструмент есть для maven. Ну а sbt это из коробки уже давным давно сам поддерживает.
это тестовое задание. как и что это показывает — от "ничего" вплоть до "все"
M> Тесты. Вы их с падающими assert проверяли? У меня вот подозрение, что после первого падения все остальное упадет с AddressAlreadyInUse. Что как-то нехорошо. Например потому, что после этого нужно исправлять первый упавший тест а не любой понравившийся.
проверял
M> Там же. Это вообще как fixture же можно написать. withServer(server => ...). И вот в withServer сделать правильный try/catch/finally.
ну если мы проверяем как я в тестовом задании, те бесплатно, пишу тесты, то может вы и правы
M> Не StorageTest а StorageImplTest. Вот представьте, у вас есть несколько реализаций Storage. Какую из них тестирует StorageTest?
вы серьезно?
M> Assert.true(false) настораживает. Я сильно за java world не слежу, но когда там сломали Assert.fail? (Маленькая хитрость — туда можно даже написать, что именно сломалось!)
M> А еще когда-то в Assert был assertEquals. И он старается показать, что же там было не equals в сообщении. Это достаточно часто полезно. Не нужно с отладчиком выяснять, какое же на самом деле значение вернулось. Очень часто по этому "неверному" значению можно сразу предположить, что сломалось и что нужно исправить.
M> Ваш restful тянет только на Level 0 (используется HTTP). Банальный RPC over HTTP. SOAP, кстати, тоже попадает в REST Level 0. Хотелось бы видеть хотя бы Level 1 — Entities & Verbs (т.е. PUT /accounts/abc, GET /accounts, ...).
кому хотелось и зачем?
M> Копипаста с ctx.queryParam хорошо выносится в отдельный метод. И меньше шансов, что имена параметров в query string и сообщении об ошибке не совпадут.
придирка
M> Ожидание ввода в main — решение спорное. Как оно поведет себя в каком нибудь Kubernetes? Там некому будет вводить порт. Вообще право на жизнь оно имеет, но такие сюрпризы должны описываться в readme.
надеюсь вы несерьезно, потому что тест не для запуска в Kubernetes
M> Там же — можно было бы и написать количество фактических аргументов в сообщении об ошибке. Это несложно, а кому-то может однажды оказаться полезно.
за деньги — легко
M> Интерфейсам Storage и Wallet документация (контракт) не помешали бы. Там как раз можно описать поведение при положительных/отрицательных значениях, возможные исключения и прочие интересные ситуации.
согласен частично
M> *Impl. Это вот у вас ровно одна реализация предполагается? Почему бы не InMemoryStorage, например?
а у вас? и почему если не одна, то не InMemoryCuncurrentHashSetStorageImpl? понятно куда клоню?
M> Я бы вообще эти реализации спрятал за Factory Method. Потому что тогда их можно по желанию менять, разбивать/переразбивать на разные классы и т.п. В конце концов, все использования идут только через реализованные интерфейсы.
нечего добавить. не вижу в 2019 году задачи явно показывать знание Factory Method
M> В StorageImpl вложенность кода глубокая. Можно было бы спрямить через if/throw и if/return.
а можно было и нет. дело вкуса
M> Там же. current + change можно было бы в переменную сохранить. Было бы видно, что это одно и то же.
опять же про вкус
M> В WalletImpl.transfer поведение на отрицательных amount немного неожиданное. Вы уверены, что другой разработчик, реализующий интерфейс Wallet, сделает подобное же поведение? Это к вопросу о контрактах (которые выглядят как документация).
тут согласен
M> Там же. С JdbcStorateImpl ваш WalletImpl будет неатомарен. Вот его за factory method (создающего заодно и storage) спрятать — и будет лучше.
он и так неатомарен. атомарен Storage*
M> Account интересен. Почему у вас конструктор по умолчанию создает счет с невалидным id?
а с каким валидным id он может его создавать? это издержки json сериализации, нужен дефолтный конструктор, и вопросвалидности отдельный и серьезный
M> А зачем model и core разделили? Тем более, что core зависит от model а не наоборот. Кстати, а зачем в "реальном проекте" разделение делать иначе? Какие фундаментальные причины этого требуют?
M>
чтобы model отдать клиенту, core ему не нужен (либо перенести ifaces из core в model, кто тоже стоит отдельного обсуждения)
M>Вот весь тот список — это "элементы стиля". Вещи, которые делаются вообще автоматически. Показывает, насколько разработчик механически следует шаблонам и насколько все же задумывается о причинах определенных технических решений.
большинсто про вкус, остальное minor по мне. нейминг — не технические решения, как и работа с Assert
M>Ссылаться на недостаток времени здесь смысла нет. За 6-12 часов все это делается (ни один из шагов много времени не добавляет) если вы раньше писали rest и задумывались обо всех мелочах. Если раньше не делали — да, хорошее упражнение. Но, вероятно, на другую позицию. В чем-то это похоже на найм в хороший ресторан. Вы приходите на кухню и вас просят приготовить обед за 45 минут. Вы сделали стейк с картошкой. Но картошку не почистили, мясо не посолили и никакого соуса не добавили. Потому что "что же вы хотели за 45 минут". А это вещи, которые как раз делаются автоматически.
какие-то не очень уместные на мой взгляд аналогии
M>Нет, у вас конечно не плохо. Откровенно плохого там ничего нет. Но и ничего выдающегося тоже нет. Видимо, у компании были более интересные кандидаты.
спасибо за отзыв энивей
