-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Next coding dojo solution #3
base: master
Are you sure you want to change the base?
Next coding dojo solution #3
Conversation
|
||
RUN composer install --no-interaction | ||
|
||
COPY . . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't a good idea:
- Without .dockerignore you're copy anything from context directory, with
.env
or temporary builded files. COPY . .
triggers on each changing in context directory - It could be problem with cacheing layer.
public function __invoke(BuyItemRequest $request): ConsoleResponse | ||
{ | ||
$item = $request->item(); | ||
$moneys = $request->moneyCollection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
money
is a plural form. So you should rename to money_collection
public function __construct( | ||
private ItemRepository $itemRepository, | ||
private PaymentCoordinator $paymentCoordinator, | ||
private ConsoleResponse $response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should have Response
or IResponse
class, ConsoleResponse
tells about specified implementation.
if ($availableItem->enoughToBuy($moneys->count())) { | ||
$this->response->setProduct($availableItem); | ||
|
||
return $this->response->setRest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, you should return a new response, not generate response / part of response from response.
my propositions:
return $this->responseFactory->createRestResponse($availableItem, ...);
or
return new RestResponse($availableItem, ...);
# The controller of commands should get response and use it.
Disadvantage of your solution is to use N methods to fill a response. What if you add next method like setDiscount
and forgot to update this command?
{ | ||
public function pay(int $input, int $cost): MoneyCollection | ||
{ | ||
if ($input >= $cost) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer IfError pattern. Is more easy to write method with validating as first.
); | ||
|
||
self::assertEquals(0, $result->rest()->count()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wrote only happy path cases. Where are cases to support errors like not enough money?
$availableItem = $this->itemRepository->getItemBySelector($item); | ||
|
||
if (!$availableItem) { | ||
throw new \InvalidArgumentException(sprintf('Item %s is not available', $item)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinking about this exception - exceptions should be for unexpected situations (like disconnect, file not found, index larger than size of array etc) and not availabled item is an expected situation by business flow so I think it should be as:
return new ItemNotAvailableResponse($item);
PS. As functional programmer, I don't like exceptions ; -)
|
||
$result = $command(new CoinReturnRequest($moneyCollection)); | ||
|
||
self::assertEquals('D, DOLLAR', $result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not return array?
{ | ||
$request = $this->parser->parse('Q, Q, Q, Q, GET-B'); | ||
|
||
self::assertInstanceOf(ConsoleRequest::class, $request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not a enough to check class - What is in request inside?
|
||
public function setUp(): void | ||
{ | ||
$vm = new VendingMachine(); | ||
$this->vendingMachine = $vm; | ||
$itemRepositoryMock = $this->getMockBuilder(ItemRepository::class)->getMock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice one.
No description provided.