Review

[aleks777777]

What about divide logic processing in other service with prepared payload? https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Services/PaymentService.php#L78

[aleks777777]

Usually you are use array or maybe collection? https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Repositories/Payments/PaymentsRepository.php#L13

[aleks777777]

https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Models/Payment.php#L127 What do you think about factory for payment?

[aleks777777]

https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Parsers/LoanNumberParser.php#L14 You have a parser class, but why you store regex in entity?

[aleks777777]

https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Commands/PaymentsImportCommand.php#L40 What do you think about a big file import and out of memory?

[aleks777777]

https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Commands/PaymentsImportCommand.php#L54 how often do you use an associative array? maybe you can use dto and factory?

[aleks777777]

Great if you use unit tests for services because it's main business logic.

[aleks777777]

Main point:

• Great clean code and structure!
• I expect little bit more structure and behavior patterns and the use of interfaces for a more scalability of functionality. (we can discuss about it on live our call)

[vladislavs-poznaks]

Aleks!

Thanks for taking the time and reviewing, will answer each topic shortly.

[vladislavs-poznaks]

What about divide logic processing in other service with prepared payload? https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Services/PaymentService.php#L78

Yes, this is a good point, I wanted to extract at least payment order creation to some other service as this has gotten quite crowded, but wanted first to take care of dependency injection, so I can switch to repository interfaces to be able to create fake repositories for unit testing the service.

Usually, I would probably reach for some sort of chain job and decompose it in this way, which also would enable persist payment with the initial status and process everything after returning the response, but this is more Laravel specific.

[vladislavs-poznaks]

Usually you are use array or maybe collection? https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Repositories/Payments/PaymentsRepository.php#L13

Yes, the collection would be a better return type as with that we could also ensure that it contains only payment models.

[vladislavs-poznaks]

https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Models/Payment.php#L127 What do you think about factory for payment?

There are two main things I do not like - one is creating the amount, and the other one is parsing the date. These could be separate variables to ensure that are passed correctly.

As with payment the already converted to int amount can be passed and Carbon's parse not always correctly handles the date string (I think it defaults to the current timestamp at some cases).

[vladislavs-poznaks]

https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Parsers/LoanNumberParser.php#L14 You have a parser class, but why you store regex in entity?

This actually was by design, I wanted to keep loan number's format encapsulated in the value obejct. In this way, for example, if the amount of numbers changes from 8 to have an option of 8 and 9, I can update the loan number object to account for that and add the relevant test cases. This way the possible format would update in all application.

[vladislavs-poznaks]

https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Commands/PaymentsImportCommand.php#L40 What do you think about a big file import and out of memory?

To be honest, I didn't think about such case, should probably get rid of that parsed array and process the csv line by line and on each handle persisting the payment.

[vladislavs-poznaks]

https://github.com/vladislavs-poznaks/sf-payments/blob/master/src/Commands/PaymentsImportCommand.php#L54 how often do you use an associative array? maybe you can use DTO and factory?

Well, the associative array is my go-to solution at first, but here it would be good to have some sort of DTO interface with defined getters of the data we care about, and then there could be 2 implementations - one for import, another for API. This way, if we have another source of getting the payment, by API call to 3rd party service for example we could have another implementation. In this case most probably we could accept this DTO interface in payment's factory method instead of the array.

[vladislavs-poznaks]

Great if you use unit tests for services because it's main business logic.

Got to test only value objects, and wanted to have dependency injection to be able to have fake repositories injected into the service when testing to control the returned data from repositories and also track method calls.

[vladislavs-poznaks]

Aleks!

Thanks for taking the time and reviewing, will answer each topic shortly.

Thanks again and looking forward!

[vladislavs-poznaks]

@aleks777777, hi!

I had some time to work a bit more on this project, so added quite a lot, created one humongous PR of all changes compared to previous version. If you're interested and will have any spare moment, feel free to check it out until our call tomorrow.

Here's the PR - #11