Another super video! I personally never write tests, mainly because I don't understand it well. But with this explanation, wow mind-blowing! That's what we (The community) need. Keep going, you are a true inspirator and the best teacher I ever had.
You underlined very good point in the very beginning: nobody asking to make a website, not even web interface , even possibly without the database. I don't know the seniority level of this task but from my point of view how I would do it: Key features of the task: 1) first of all I see here that this task is about the architecture of the application 2) nowhere said about the storage engine 3) main task: flexible discount system 4) important requirement is to keep the interface provided and example of usage flow 5) tests required Up to me in your solution you didn't follow the requirements of the task as well, but for sure your solution is much more closer to the requirements then the original one. A) You didn't pay attention to the required interface which is named "Checkout" but not the "CheckoutService" B) you didn't provide a flexible and convenient way to handle the discount system. C) you didn't pay attention that in the task wasn't mentioned the storage engine and you decided by your own this should be a relational database. What I would implement: 1) repository pattern coz storage engine not mentioned. This would also speed up my development process cuz I wouldn't use database but simply use data from json or even array and the same time I would show that I know what is repository pattern. 2) to handle flexible discount system I would use strategy pattern Each discount would be handled by the separate discount strategy. In our case would be implemented two strategies BuyOneGetOneFreeStrategy and BuyFewGetDiscountStrategy To make connection between products and strategies I would implement it through the array to speed up the development process but in real life would be using database and many to many relation (and this must be mentioned in the task solution description) Without the bells and whistles this task could be really made in 1 hour for the core and additional 30 minutes to prettify the code and fix small issues here and there. And yes I wouldn't use tdd to speed up the development process, I would write tests in the end, but of course for somebody else and his mind attitude tdd could improve the development process.
agreed. I think he only want to improve the test of the guy who sent the email, UI and database is wasting of time. And the design CheckoutService class seem like just helps passing the tests, doesn't well enough. IMHO he should spend some extra minutes to improve the pattern to really get the job, or that poor guy gonna failed again.
My thoughts exactly… especially about the strategy pattern! Using repository pattern is pretty good too as you rightly pointed out that no storage engine was mentioned. I have to say though, that Korop’s TDD approach at shows how TDD works and can help especially with limited time and a bit of anxiety being under exam conditions ☺️
In most small to midsized web applications you will not need to implement repositories. The Repository pattern may be beneficial where you have many entities and have many complex queries to work with those entities. An extra layer of abstraction in this case can help you to eliminate duplication of query logic as well as keeping the code storage engine agnostic. BUT your choice of "showing off" that you know the Repository pattern is a bit of an overkill compared to what is mostly expected and normal (using a database, most of the time a relational) The task doesn't mention a storage engine but it also doesn't mention complicated patterns that aren't used without specific needs and no mocking your data isn't specific to repositories :)
What a great suggestion, most especially the part where you suggested implementing different strategies for each promo, It would be really nice if you could help write a code that would do exactly this and publicise it here in the comment "maybe with a Github public repo" will do and I am sure the community would beneifit from this and people will learn more and improve their thought process too Thank you!
Awesome video!!! I especially like how you explain what can be improved and stick with what works given the time constraint. TDD approach you used makes the solution come together in such a simple clear way. The comment section also taught me some interesting concepts. I had never heard of the strategy pattern.
Something that should have been mentioned is that the interviewee should have used integers (or better, objects) rather than floats to represent the cost of products. You can avoid tons of problems by returning an integer and always counting in cents. Easier to store, easier to divide, easier to multiply, no weird rounding concerns. Also, using an object is really simple now that you can create custom casts in Laravel. You could store a "Money" object in the products table and retrieve it from the database without any extra effort.
@@AMD-1087 Yeah it's great practice actually, this article shows a good example for this : www.twilio.com/blog/create-currency-data-type-custom-casts-laravel-7
Enjoyed you building up the tests as a lesson. Probably need to add a user or session id to the checkout as seems there would only be one checkout and if multiple checkouts happening. But I know that was not specified in your brief or maybe I missed it.
I did the task with Pure PHP, but I really would like to see more of TDD, as I have never used it professionally... I would like to add, to the person who requested the code review, to PLEASE read about PSR and especially what relates to CODING STYLE!! I would have rejected them for that...
I cannot stand those short-time limited tasks. I can't really push myself to do a subpar implementation that I am not happy with and send that back for test (imposter kicks in - "they won't count that as a mid level developer work, that's a crappy test"). Luckily, I haven't gotten such tests for the longest time. They usually just give 2 or 3 days now. Of course, can write clean code, but I love to implement DTO's to help with parameter clarification and autocomplete and without them it does seem kind of wack.
TDD is a very good way of thinking, you just think of 'what should I get in the result' and starting doing it step by step. Also don't forget to mention that ideally all methods that do calculations of discounts should also be TDD'ed, that's why you got 'get_one_free' rule with 2 args that you never used. I understand that 1 hour is really not enough to do this task at least 7/10 way. Ideally all rules should be separated classes implementing one 'DuscountRulesInterface', so in the end it is very flexible, but 1 hour is just not enough for that. Your video is 36 minutes and you never structured a DB, nor seeded the products, so that should also take at least 10-15 minutes more. The thing I really don't understand is how one could make a properly working website with good design and all the functionality in 1 hour (credits to whoever wrote the actual code)
Great review. I think I would have abstracted the calculation strategies in separate classes, both of which would implement a single interface that forces a "perform" method that asks for an array of "CartItems". I likely would have made separate instances/models to represent the Cart, the Cart Items and the Products as well. I also wouldn't have applied the rules on particular items, since it's not excluded that in the future, some rules might apply to separate different items combined together. The downside is that the checkout total likely would change depending on the order in which the strategies are being applied. But at this point it's up to the imaginary "managers" to determine which should apply first.
creating dynamic callable function with different parameter numbers is easy you can use this method: $a = ['array_merge', [1], ["a", "b"], [2]]; // define your function name with parameters $b = $a[0](...array_slice($a, 1)); // call it using the parameters without worrying about the count of parameters
Just some suggestions: - I would NOT have used models inside the service. - instead of calculating the total on each scan, i would have added a magic getter for the total and calculate it only then. - the rules array would be more flexible if it where an array of closures pass them the "cart" and return a discount in currency and subtract that at the end so the user sees his savings - instead of 3 tests use a data provider - i would have used laravel at all for this - just PHPUnit or pest would be cool if you can share such code katas on twitter and gather results from ppl and compare the approaches - would be a interesting format too
Valid points. But remember, the limitation of the task was one-hour (well, in reality all preparing/shooting/editing took like 4 hours, but still), so I went with the quickest approach that came to mind. Also, I've so many people write/structure the tests in different ways, that I stopped arguing about which approach is wrong or right. It depends on the project requirements and past experience of developers.
3 года назад
@@LaravelDaily no front. I can't imagine how hard it is to produce such a video. It's easy to sit an watch and recommend 😉 just wanted do give you my opinions ☺️
Excellent video-thanks! I think if this code was submitted along with some comments about what would be improved given more time, it would surely be a "pass".
Great explanation on TDD. Makes me rethink my approach to not write unit tests.. it seems easier than expected.. Maybe you can make a video one day about writing unit tests for existing code and how to implement automatized testing
I've done a few videos already on this channel: ru-vid.com/show-UCTuplgOBi6tJIlesIboymGAsearch?query=tests ru-vid.com/video/%D0%B2%D0%B8%D0%B4%D0%B5%D0%BE-P-cuiTVXlck.html
@@LaravelDaily Povilas you are absolutely right! Thanks for point that out! I will check your other videos for that! Offtopic: Thanks a lot for your videos & your great style of explaining things! I have been following you for quite a while and you and your videos encouraged me to start a project, I was planning about for quite a long time. Thanks to your videos, this is now working out! All the best, David
@@flying_david This video about testing from Povilas ru-vid.com/video/%D0%B2%D0%B8%D0%B4%D0%B5%D0%BE-DRhhfy2sG1E.html is very good. It's feature testing, but it's an amazing starting point on how you can easily test Laravel apps.
Perfect :) But i was expecting, that you will play with arrays or collections (without database) and use interfaces as pricing rules :) And for $co->total we can use php magic.
Hey Povilas and chat. In the subject-ish, what article or video would you suggest to study about implementing a cart system with Laravel? I see that exists a lot of options in Github, but for someone that still learning, do you have any recommendation? Thank you!
The test could be more easily written using a dataprovider. (google phpunit @dataProvider example). I had posted a more complete comment but it probably got autodeleted because I shared two links in it. I tried my hand at doing this interview before watching your video with plain php and minimally implementing the interface shown in the mail backed up by tests but it took me 2 hours in total.
Even using eloquent is not mandatory here I think, just making a simple DTO for the products would be enough and the on the fly calculate the total based on the pricing rules. We don’t even need Lara ela for that
@@LaravelDaily I agree with you. I said a DTO as an example, even an associative array could do the job for the interview. I'm sure you get my point. Of course it depends on to whom this interview was given to, If he/she was a junior developer then sure go ahead and spin up a whole framework and a database, If this was meant to for a senior developer position then I would be sceptical about the experience of that individual - given that laravel was not a demand for the assignment. I frequently meet people that consider themselves as a senior developer just because they can use laravel or whatever without knowing at least some architecture (design patters, DRY, readable code etc) and to me that's a problem for our PHP community. I mean as a fellow developer, not from business perspective.. Thanks for the video!
Sir, I have even shorter time to do a test check my developper capabilities. 10-15 min to write some sorting and min-max algorithms in memory, that's insane was for me and as a result I failed to get a job. Their execuse was that a great developper should handle and solve stressy situations ASAP. In that company, where mentioned developper tried to apply, in that one hour much better to do test: quesitons with chosen answers and some open questions orally or written.
I would avoid such companies that give stress to developers at the job interviews. It means they will give the same stress on the job, so do you really need that job?
I do not think that it should be unit test. You implemented feature test actually. You tested not functionality of specific functions and methods. You connected to database, and in future you will need to get config of rules. And later you made unit test as feature one. It is bad idea. Then I saw that you have a mix of approaches in strict types. Sometimes you pass a params with strictly typing, sometimes not. I think if you started to use approach like: public function scan(string $param1, float $price, float $discount) Then you should use it in all functions and methods. And I do not think it is good implementations - to put methods for different scenarios in one class. Use Strategy pattern instead.
@@LaravelDaily Its always easier to review code and improve it than to code on the fly with a strict time limitation. Honestly I think the approach used was very clean and clear given the time constraints
@24:49 can anyone please give me more clarification about what he meant by telling "if the quantity is 3, you will pay for one product, plus 1 product" ? about what product he meant same FR1 product? or all the products together? Thank you very much. and this is pure gold for free.
That's the pricing rule for FR1, "get_one_free". For instance, if there are 7 FR1s in the cart, floor(7/2) + 7%2 3 + 1 = 4 of FR1s will be charged. 1. Buy 1, 2. Get 1 free, 3. Buy 1, 4. Get 1 free, 5. Buy 1, 6. Get 1 free, 7. Buy 1
This is what I have from developer: "the recruiter said the coding is not in enough standard!. As for the test, I have been only instructed that it's a PHP test, and as it says I can use a simple class, framework, or structured files as I wish. So I decided to use Laravel without any Livewire or Vue JS. I also got feedback that I calculated the discounts on the checkout section which is wrong!! They should be calculated while they are being added to the cart. It felt strange as it says on the instructions that in the checkout systems the rules will take place." But I don't have any information about what POSITION it was for, so that context would draw the full picture.
Thanks for the great video. I need your opinion if we want to store Cart or Wishlist without user login. and when user will login the Cart or Wishlist goes to that person account. what will be the good approach. Any package for laravel which will handle this thing. Or using session for this (Any article for this). Need you suggestion regarding this point.
Hello sir most of the I got confused that should I make multiple routes for performing different tasks or should I make only one and pass query params and use some if else logic or some pipeline logic to achieve those task more similar to graph api. Your guidance will be helpful
Doesn't adding rules as parameters to the Checkout constructor breaking the single responsibility principle? In my opinion, Rules must be set in the Checkout class, not added from the outside. And $co->total brakes encapsulation, it should be a method $co->total(), so the example is not ok.
Povilas, isn't there a concept that once you are touching the database that you are no longer actually writing a unit test? I've wondered what the line is here and occasionally have seen tutorials suggesting to turn unit tests into feature tests by changing the aiming of the TestCase class to use the default feature TestCase class. Would it maybe be better to just move the test to be a feature test at that point? Curious what your thoughts are. Thank you for the great videos btw!
Interesting thought. I always thought that feature tests are for all the cases that actually touch the exact features, like web routes. And unit, in my opinion, is exactly what I did here - testing the service method unit whether it calculates correct numbers. But I guess you're right, and there's a reason why I changed extending TestCase of Laravel instead of PHPUnit. So yes, moving this code to Feature would make sense.
Honestly it depends by what you define unit test. There are many definitions of it etc. If you define unit as a some kind of process - then touching database is fine, if you need speed you will switch it to just in memory database. If you define it a smallest part of your code - then yea, you shouldn't really touch a database there. Most likely when you want to touch database this is kind of integration test - where you check integration with 3rd dependency like database. But i sometimes saw opinions that those integration tests.
Philip Theobald you're 100% right. Those are feature tests. This kind of logic is perfect for true unit TDD, which would not require any DB. Passing a Product object instead of fetching it from the DB would allow this to happen. Also, calculating the total in memory instead of doing a request on each iteration would also make this possible, and would make the design better. It'd take out the responsibilities of handling the DB from this service, which would only be responsible for calculating the cart amount. Feature tests are amazing in Laravel for simple CRUD or HTTP validation testing. But they're very costly, the test case of Laravel by itself will usually take around 100-250ms on each test, and that's without DatabaseMigrations trait / db:seed setUp. I have seen projects which had test suites that'd take several hours to complete because they only did feature tests, so no one ever run them anymore, only the CI does, which makes the feedback loop for the dev extremely slow, and that removes a huge part of the benefits from testing. So try to do mostly unit tests for each small part of the app, and some feature tests to prove the app works as a whole.
@@zHqqrdz well it all depends, i agree that writing only feature tests is kind of bad, but it's also good to have integration tests, which will show integration between components in your app, or run on inmemory DB rather than feature tests running through request and response, having mostly unit tests can be a mistake because in units tests everything will be green, but will fail at integration level
@@LaravelDaily I would love to see you talk about this at some point after deep-diving and forming a strong opinion about it (if you find it useful - of course). I've been learning a bit more about unit testing from various sources (mostly co-workers who know far more than I do) and have come to understand that true unit tests should generally avoid other units of code, services, and, importantly here, databases. I, like you (probably), have seen many courses that suggest to do exactly what you're doing here. This is precisely what makes unit tests more difficult to write than "wide" / feature / integration tests. Basically having to mock data and write code in a "testable" way. Bypassing this could potentially leave a false impression where new developers could think - "writing unit tests is easier than I thought - I can just use the database like I do with feature tests" but if that were the case there would be no real fundamental difference between the two and there would be no need to change the TestCase class to make that work. Hope you didn't mind me pointing this out and this is completely just my understanding of things and by no means "100% absolute truth". As always, I really appreciate all of the knowledge you share! This video is absolutely great. Keep up the amazing work!
I think that the main reason he didn't pass is because the reviewer had no chance isolating the candidate code from the laravel bloat to actually assess it. The task was simple and clear, 2 php files would have been more than enough i think. The goal of the test was to assess the problem solving skills, not laravel mastery.
Its obvious that they just expected api or even just CLI script. Not sure why create website, if this is recurtation for PHP Developer, not full stack. The question is what exactly does cart delete do? Does it delete cart or product from cart? If the second one, the better solution is to just have put for cart with saving new cart state. To be honest - why even make rule as array? Much more appropiate seems to be some kind of class behind the interface like PricingRule which would have method like apply to a checkout object. Honestly many times in many jobs very crucial is understanding of the task and what they expect - if they don't expect api/website - then just don't create it, simple as that. Personally also for me - scan method is kind of pointless, it doesn't really say what it does, and in your case it's setting total on checkout, for me this would be very unexpected. Scan method also feels that it should return something like in redis for example, or even scandir in php - return array. I would just ask recruiteer in such case for more details what exactly scan method should do because in this description no assigning it result to something is very unexpected for me. Also about RefreshDatabase - it's much better to use DatabaseTransactions - this way on setup of your test you have transaction started and on teardown it's rollbacked - nothing is ever written to disk so it's very good for tests performance.
it's worth to mention that this particular implementation is not useful for multiple users system as the carts table are not storing user_id. anyone who uses the system would be able to see another user's cart and they will be sharing the same cart and same total price.
Yes I've mentioned it at the end of the video, that for real-life system there should be some kind of a session or registration with user_id, attaching a cart to them.
@@LaravelDaily I must have missed the final parts of the video then 🤣 regardless thanks fot sharing tdd video. Laravel Octane next maybe? for windows hahah
main moment - here is not an a 1 hour journey. To much hard coded elements and very blur conditions. Like existing DB. My opinion - junk job offer behind all of this.
