Здравствуйте, Tourist, Вы писали:
T>Ну, не знаю. Самое просто, а почему для money у вас BigDecimal?
Оно вообще то всю жизнь было рекомендованным решением по умолчанию для денег. Сейчас что, появились новые рекомендации и я что то пропустил?
..
SVZ>Ну т.е. ты предложил второй стороне разбираться самим, какие соглашения ты выбрал и угадывать, почему именно так? SVZ>Я бы такое не оценил. SVZ>"common sense" он у всех свой Вот они и оценили соответственно.
ну это не была q&a сессия и ее наличие как-бы не предполагалось
обычно она случается уже после, по результатам теста
A>>2/ в реальном мире она нигде не атомарна и не может быть таковой S>ACID
ну в реальном мире с реальными требованиями все сложнее чем аббревиатуры. теоретически можно в контексте _этого_ задания сделать atomic move,
но я решил, что достаточно atomic exchange (add). конкретно здесь я не вижу необходимости и потенциальных race conditions тоже — основные операции атомарны, а
сам wallet введен лишь как иллюстрация возможного BL
ну то есть за бабки в пилот я бы написал совсем иначе, но для тестового задания я выбрал такой путь и не очень понимаю почему он неверен
Здравствуйте, arth, Вы писали:
A>выложил на github, поправил стартовое сообщение
Может им не понравилось, что нет истории разработки? (Все одним коммитом.)
Вообще если дали тз на несколько часов, а потом леняться дать вменяемый ответ что для них плохо, то с чистой совестью можете раскрывать название конторы.
Здравствуйте, arth, Вы писали:
GIV>>Как то смущает рукопашный разбор параметров. Почему не https://dinject.io/javalin ? A>почему рукопашный? тот же javalin вроде.
Я про @Post вместо
val to = ctx.queryParam("to") ?: throw IllegalArgumentException("@to is required.")
GIV>>Ну и для датастора я бы ожидал какую-нибудь ин мемори бд. A>зачем? чтобы просто показать, что я умею затащить в апп как депенденси и сделать таблицу из 2х полей?
The solution is no concurrency safe — transaction is not atomic
me — there is no explicit requirement and actually basic
transaction (add) is atomic. There is no race conditions. Atomicity of
_transfer_ is required if we don't want to loose data — but here there
is no such problem since we don;t store anything between sessions.
Insufficient testing coverage
nothing to say. It's 6hrs test. Not 40hrs
If we transfer negative amount money, author don’t throw exceptions, it takes abs value of this amount and transfer positive amount of money.
transferring negative amount is something which is odd. So it's a
convention. It either has to be documented or exception should be
thrown. So I partially agree.
Author don't know how to catch exceptions in junit
minor. Nothing to add.
Procedure of transferring money is not clear and difficult to support
no comment.
Poor documentation — there is no specifications of endpoints, how to build a project
KL — there is a README. And there is a build.gradle. It's kind of
misunderstanding what we expect from 6hrs test.
Здравствуйте, Skorodum, Вы писали:
S>Здравствуйте, arth, Вы писали:
A>>выложил на github, поправил стартовое сообщение S>Может им не понравилось, что нет истории разработки? (Все одним коммитом.)
это другой репо. для них я открывал и там несколько коммитов с моим настоящим именем
S>Вообще если дали тз на несколько часов, а потом леняться дать вменяемый ответ что для них плохо, то с чистой совестью можете раскрывать название конторы.
ответ дали после того как я их попросил/он ниже в ветке
Здравствуйте, arth, Вы писали:
A>The solution is no concurrency safe — transaction is not atomic A> me — there is no explicit requirement and actually basic A>transaction (add) is atomic. There is no race conditions. Atomicity of A>_transfer_ is required if we don't want to loose data — but here there A>is no such problem since we don;t store anything between sessions.
вообще то было в требованиях, конкурентное исполнение.
A>Insufficient testing coverage A>nothing to say. It's 6hrs test. Not 40hrs
Здравствуйте, AndyCyp, Вы писали:
AC>Здравствуйте, arth, Вы писали:
A>>The solution is no concurrency safe — transaction is not atomic A>> me — there is no explicit requirement and actually basic A>>transaction (add) is atomic. There is no race conditions. Atomicity of A>>_transfer_ is required if we don't want to loose data — but here there A>>is no such problem since we don;t store anything between sessions.
AC>вообще то было в требованиях, конкурентное исполнение.
atomicity и конкурентное исполнение не одно и то же/и мой код concurrent safe
A>>Insufficient testing coverage A>>nothing to say. It's 6hrs test. Not 40hrs
AC>тоже было в требованиях.
Здравствуйте, arth, Вы писали:
A>The solution is no concurrency safe — transaction is not atomic A> me — there is no explicit requirement and actually basic A>transaction (add) is atomic. There is no race conditions. Atomicity of A>_transfer_ is required if we don't want to loose data — but here there A>is no such problem since we don;t store anything between sessions.
Здесь они в принципе правы. Для тестовых заданий я делаю базовую атомарность, тупо ставлю synchronized на все методы бизнес логики, чтоб не заморачиваться и оно работало. Оптимизировать влом и все эти оптимизации потом. Даже если там атомарность есть фактическая — совсем не очевидно что она есть, лучше явно по умолчанию поставить лок, да и все.
A>Procedure of transferring money is not clear and difficult to support A>no comment.
Здесь они тоже правы. Там логика то простейшая, но навернул ты сильно больше, чем можно было бы сделать. Лучше б простой императивщиной в лоб бы сделал да и все. Я про метод add, а не про transfer. Такое впечатление что начитался про паттерн матчинг и захотел непременно применить что то похожее. Да и transfer тоже — amount.abs() и доп логика.
То есть код можно было сделать одновременно и более простым, и более надежным. Плюс более компактным. Еще я б для тестового задания не стал бы выделять интерфейс. Принципиально не стал бы, если бы стали придираться — аргументировал бы тем, что мне не нужна дополнительная сложность и дополнительный код на ровном месте. При необходимости рефакторинг вида extract interface делается мгновенно, и если у интерфейса только одна реализация, то лучше не выпендриваться и не городить доп сложность на ровном месте.
Но, только бы за эти недочеты я бы браковать не стал.
... E>Здесь они в принципе правы. Для тестовых заданий я делаю базовую атомарность, тупо ставлю synchronized на все методы бизнес логики, чтоб не заморачиваться и оно работало. Оптимизировать влом и все эти оптимизации потом. Даже если там атомарность есть фактическая — совсем не очевидно что она есть, лучше явно по умолчанию поставить лок, да и все.
ну а на что им мозг? чтобы смотреть и разбираться/атомарность там есть в нужном объеме
A>>Procedure of transferring money is not clear and difficult to support A>>no comment. E>Здесь они тоже правы. Там логика то простейшая, но навернул ты сильно больше, чем можно было бы сделать. Лучше б простой императивщиной в лоб бы сделал да и все. Я про метод add, а не про transfer. Такое впечатление что начитался про паттерн матчинг и захотел непременно применить что то похожее. Да и transfer тоже — amount.abs() и доп логика.
ну, мне удобно паттерн матчинг, я писал на котлине прод, это не для выпендрежа
ну и интерфейс concurrenthashmap прост и понятен вроде. зачем локи когда есть просто merge?
E>То есть код можно было сделать одновременно и более простым, и более надежным. Плюс более компактным. Еще я б для тестового задания не стал бы выделять интерфейс. Принципиально не стал бы, если бы стали придираться — аргументировал бы тем, что мне не нужна дополнительная сложность и дополнительный код на ровном месте. При необходимости рефакторинг вида extract interface делается мгновенно, и если у интерфейса только одна реализация, то лучше не выпендриваться и не городить доп сложность на ровном месте.
ну, зачем на котлине писать в императивном стиле, подумал я/
E>Но, только бы за эти недочеты я бы браковать не стал.
Ну давайте по порядку. Это то, что я на автомате проверяю на ревью, без определенного порядка.
Как уже правильно сказали, в 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 минут". А это вещи, которые как раз делаются автоматически.
Нет, у вас конечно не плохо. Откровенно плохого там ничего нет. Но и ничего выдающегося тоже нет. Видимо, у компании были более интересные кандидаты.
Здравствуйте, arth, Вы писали:
A>Попросили сделать тестовое задание часов за 6-12 (всего на задание давали примерно неделю): A>Design and implement a RESTful API (including data model and the backing implementation) A>for money transfers between accounts.
A>Я написал и им не понравилось. Не оч понимаю, чего они хотели и где я налажал?
Здравствуйте, arth, Вы писали:
A>Попросили сделать тестовое задание часов за 6-12 (всего на задание давали примерно неделю): A>Design and implement a RESTful API (including data model and the backing implementation) A>for money transfers between accounts.
Не смотря на то, что делать домашнее задание в 2019 году это полная дикость, все же глянул одним глазом.
Не понравилась модель предметной области.
Если с Account, который id + amount еще как-то можно смириться в тестовом задании, то кто такой Wallet?
Его интерфейс и реализация никак не следуют из названия.
Это что-то более похожее на Transaction чем на Wallet.
Зачем вообще нужен add, когда у вас в задании "money transfers between accounts"?
Почему пополнение счета не сделать как transfer(from='Cash/Wire', to=account)?
Здравствуйте, 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>Нет, у вас конечно не плохо. Откровенно плохого там ничего нет. Но и ничего выдающегося тоже нет. Видимо, у компании были более интересные кандидаты.
Здравствуйте, arth, Вы писали:
A>ну а на что им мозг? чтобы смотреть и разбираться/атомарность там есть в нужном объеме
Вообще то основное правило — при чтении кода мозг напрягаться не должен . Исключение — когда это высоко оптимизированный код, тут приходится. Соответственно решение по умолчанию — это минимальное количество кода, максимальная линейность кода, минимальный уровень вложенности. Применив merge ты возможно увеличил производительность по сравнению с решением в лоб, но понизил читаемость и поддерживаемость кода. Кстати, если уж начал выпендриваться с не совсем очевидной атомарностью — тест бы на параллельную попытку изменить счет бы не помешал. Накосячить на вот таких неблокирующих операциях на практике очень и очень легко, соответственно если решил так повыпендриваться, то тест нужно написать по любому. Если бы сделал через synchronized в лоб, причем synchronized был бы на все методы, то тут можно было бы и схалявить, ибо скорее всего ты бы не ошибся, но на более продвинутых решениях тестировать все таки надо.
У меня как то был случай, когда я увидел не неатомарность в чужом коде. Было запутанно, я предположил что автор проверил нормально, тем более он там был спец по алгоритмам, в олимпиадах участвовал и т.д. Так вот, он блин все таки ошибся и его творчество пришлось мне переписывать после того как он уволился. А шишки полетели на меня, исправлять ошибки в авральном режиме пришлось мне, не смотря на то, что код мне достался в наследство.
Здравствуйте, elmal, Вы писали:
E>Здравствуйте, Tourist, Вы писали:
T>>Ну, не знаю. Самое просто, а почему для money у вас BigDecimal? E>Оно вообще то всю жизнь было рекомендованным решением по умолчанию для денег. Сейчас что, появились новые рекомендации и я что то пропустил?
Интернет банкингом вы наверно пользуетесь? Как думаете, возможно ли там сделать операцию и перевести 0.005 рубля?
В Java есть Money API, правдо оно ни как не дойдет до включения в JDK. Можно посмотреть его как на пример дизайна.
А так, как минимум у любого счета есть тип валюты этого счета. Что тоже минус в вашей модели.
Вот они и оценили соответственно.
