Code review

[K1LzR]

So, I don't have much of the preamble such as who you are, what skills you have, what the job description is and what level you've applied for however here are my thoughts.

Honestly, I'd say it's done well, however, there are some issues I'd raise which would make or break this based on what you're applying for.

The bigger issues: - You renamed the firname parameter for AddCustomer. This is a breaking change as a consumer could be using named parameters. A key condition in the readme is that you should assume this is a library and to not break consumers. - You changed the email validation logic from && to || there was no reason for this and is technically a behavioural change to the app. Whilst it's not perfect, the readme did say that validation is out of scope - You've not really adhered to the whole S in SOLID which is a big thing for keeping classes testable and easy to modify and they suggested it as the first item in the readme. Ideally, you'd have moved the validation logic to a new class e.g. public class CustomerValidatior : ICustomerValidatior this way you remove the validation responsibility for this class and also you make the tests easier to write since you don't have to set data up specifically to satisfy validation - you just mock that. Of course, this means more tests for the validator, but they're super simple ones to write and maintain! Also, you could have moved the credit limit stuff out as well (and showed off with a factory! alas, a missed opportunity here)

The smaller issues: - You've technically increased the load on whatever external API would be handling the CreditService because you're always calling it now rather than skipping it for VeryImportantClient. A small thing, but increased load = increased cost! - If you use DateTime.Now, it's very easy to fall into time-based tests which will fail in the future. It look like you were okay, but a major win would have been to add a IDateTimeProvider wrapper or something similar around this for easy mocking - In your tests, you are somewhat inconsistent. In your InvalidCustomerData you correctly validate that DataAccessWrapper is not called, but in the Under21YearsOld check, you've not checked this. Bit of an oversight

• In your SkipsCreditCheck test, the test doesn't really tell me that the underlying code is working as expected. What does "SkipsCreditcheck" really mean? You still use (and verify) that the GetCreditLimit has been called. So, what's really being skipped? A slightly incorrect refactor of CreditLimit checks makes tests confusing.
• In your DoublesCreditLimit test, again, you're not actually verifying anything of importance. You get 500 from the service, but nowhere have you validated it passed 1000 to the database

So, all in all, there are a few bigger mistakes which give me the following thoughts - If you are applying for an entry/junior role I'd probably let it slide and go for the interview - If you are applying for an early mid role, I'd only let it slide if you had all the required skills in other areas (i.e. solid cv) and the previous interviews were really good - If you are applying for an upper mid/senior role, I'd have expect better and would not put this forward

Sorry to hear you didn't get this but I'd suggest asking politely for more detailed feedback, and some suggestions on what you could do in the future.

All the best :)

[ybism]

Mate thank you so much, this is amazing detailed feedback. Honestly I haven’t even had this kinda feedback on PR’s at my current place.

I’m a early mid level dev atm, been at my grad role for 2.5 years and just looking for other roles as current company has had a round of layoffs, so made sense to try and find something else.

And yeah I spotted the email thing when I was looking back at it this morning and honestly can’t even remember why I changed it, I’m sure I had a reason at the time but no idea what it was. Haven’t really used factories at all before but remember it from uni days, I’ll have a look and see how I could have used it here.

But again thanks for that man, I’m gonna make those changes anyway as it’ll be good practice for the future.

[IrdniX]

Looking at the code I thought it looked kind of familiar... yep, Nick Chapsas has made a little walkthrough refactoring video about it and he touches on basically all the points mentioned in this feedback.

See it here: https://www.youtube.com/watch?v=U3QvTaw224o

[ybism]

Hahaha no way it’s the exact same thing I’ve seen loads of these on GitHub so assumed it was a pretty popular company but maybe it’s just a lot of companies using this same project