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