PR: Initial merge as requested by Royal Mail #5
Conversation
…store seems to be read from options appsettings and not request and forgot to update code
…ust pseudocode before). Also added some more tests for 100% coverage for the ones being tested.
|
Hi Ricardo. Can you explain what you mean by the tests being 'pseudo-code' before? When the instructions ask you to add suitable tests, we do assume that those will be working tests that pass. I also note you've fixed your strategy classes so that they do now work correctly, but had the tests been complete and working originally, you would presumably have spotted these issues immediately. Did you run out of time? To be clear, the two-hour thing is not intended to be a hard timebox, just an estimate of the rough amount of refactoring effort we'd expect to see. We have moved this exercise to a new repo and updated the instructions. I appreciate you worked from this repo with the old instructions, hopefully it'll be clearer in future. |
|
Hi Neil,
Since both projects (src + tests) were not runnable (csproj config + program.cs) I thought the goal of the exercise was to write the code and then have another interview to go through it so I never actually ran them. A bad assumption from my side. Indeed, after getting the tests running, the strategy tests didn't pass as the implementation logic was wrong ('&&' instead of '||') which I then fixed. They would have indeed been flagged immediately as they have after running them.
I did use the whole 2 hours. I could have gone for full coverage, and it would have taken me maybe another 10-15min in that case. The 2 hours were enough for the whole exercise though. |
|
Hi Neil, I just realised that I didn't update the request object (MakeMailTransferRequest) with the value object (MailContainerNumber). Is it ok if I push a quick change? (won't affect tests) |
OK, that makes more sense. It sounds like the instructions need to be clearer. We would discuss your code in the next interview stage, certainly, but the test exercise is designed to stand alone. If you can finish up your pending change, I can look at the final code, and then we can close the loop on this. |
…) solves a few things, like SRP and DRY as the object itself is now responsible for the preconditions validation (ie: null checks)
|
This is now done/complete. |
…own value object and encapsulated its state (SRP)
|
Ended up doing 1 more push as I noticed the strategies were missing checking the NumberOfMailItems and Capacity validation and noticed as well that state was not properly encapsulated. Will understand if this commit is not considered as this was done later than the stipulated exercise time and you might have already spent time reviewing the code / tests. Unfortunately, this required changes to tests code. |
|
That's fine, Ricardo. I think AMS will have been in touch about setting up a final interview - please work with them and we'll get it booked in. |
No description provided.