Money validator #1
Conversation
bumblecoder
force-pushed
the
money-validator
branch
from
5ef4eeb to
928526a
Compare
bumblecoder
force-pushed
the
money-validator
branch
3 times, most recently
from
71ee489 to
6d23333
Compare
bumblecoder
force-pushed
the
money-validator
branch
from
6d23333 to
951a182
Compare
| FILTER_VALIDATE_INT, | ||
| ['options' => ['min_range' => 0, 'max_range' => 100]] | ||
| )) { | ||
| throw new InvalidArgumentException('Deviation must be between 0 and 100.'); |
There was a problem hiding this comment.
почему валидация здесь, а не в конструкторе?
There was a problem hiding this comment.
только из соображений того, что я ловлю эксепшн в контроллере чтобы избежать 500 и выдать контролируемый эксепшн. Было бы и правда логичнее делать проверку в конструкторе, но тогда будет 500 при инициализации объекта
| return false; | ||
| } | ||
|
|
||
| $deviationPercentage = (string) ($this->deviation / 100); |
There was a problem hiding this comment.
зачем нужен каст в строку - (string) ?
There was a problem hiding this comment.
хороший вопрос :) тут каст не нужен
| } | ||
|
|
||
| $deviationPercentage = (string) ($this->deviation / 100); | ||
| $deviationAmount = $transaction->getAmount()->multiply($deviationPercentage); |
There was a problem hiding this comment.
почему решил вычислять отклонение от суммы Транзакции, а не Реквеста? Имеет ли смысл вычислить отклонение в обе стороны - от Транзакции и от Реквеста?
There was a problem hiding this comment.
Сумма транзакции это, по сути, "истина", а запрос должен быть проверен, чтобы отклонения были в допустимых пределах относительно этой "истины". На счет отклонений в обе стороны - действительно это имеет смысл, если на стороне запроса была ошибка. Т.к. не было конкретных условий вычисления отклонений с обеих сторон, то оставил так
| @@ -123,5 +123,5 @@ | |||
| 'store' => env('APP_MAINTENANCE_STORE', 'database'), | |||
| ], | |||
|
|
|||
| 'transaction_deviation' => env('TRANSACTION_DEVIATION', 5), | |||
| 'transaction_deviation' => (int) env('TRANSACTION_DEVIATION', 5), | |||
There was a problem hiding this comment.
разве не может быть отклонение дробным числом - почему оно должно быть целым int?
There was a problem hiding this comment.
конечно может, использовал инт для упрощения решения, т.к. это тестовая задача
| throw new InvalidArgumentException('Deviation must be between 0 and 100.'); | ||
| } | ||
|
|
||
| if ($request->getCurrency() !== $transaction->getCurrency()) { |
There was a problem hiding this comment.
minor: есть Money::assertSameCurrency()
|
|
||
| class RequestMoneyValidatorTest extends TestCase | ||
| { | ||
| public function test_validate_currency_mismatch(): void |
There was a problem hiding this comment.
семантика, но по PSR-1 методы должны называться в camelCase формате - https://www.php-fig.org/psr/psr-1/[#1](#1)-overview. Код сниффер не пропустит такой нейминг
There was a problem hiding this comment.
вот тут я не знаю честно говоря почему так написал))тут прям самому смешно, может потому что делал на Ларавел. - нет ответа у меня. На Симфони всегда пишу по ПСР через camelCase
| $this->assertTrue($validator->validate($request, $transaction)); | ||
| } | ||
|
|
||
| public function test_validate_small_deviation(): void |
There was a problem hiding this comment.
в тестах рассмотрены кейзы для девиейшн 1%, 5% и 10%. А что если будет 0, null, 99, 100, ... ?? А что если в Транзакции или Реквесте будут отрицательные или пустые значения? Мало, мало тестов...
Еще мне кажется здесь очень бы подошел дата провадер: https://blog.martinhujer.cz/how-to-use-data-providers-in-phpunit/
There was a problem hiding this comment.
да, согласен, поскольку тестов и правда не много, то решил не использовать дата провайдер, хотя мысли об этом были
|
В целом - очень хорошо: есть DI, есть Money. Как по мне - хороший код, который если немного доработать, то вполне рабочий. P.S. Роутинг, контроллер и пайплайн - это конечно спасибо, но выходит за рамки задания, поэтому я это не валидировал :) |
No description provided.