The refactoring test (2) - Open-Closed, Single Responsibility | Cracking the .NET interview

hello everybody i'm nick and this is the second part of our crackingthe.net interview all about the refactoring test that you might encounter and it's all about being asked to refactor an existing legacy piece of code usually now if this is the first video you see in the series i highly recommend you go to the first part first you check that and then you understand how we landed here because i think there's much viable information in that first video that being said if you have already watched it please stay along and let's go dive into it before i show you the code and before i talk about what i'm going to do today i just want to mention that if you like this type of content and you want to see more please subscribe during the summer notification bell so you get notified when i upload a new video and something else that's very weird is that only 30 of you watching are actually subscribed so i would really appreciate it if you're a regular viewer who enjoys the content to actually click that subscribe button it really really helps me you can't imagine how much so without any further ado let's go straight into the code so what we had here from the last time is we ended up writing a few unit tests to make sure that the logic is guided and we are ready to now refactor the user service add user method where we're gonna identify what is wrong with it and we're gonna try to make it cleaner it's all about that so before i i talk you through the lines i want to address something else which is in the comments somebody mentioned why didn't you rename fur name as the parameter in this method because third name is referring to first name which normally you wouldn't abbreviate you would just say first name as the parameter the problem is that if somebody chooses to use since this needs to be backwards compatible anything we do we wouldn't break this code if i was to rename this but the system although actually the test says that assume that this is part of a bigger system so if somebody was to use that method using named parameters or named arguments and had something like this because some people like to do this even when they don't have optional parameters then i would break their code because they would need to update the named argument to point to the new value so that change is not backwards compatible you probably won't get failed for doing it but the interviewer will question you about it it's one of the smaller things that you might not think about when working for code that needs to be backwards compatible but it would be a breaking change and in a realistic scenario you'd have to increase the major version of the package and document it which we cannot do here any change we do needs to be backwards compatible so keep that in mind you know try to think outside of the box so back to what we have to do now here the first thing that i can see is that we're doing validation for this request here and this other user knows about the validation uh we're doing it here here and we're also doing it here now in a realistic scenario where you do this from scratch in a new piece of software i would argue that this sort of validation would happen outside this method on the uh maybe on the api level to make sure that the user is providing um the fields that they should provide i don't know if you know check for email should be there um this should definitely not be there but things like the field is not empty is something outside of the domain but it could very much be part of the domain as well because we're so tangled with ddos and domain here i won't be making a validator per type of thing i'm validating on different layers i will just call it a user validator and move my logic for validation there so what i'm going to do is i'm going to create a new validators folder because the reason why we're doing this in case you didn't understand or didn't explain it is that this method shouldn't know how to validate directly to use a validator who is responsible for knowing what it should do this is all about adding a user um not about knowing so much about the validation logic so i'm gonna go ahead and create a new class called user validator and before i put anything there what i like doing is i like extracting the things that need to be extracted in this method first in this class and then moving it to the user validator so the first thing i want to extract is this and i want to say has a valid full name and the reason why i don't say user has valid full name is because this will be moved to the user validator so you understand who needs to have a valid full name from the name of the class so i don't need to repeat myself by saying user this user that in something called a user validator like i would argue that even the method being called add user in this user service is redundant but in any case i'm going to extract this now the check is wrong because it says user has valid full name but i didn't want to call it user has invalid in name full name because negative checks that are being negated are very hard to read so instead what i'm gonna do is i'm gonna negate here and i'm gonna go on this and i'm gonna negate the check so i turned it into a valid full name check so i reverted it and if i run the test by just saving since i have continuous uh testing on you can see that the tests are passing so that was the first check the second one is um has valid email uh so has a valid email i know that this valid email check is not really a valid check to begin with but we cannot change that logic because any change that fixes the logic wouldn't be um backwards compatible with what the code is expecting right now so i'm negating that as well saving and as you can see nothing is breaking and i'm gonna go back and i'm gonna extract the age check as well so here the age check is getting an offset and is trying to make sure that you are not um younger than 21 years old so you need to be at least 21 years old so if user is at least 21 years old and yes this check was inverted for me automatically this is amazing and i'm gonna go in that method i don't like i like using var instead of uh using the int or explicitly specifying what the thing is um and now if the age is more or equal to 21 then we are returning and by saving you can see that my tests are passing and i have tests for these cases so if i was breaking the logic the tests would fail i almost died there and the last thing is that assuming you have a credit limit the cred credit limit is less than 500 that you're still an invalid user so if user has credit limit which is less than 500 actually the wording here is a bit weird so we don't need the user uh has credit limit and limit is less than 500 that's more appropriate now we're gonna treat everything again like i said under the same banner of user validator but i'll show you once i move them a small trick to keep the logic in the main method more to the point so this method should contain everything about the validation i'm gonna go ahead and add everything that we're missing and now we also need this user time and so the daytime providers that we had from the previous video i'm going to put this here now you go and inject it from the constructor and all of them need to be changed to public so i could use ctrl h and do this automatically but what would be the fun in that i just almost broke the code fine cool so now this is fine i need to remove the daytime provider from here as well and here as well and i'm gonna need you uh cool but i now instead need the private read-only user uh validator the reason why there isn't an interface for that user validator is because i won't be mocking it i don't need to mock anything about it i will mark one of its dependencies but not the thing itself so i don't need to have an interface for this specific use case and this one is new user validator with a new date time provider and we are going to use the user validator dot here here you go now a couple of things here could be oh the test broke so i have to address that as well you go here user validator datetime provider which is now a dependency missing bracket because i have fat fingers and now everything should be passing and this gives me gives me so much confidence when it comes to refactoring because i don't need to worry about breaking anything i have my tests checking everything about the code if they break i know i broke something so here i have three checks that are like prerequisites to the whole request to be started you can argue that the age check is very domain specific and i do agree we could just extract those three things but i'm going to group them anyway because i'm going to treat it as user provided data is valid so i'm going to say extract method user provided data is valid and the check is inverted so if it's invalid it returns false and if i save and expand my test you can see that they all pass and now i have this individual check that's summarizing all the incoming property validation then i'm gonna fix this because it's really getting through my nerves i don't know what key binding i just pressed but it worked so the main beast which is this this part here right this is a clear open close violation every time we want to treat a client specifically based on their name we need to edit this method um and add the new case with an if there's another thing here which i think i would gloss over later so i'm just going to mention it um we're using codes oh sorry we're using comments here um obviously the code is using comments to explain what the logic is and that's a big no no in general the only comments you want to leave anywhere in your code are either method summaries which are not really comments but this type of thing you want to have especially in public members you don't want to have comments like this forget about this this is absolutely i want to say horrible it's really bad um the only comments you want to have are to do comments that can be filtered and searched um and they imply intent rather than code behavior and mainly because the comments can lie they can don't use them like i'd rather have this skip credit check and now i don't need the comment because the code is the comment and it tells me anything i need to know about this and i can do this with all the methods i can do do credit check i would even do credit check sounds vague like aren't you doing that anywhere like you're not really doing the credit check here you're checking about the credit here here you're just assigning some values so even the comments in this case are actually lying but i'm going to remove them anyway i don't really care about them to give you an idea about the logic we're going to be dealing with if the client name is very important client then the credit limit check is false and we won't check about the user's credit limit if they're an important client based on the name then they have a limit but it's twice what the credit limit service is providing and then any other user has a credit limit which is the exact limit that the api is returning for that user so there's so many ways to deal with this violation strategy patterns i don't want to name any others because i have a very specific way i like dealing with this um plus i don't want to give away how you could try and solve this without something different than what i'm going to show you but what i'm gonna do is a bit of a like it falls under flex coding like you're doing it just to show that you can do it it's gonna be perfectly correct and perfectly performant and reliable and what you wanna do um but it goes over what some people might think that the solution to this problem will be so don't feel like you have to solve that problem in that very specific way there's many ways you can do it and one of the other reasons why i'm choosing to go over the top with the solution is because if you actually watch this video and you encounter this test and you try to copy what i did in this video in your test and the interviewers of the video as well you're going to get question about it so that's another reason why i'm doing it so let's go ahead and see how i would solve this right this is a number clause violation so normally i would have some sort of handler per type of client name and then a default and that's how i would go about it um and then i would just send a request and say that's the name client what uh credit calculator provider do you want me to deal with you know with some sort of factory so let's do that so the first thing i want to do is i'm going to create a new folder and i'm going to name credit providers now should this be in a folder called credit providers not necessarily but it does in this limited context give some organization to the classes so i'm going to use it anyway and close the ones i don't need to use i don't need you then you i definitely don't need you and in here i'm going to create a single interface called i credit details or credit limit provider and this interface will have two things first it will have a method called get credit limits and it's gonna accept a user and i could make an object for this but i could as well just use a tuple here and i'm gonna say boom has credit limit and and credit limit i could of course like i said create an object about this one but i'm going to show that i also know how to use this and it's perfectly fine you don't need to make an object test for that you could but you don't need to and the other thing i need is a property which can be that name requirement and this will be a getter only property now i have three use cases which is the very important client which doesn't have a limit check an important client which has some logic and then the default so i'm gonna go ahead and start with the default provider so default credit limit provider and i'm gonna say i credit limit provider and i'm gonna say that the requirement here is string dot empty so there is no limit and i'm gonna show you how i'm gonna tie it into the rest of the logic when i'm done with the others and here all i need to do is go to the user service grab this fella and this method will be pure meaning it won't be assigning anything to the user it will just use the user as an argument and the assignment will happen in the user service itself so the responsibility of the provider is to provide the data not to set anything so this line bye bye the only thing i'm gonna say is var credit limit equals this and now i need to inject this credit limit service so i'm gonna go ahead and copy that and since i want to use it anywhere else in this method or class i'm going to go ahead and delete it fully why you didn't like this oh yeah here we go and i'm going to inject it here so you go here you go here you go here and return the has credit limit is true and then the other the number is the exact thing i get back so this is true and this is credit limit so i just made the default credit limit provider and now i need to make the other two one will be for the important client and the other one will be for the very important line so let's go ahead and create that important client credit limit provider i create limit provider yada yada implement those things and then the name i'm going to go ahead and copy it important client here we go we paste it here and then the logic for the import client is copy that where am i here we go like i said pure method we don't actually mutate the user in any way shape or form wait a second this should also be deleted good catch and this doesn't need to be here cool so now we're gonna return the is true and the credit limit is the value multiplied by 2 and we need to inject the private read-only i user credit service yes that's it and inject it so here we go and we need one last one of these fellas for the very import fingers important client credit limit provider nice long descriptive names all right credit limit provider here we go the idea and the value is let me just copy the right name because i don't want to have to worry about messing it up and here there is no logic the value is just return has credit limit false and the default of an integer which is zero and now we have all the providers obviously this logic has to go correct yes and the two things that we want is that user dot credit limit equals something and user dot has credit limit equals something let's just limit them like default them to the default thing so here what i need is something to give me the right provider uh so provider goes here equals credit limit provider factory maybe let's say yes get a provider by client name and say client dot name and then now we have it we can get the var credit in fact i could actually do something cool i could say var has credit limit coma credit limit um equals provider dot get credit limits provide the user and get that and then all i can say is has credit limit and credit limit and now i just need the credit limit uh provider factory um so i'm going to go ahead and do that so in the credit providers i'm going to create a factory um do i need it to be an interface i don't really need to need to be an interface um so i'm gonna say credit limit provider factory and create that and now what we're going to do here is a bit of a stretch in what you could do normally you'd have um something like this done automatically based on convention but here we're going to get our hands dirty and we're going to write it ourselves um so something that is scanning finding the providers initializes them and then returns them now we could create a new provider every time for every request um in this scenario i'm gonna pre-initialize all the providers and use them as singletons since they are pure methods um that don't mutate the object and they don't they're completely stateless but you could very much change the name of this from a provider factory to a holder an orchestrator it's completely up to you in this scenario since you don't know how they're being returned you know something like this could even be internally you wouldn't even get exposed to it i'm gonna call the factory so what i need here is a private read-only eye dictionary in fact i read only dictionary because i don't want this to be mutable in any way shape or form it's except a string which string will be the name requirement and it returns an i client provider and i'm calling this credit limit providers here we go and i'm going to be initializing this in the constructor so once for the factory for the whole lifetime and we're going to treat it as a singleton as well and the way we're going to return them is we're going to have a public what they call it get provided by a client name so and it returns a ni credit limit provider and then you accept the client name assuming that this requirement changes in the future you might want to accept the whole client object instead here and base your check off of client properties not just the name but since we're factoring what we already have i don't need to use the client or the whole object so i'm just going to use the string instead and then we're going to return it by uh saying client providers client name we're going to fix some checks around this because the client provider might not exist in the dictionary but fundamentally this is what we're going for and then here we would have something like private read only credit limit provider factory uh initialize from here and delete from here let's say new credit provider factory here here we go and let's fix our tests so this service goes away that being said i'm probably gonna need that and new provider factory goes here so if i save and i run my tests they obviously now fail because there is nothing in that provider to sort the factory to provide a provider now this is the part where i might scare you with what i'm going to do but bear with me it actually does solve the problem some many many people might object that you know i'm going to use reflection because i am going to use reflection and that's slow but a reflection is only slow for when you're initializing it and since i will treat this as a singleton which is being injected and maintained for the lifetime of this service there's nothing wrong with using it because the initialization which can be very fast by the way reflection isn't slow anymore really um then there will be no problem it's actually an absolutely perfect and acceptable uh way to do this so what i'm gonna do is i'm gonna say type of um i'm gonna choose a type in this assembly so create limitprovider.assembly.org exported types and these are the types that the specific assembly is exporting and i'm gonna say where this type and in fact let's just overkill here provider type and save the type in a reference and reuse it in a couple of places and i'm going to say credit limit provider type dot is assignable from x which is the type and x is not an interface and is not abstract the reason why i'm doing this is because i'm scanning all the things that implement the i credit limit provider and i'm doing that because now i'm able to initialize them pre-initialize them in the factory um and be able to find them out based on the request that they get now the problem i have with doing this dynamically is that the default and the important client classes have the user credit service as a parameter or an argument in the constructor while the very important doesn't because it doesn't need to to keep it consistent you could just inject it without actually doing anything about it i'm just gonna do something else what i'm gonna say is i'm gonna select i'm gonna say parameter less constructor equals x dot get constructors dot single or default where get parameters dot length equals zero so basically get the at least one parameter less constructor for this implementations and now i can say return parameters constructor is not null then use activator to initialize the thing so create instance if else activator dot create instance x coma and this is where we're gonna now inject the private read-only i user credit service that this factory requires in fact i don't even need it here i just need it injected and i'm gonna use it as a parameter here if you don't exactly understand what i'm doing with this bit basically i'm trying to get the parameters constructor for the very important client first and if there is a constructor that has parameters then the only acceptable use case is a single parameter which is the user credit service so i'm reusing the same injectable thing keeping them testable even through the factory and now they have that i can say dot cast as an i credit limit provider and then to immutable dictionary with the first thing being the name requirement and the second thing being the instance itself and now i have all my pre-initialized credit providers in the dictionary and every time i just want to add a new one i just create a new class with like super very important provider i give a name and a logic and it automatically will be picked up fixing the open closed principle both on the class and the method level and being able to use a very dynamic approach to solve the problem and now this is clearly wrong because we don't want to deal with it like this what we're going to say instead is var provider equals client providers dot get value or default and by getting default we now have a null to deal with if there is no provider specific for the client which in this scenario will default to the well default scenario which is the string is empty and i'm going to extract this for readability so i'm going to say default credit limit provider which we know for a fact that will always be there and with that out of the way i'm going to go ahead and fix all the missing dependencies so this needs to have the new user credit service client here and the tests need to have the new user credit service client here and if i save i have failing tests perfect why is this failing oh i shouldn't have initialized a new client here i should use the the mock instead that was on me use your credit service save and my tests are now passing because and let's just quickly debug one of them let's say we're debugging the uh good use case scenario yep should be true so let's go ahead and debug this uh yes here we go so test is running and i have hit the end point the breakpoint so i'm gonna go here yep this is provided we get the client from the mock and now we're gonna get the provider from the dictionary so as you can see we have three providers pre-initialized you don't need to have them pre-initialized you could just have instructions on how to initialize and create one every single time but since they're pure method pure methods in the stateless there is no reason why we cannot just keep them in state in memory and we get that in that scenario we have a provider so we return it's the correct one and we get the credit limit which is true and a thousand or twelve thousand uh 1200 jesus and everything works absolutely fine so this is how we now fixed this violation once and for all every time we need to deal with new providers we just create a new provider implement the interface use it with a different name and then automatically they will map and that's where i'm going to leave this video let's replace that tomorrow so what we did is we fixed the dependency inversion principle and we were able to inject now and test the service we extracted some of the responsibility of the validator into its own valid data or class we ah in fact this can this thing can also be extracted in the method and let's just say apply credit limits so now this reads even better where we just apply credit limits in here using a factory we're getting the provider appropriate for the name and then we are getting a credit limit dynamically and in the end we're doing a check and we create and that is where i'm gonna leave this code there isn't really much else we can do on this one and if you have any other suggestions to make this cleaner or apply more principles please leave a comment let me know but i think that this is a test that is passing with flying colors and it's addressing all the major issues that they are presented to the person being interviewed that's all i had for you for this video thank you very much for watching special thanks to my patreons for making videos possible if you want to support me as well you're going to find the link in description down below leave a like if you liked this video subscribe for more content like this ring the bell as well and i'll see you the next video keep coding

Channel: Nick Chapsas

Views: 27,449

Keywords: Elfocrash, elfo, coding, .netcore, dot net, core, C#, how to code, tutorial, development, software engineering, microsoft, microsoft mvp, .net core, nick chapsas, chapsas, The refactoring test, Cracking the .NET interview, .net interview, c# interview, c# interview questions, .net interview questions, interview questions, programmer jobs, .net jobs, c# jobs, refactoring in .net, refactoring in c#, interview, software engineering interview, open-closed, single responsibility princinple

Id: Yd4GnWeEkIY

Length: 31min 59sec (1919 seconds)

Published: Mon Feb 22 2021