Nice job overall Nick. A couple of comments regarding other issues that are still in the code (pretending this is the production code that should be maintainable and easy to reason about): * Code organization. You have folders for Models, Services, Repositories, DataAccess that define the design responsibility of the classes within those folders. And then you add CreditProviders which breaks this pattern as CreditProvider is a business concern. I would suggest not to mix design and business concerns. You could have created a CreditService and put it under the Services directory. That would keep the top-level folder structure clean and consistent. * Naming sometimes is odd. The method name should say what it is doing and not how. For instance, IsUserIsAtLeast21YearsOld could be something like ‘IsAgeRequirementsMet’. The reason being, if you change the logic inside this method to not allow people over 65 years old, for instance, would you rename the method to IsUserIsAtLeast21YearsAndNotMoreThan65yearsOld? That would be strange. Things will get even scarier when you add several validations inside that method - there is unlikely you will try to squeeze all of them into the method name :) * User class should have a method ‘SetCreditLimit(credit)’ where hasCredit=true, credit = creditValue get set. Otherwise, good luck troubleshooting situations when somebody does this user.hasCredit=False, user.credit=200. Which property (hasCredit or credit) should the user of this class trust? If ‘SetCreditLimit’ is the only way of updating the credit-related properties, this method will ensure that they are always in sync (if credit > 0, hasCredit = true). * The dynamic resolution of credit provider is cool and fancy, but in a real application, one would rather prefer things to be more simple and straightforward. Creating a dictionary with all CreditProvider initialized explicitly lets the code reader understand which providers are being used. And what if I create a new provider, and I don’t want it to be used in production yet. Do I have to comment it out to ensure it does not get picked up accidentally? * A bunch of things like CreditProviders, the constructor for UserService that takes params should be marked as internal. * The business case was a bit strange here. Using ‘Clients’ and ‘Users’ together. Kind of hard to tell the difference and the relations between them. For instance, method ApplyCreditLimits(client, user)… there is no way a code reader will guess what this method is doing without looking at the implementation. Again, good job, and I think many people will find this information useful and I didn’t mean to sound negative. I am just talking from 15+ years of personal experience dealing with code of different quality…
Here's my input on your observations ;) *Folder structure - nice observation; I keep seeing more and more the domain structuring, instead of code structuring patterns inside the same assembly. *Naming - a method name should show intention. If you want to extend it then it's time for a rename, or even take the min and max values from a configuration (probable database). Depends if your component is afferent or efferent as well... it may break the design. *User class should... - totally agree on this one *The dynamic resolution - agree to disagree... I keep doing this for a few years at least and works like a charm. What Nick didn't cover here is the fact you can have a feature toggle on it that is configured externally (probable database). I'm always trying to make all the stuff configurable so I can disable them if those are not production-ready (yet). *A bunch of ... - by default all classes should be sealed and internal, except the fact you want to expose them. So yeah... not only ctors... If somebody dares to send a code review request without this covered then it's clear that has an undisputable rejection from me. *The business case - it is weird... but sometimes this happens even on bigger houses. When you're saying client I'm thinking of the direct consumer (app), that's used by an operator, or even by the final user. So, you may offer extra "stuff" based on the consumer...
I'd watch 100s of these refactoring exercises. This is what I do all day in my day job, and I never get tired of it. Nick uses the same processes I use, with lots of subtle improvements. I wouldn't have thought of using reflection in a Factory like that, for example. If I did something like that in enterprise code, I'd leave comments explaining how it works and how another engineer can extend the pattern. It won't be obvious to every maintainer that they get free registration. They might try to head to Startup.cs and add a services.AddScoped() registration.
Nick, thank you for pedagogic and clear tutorial. I am a senior developer with almost 20 years of experience in .NET, but I know for a fact that experience is not only a strength, it also means a challenge. Challenge to evolve, re-learn and reconsider things taken for granted as software tools, paradigms and methodogies change and evolve. Choosing what to keep as part of the past experiences and what to revise is what is the biggest challenge as far as I'm concerned. I have since quite some time acknowledged the usability of all the patterns and "features" you mention in this video but since there are equally many ways to solve things as there are tools and developers and scenarious - it's often hard to extract the core, the essence of what is what. Details that are features of a specific package or pattern, and what is reasoning on a higher level. It blends together so easily and can be very confusing. What you manage to do in these two episodes is to provide a compact, pedagogic and chrystal clear explanation on many important refactoring and pattern-related considerations and techniques. You are also doing it in a modest way which increase the credability in what you say: many programmers tend to be overconfident when evangilizing something. While this is showing confidence in what's beaing "preached" it sometimes tend to lead to biased and non-constructive discussions. You leave options for doing more OR less and also reasons why one might choose either way. And you pick one path yourself, motivating it. I have (many years ago) been educating in programming, methods of programming (not as lecturer but still) and I find it quite hard to keep up with all new patterns, all new technologies etcetera. Still, the core of what they try to accomplish often seem sound and logic one I understand what they try to solve - but it is often hard to get there. Things move too fast, different flavors of patterns (as in different tools using them slightly different) makes it a challenge to see what's what. What I'm trying to say here is that you are a talent in explaining and showing these concepts and it was fun and informative to watch. I kinda could think "oh, that's a problem" or "you surerly want to do something like..." along the way. Still, there was a constant feel of "ahh, yes, THAT's what they mean when doing like that" or "ahhh, this is why I would do it like that instead if like this". Sort of. In other words - you did precisely what I tried to explain is hard (for me): by concrete and simple example showing the essence of multiple refactoring and testability methods. Thank you and I'll look forward to see more videos from you! //Jens
Thank you so much for your comment Jens, I really appriciate you sharing your perspective. Making those videos some times feels like shouting to the void and hoping for someone to listen so it's lovely to know that I am having an impact.
This is a very nice "by the book" refactoring, so thumbs up for that! However, I must point out that an 85 line sweet little method now has expanded out to a dozen classes and interfaces and probably close to 500 lines of code including reflection and other goodies. And it can be argued that it is way less readable now than before for someone unfamiliar with the code. Because you would now need to go through all the interfaces and classes (most of them pretty empty of logic and functionality) and see what is going on to understand it fully. Just to find some complex reflection logic hidden in a factory method somewhere - that points you towards needing to implement an Interface of the right type in order to add a new card type check (if so required). Before it was pretty simple - expand the if/else statement if required - done. You could see it right away. So while you have done it "by the book" the KISS principle is in my opinion thoroughly violated here. Because if 85 lines of code gets bloated to 500 - then 10.000 lines of code gets bloated to 50.000. And I rather have 10.000 "not so great" lines to read than 50.000 super advanced "by the book" stuff to read and wrap my head around.
My friend, your are either an old-timer or you *really* didn't understand KISS... It is now, by any metrics possible, more readable than before. You say that it's more difficult if you want to see every part of the system. Yes it is, but why should you ? It's called abstraction. You write high-level statements, so that *only* if needed you go see the details. Before, you were attacked by those same details ^^ Simplicity is not the number of lines total. Simplicity is the cognitive charge needed to do the simpliest change in a method. When that method goes to 2k lines, trust me, this is better. Because you'll understand the macro functioning way faster. More info in the book "Clean Code"
Wow! You are the real deal. This video was highly educational. As someone who’s been programming for years but somewhat new to .Net development, this refactoring approach is exactly what I need to fill in the gaps of my design principals knowledge. The reflection portion was something I would’ve never though of and I had to watch it a few times to fully understand how it works. Very useful!
One of the best youtube channel about how .NET works. I really recommend to all .NET developers watch his videos, you'll find many helpful info. It doen't matter are u Junior,Middle or Senior.
Great video. Gonna learn more about providers because this part was hard for me to understand. What I like about this video is that it shows practically how tests can help your workflow.
You are such a guru Nick. Ive never seen anyone else explain these things in such a easy to understand way. I will definately start implementing these teachings more and more to my code. That reflection topic needs to be covered on a different video since it just blew me away because of that syntax alone
Great series, excellent channel. Just a small suggestion for others watching this video. Try to avoid using “var” when on the right hand side you have a method that returns something. Because if someone is reading the code outside of an IDE he wouldn’t know what the return type is without opening the definition of that method. Example: Worse: var client = _something.GetClientById(id); var someName = “John”; Better: UserClient client = _something.GetClientById(id); var someName = “John”; Notice that I didn’t change the second var because it’s obvious what type it is by just reading the code, but the first is not.
I disagree. Unless you need to use an implicit operator you shouldn't use var. For example what you described as "Better" to me is way worse and redundant. I can already see what the var is. It is a client. I can tell both by the name of the variable and the name of the method it's calling. Having it in the declaration too is pointless.
@@nickchapsas how do you know it’s a *user* client from the first example? Trust me I’ve read a lot of code like this and I had to jump across the code base for something that should’ve been obvious just by a glance. Now maybe the example I quickly wrote on my phone is not the best, but i hope you get the idea of where I’m going with it. Also if you’re using an IDE and can just hover on things and get a description, it’s a non-issue, but if you’re reading code (which is what you do most of the time) on GitHub (for example) it becomes a lot harder. Anyway let’s agree to disagree :) I like your content, keep up the good work!
Great video (as always) What I would change, however, is transfer the logic of chosing the right ICreditLimitProvider on the ICreditLimitProvider itself. So, instead of having a string NameRequirement property, I'd add a bool IsAvailableForClient(string) method (or even Client object as parameter). That way, you wouldn't need to add a new provider for each new unlimited client for example. You could even (depending on performance requirements, api calls and all), have multiple available providers, call GetCreditLimits for each of them and take the best one.
wow that was some codegasm. Thanks for your awesome videos. 23 years of coding, managing project and running my own business, your videos teach me something new every time.
Great video and honestly I would love more videos like this. I know that these videos can take a long time to make, but I really appreciate that you go over and don't spare anything just for time. Great job! It really shows a realistic scenario of refactoring code in a real codebase.
As a student (who hasn't been programming in C#, but Java is close enough I suppose :) ) this was very enlightening. Really appreciate the way you explain your thought process while going over the code and the way you structure your approach.
Great videos Nick. I am .net developer of many years experience and I find your vids very clear, very informative and very helpful. Keep up the good work
Once you mentioned 'it is hard to understand a Not check for something with negative in the name' I noticed just how often I was doing that in my own code, and how much I hated that. Good points all around. BTW I am watching to see if my interview questions were too hard!
Haha nice effort (especially the first vid on testing), but I bet you got a bit too enterprisy at the end 😂😂😂 I know it's really hard to hit the sweetspot between testable modularity and expressiveness, so no judgenent. But a little domain model of "RequestForCredit" would have really helped to encapsulate the real business logic and do the service / provider / validator / factory mess somewhere else 😂 But anyways, the outlined skills get C# devs hired, so thank you very much for sharing this vid 😂
Great video! Just one more annotation how even without using named parameters changing the parameter name might be breaking: If in some other part of the system reflection is used on this class for some reason, that would also potentially be breaking.
Nice. I would have injected the ICreditLimitProviders as an IEnumerable and just find and use the right provider based on ClientName. Although this would mean that the AddUser method would need to know how to select the right provider, which could be inappropriate intimacy code smell.
Oh yeah. Thank you Nick. I also use that reflection method to get all interfaces prior to O/C principles, though reflection might go slower during loading of all class with that interfaces, it will generate you an instance by name will get faster
That original startup time is so negligable that to be isn't even a point of discussion but I thought I would mention it in the video because people sometimes think that reflection is as slow as it was years ago
Hey Nick can you do a project without using the LID of SOLID to identify how you dont need LID to build maintainable dotnet project and see if you notice a lack of code pollution
After watching this part I have a couple of comments. First one is about a placing credit logic inside credit validator. For me it looks wrong that validator decides what is a user credit (single or twice). I would say it should be a part of user credit service. The second one is credit validation should be a part of all other validation happens before adding user. Plus I would prefer to design validation logic as method decorator or interceptor pattern. It would be cool to see it in your next videos
Which credit validator? There is no credit validator in this video. The credit logic (none, double, single) is in the CreditLimitProviders which are classes specifically designed to provide the correct credit limits for a specific client and user. Decorator and interceptor usage was out of the questions due to how the Consumer is calling the _userService.AddUser method directly. It would still be possible to implement without modifying the Consumer but it would come across as more hacky than elegant.
I love these videos! Quick question about the method extraction at the end on ApplyCreditLimits. Would the assignments to the properties on "user" be considered side-effects? If so, should that be something to avoid? Been struggling a bit with that, especially with all of the F# guys yelling about it. :) Thank you so much for putting these videos together. They are very enjoyable!
Technically we could move that logic up, yeah, and turn the User properties immutable. That being said, it’s not a big issue here at least not as big as it would be in a functional language
I don't know if I'm right, but In my mind the reflection initialization of a factory is a violation of YAGNI principle, because now you definitely don't need it and in the future, you don't know much about how this interface is going to be implemented, so your Activator.CreateInstance can fail anyway.
Folders: You've spread code that is very close over different folders because of using a technological distinction (services/model/repos etc). If you'd group your code based on function (Client, User, Ordering, Shipping) you would keep that together and get an easier to understand layout IMO.
It is very rare that I would split code based on domain because codebases like this are only focusing on one domain only so there is no point to split by domain in this case.
@@nickchapsas Functional split is not only related to domain. There are functional groups that are smaller than domain. To my mind splitting by tech is like having an Enums, Interfaces and Classes folder... (yes, I have seen people actually doing that :-)
@@obiwanjacobi I think I will have to disagree with this one. Obviously splitting by object type is bad, no question about that, but thwne the project is focused on the User Domain only then there is 0 value in sub splitting thing by domain since the domain is infered from the proejct, and having them all top level just because of that, is equally bad.
Nice video Nick! Thanks a lot for your work. I personally would never write that reflection logic. I would either initialize the dictionary explicitly or use a DI Container. You could argue that explicit initialisation also validates the open-closed principle, but or me it is way easier to read und to understand. As this factory is an internal class, I would not value the open closed principle for that class as much. What do you think about that?
Yeah explicitly populating the dictionary would technically be an open-close violation but by no means would I fail the test if someone used it instead of using reflection to load them. Reflection in this case is doing DI's job since we don't have DI in this Legacy .NET Framework 4 codebase. Ideally you would use the IServiceProvider or the IServiceScopeFactory (depending on what you wanna do) to resolve the dependencies by using the ActivatorUtilities class instead of Activator. Internal or Public, it is technically still a violation but not one that would cause you to lose any points. You would just gain more by completely eliminating it.
@@nickchapsas thanks for your reply :) A note on your technique on multiple userservice ctor's using the constructor as a composition root: I had to use something similar to create Powershell Commandlets from C#. They excpect your Commandlet class to provide a parameterless ctor from which an instance is created. So I used (almost) the same pattern as you did with the userservice. I had acess to a DI Container, so i just resolved the dependencies from there instead of newing them up. So there are definitly use cases for this pattern outside from learning examples
@@nickchapsas Isn't part of the difficulty here that the Open-Closed Principle very much by itself in a way violates the YAGNI principle? If we shouldn't look ahead and solve future problems, then we also needn't provide future extensibility, do we? And the difficulty is very much to strike the right balance between the two. Here you are offering extensibility in the way that someone could write another provider and stick it into the same assembly. But tomorrow, someone will want to write a provider that sits outside. Or maybe not. This is the very difficulty of YAGNI.
Oh when you said you were gonna flex a bit I don't know why but I figured you were just gonna use a chain of responsibility to effectively run all the checks in order but blown out into their own classes. Did not see the factory with reflection coming at all lol
Do you ever work on legacy desktop apps (such as WinForms)? I am curious what principles you would apply, and how you would apply them in such a project.
Hey Nick! wouldn't reflection factory become a bit messy when different providers will require more different services for instantiating? What could be other way of solving it?
It would and that's where you'd be introducing a simple dependency injection mechanism/framework instead of the solution that I presented, but the test is about the present not the future. Spending time to solve a problem that we don't have is wasted time.
@@nickchapsas I'm glad you responded to this message. When I saw you developing factory class, I thought it may get messy as well. I too would like to see a better method. I've seen the use of using abstract classes but I'm wondering if there's a way to use interfacing in the same way.
Great videos, definitly got my subscribtion to see new stuff, i honestly wouldnt have done the last part like you did and i would like to see another implementation for it. What do you think of a static validator class instead of injecting? Thanks
Great videos. I would also like to see more on refactoring, including class and folder structure. Would it be an anti-pattern or a bad practice to create a dependency which contains dependencies? For example, creating a LoggingAndMetrics dependency which is a collection containing the individual Logging and Metrics dependencies? dependency
This would be bad yeah because each class should be doing one and only one thing. If just having And in the name means that you're violating that principle
Great demo. Just a little bit of my feedback. I don't think the CreditLimitProviderFactory needs to be injected. It can be created inside the class of UserService. If it is to be injected, then we'd better make it an interface.
Awesome video, I’ve recently been recommend your videos, and they are great! Some really useful and applicable tips! I have one question regarding the reflection usage in the factory class you demoed - Would this make the implementation of the factory class brittle? For example, if you wanted to introduce a new implementation of ICreditLimitProvider, which didn’t require UserCreditService, but required another injected dependency instead, then would this cause a runtime exception when that new ctor was activated, due to it being passed the UserCreditService?
When you used reflection to catch all implementations of the interface, but assumed, that only two constructor variants would be used by them, that was sketchy. Assuming you would have used a dependency injection provider, that could have maybe injected itself into the factory so that the provider can resolved the implementation dependencies. But thinking back, using a dependency injection provider would not have been compatible with the example code, right? Great video!
Answered this a few times already. You cannot use a DI container in this case ins a proper way because of the harness project. Also this is not about predicting the future but refactoring the present. Yes, in an ideal world you would use a DI container but in this test it’s impossible because a test sure says that you can’t break the UserService
Btw after refactoring the logic of VeryImportantClient is changed a little. In the original version user object changes HasCreditLimit and then added. After refactoring it changes 2 properties HasCreditLimit and the CreditLimit. But we don’t know what was in the user object originally and it may cause side-effect if it changed to 0 value :) Not a big deal but might be nightmare during production deployment or warranty. The implementation should return original user.CreditLimit instead of forcing value to 0 for a back compatibility
The logic is the same and the changes are completely intentional and backwards compatible. The CreditLimit is an int in the object that defaults to 0, so there is no change in the logic. There is also a test guarding against any changes for that and it would fail if the logic was different. When a value isn’t set it’s default is used and the default I was that we used in the test as well. I could use the default keyword instead of 0 but it’s the exact same thing and I find 0 more explicit and clear
@@nickchapsas Right. It is a bit difficult to remember all code lines in mind. I forget that the user object is created inside the AddUser. Sorry by that
Hi Nick, first and foremost, thank you for the amazing video! I have a question, during the analysis, I have noticed that variable instatiation often starts with the keyword "var", for instance: var stuff = new Stuff(); other times, the interface is used, for example: IStuff stuff = new Stuff(); Rarely, the implementation class comes in place. Are there any guidelines? When do we need to use the "var" keyword? When the interface?
really good video but @26:20 where you conditionally pass `activator.create-instance` the `user-credit-service`, my concern would be that you loose compile time checks
So at 15;07 you're using a tuple, as an alternative to making a dedicated object. I've been looking online and, as per usual, there are examples but they look nothing like this. Any chance of further explanation here? Thanks
I have a problem with logic inside private methods..... I mean if you wrap the method logic in an interface, you will be able to test the private methods logic, without initializing the dependencies of the current class. You will be forced to deal with mocking the dependencies of the entire class. Even when your private method does not use all properties on your class. Basically, wrapping the private method in an interface on and DI in the behaviour method into the class, will allow you to test the private method, and compose new behaviour into private methods, when change requests hit you. Also simply using an interface on the factory, allows you to extend the factory, without using all the reflection mess. Which makes the code more difficult to read, less efficient to execute, and harder to extend (Say new constructor parameter required). I see what you are doing, but I don't like it for that reason. Especially when you are demoing how and why to extend rather than modify.
Hi, Thank you for giving a video. I'm working on .net MVC , .net core , api but my question was what is the roadmap of .net developer? What should I learn for next level?
Great video series Nick, Question: Could you also have used "Fluent validator" in the "User validator" class? I could imagine you didn't want to use fluent validator in this video, because the focus of this video is refactoring and not how to use Fluent validator.
You absolutely could but I didn't wanna introduce it because I would need to talk about it as well. Same way that I would use MediatR instead of writting my own Factory. When I'm doing tests with such narrow scope, I try to show that I know how to solve a problem myself and don't let a package do it for me since it shows that you understand the problem at its root.
@@nickchapsas Thanks for your reply. It makes sense. Now I'm trying to get my head around on how to convert the factory to using MediatR. :D I think you need a request and request handler for each 'CreditLimitProvider'. For example a 'Default Credit limit Request(handler)' and a 'Important Client Credit Limit Request(handler)'. But how would you write the code that gets the correct request using the 'clientName' string without using the switch statement? Or do you have another approach on using MediatR?
Really cool video, I am curious if anybody would have received only the tests and empty classes for the rest of the project, if there was a way to make all tests pass but still have missing logic.
If you had more time, I think using a DI library like SimpleInjector would have gone a long way in improving the maintainability of your code. Looking at your UTs for example, by the end you had a massive constructor with 6-7 parameters. Using a DI library, you could have instead registered all of your mocks so that everything "news" up for you. Similarly, in your factory class, you're doing a lot of fragile reflection work that will break as soon as you add new provider type with a different constructor pattern (e.g. what if one of your providers needs both the user service and some new service?). That would also be trivially solved using SimpleInjector or a similar library since you'd be able to say "get me all concrete implementations of my interface" to the DI container.
Introducing a DI container would be impossible on this one due to the harness project that prevents us from changing the interface so it’s not a matter of time but rather project limitation. I could technically introduce one and I would probably write a simple one from scratch since this is supposed to be a .NET Framework 4.0, but ultimately this is about passing the test not making something that is 100% perfect.
@@argamon2025 Using a static container would violate DIP in the worst possible way. I would fail someone who would do that. Not only it would use the service locator antipattern but it would introduce it in a static way which is way worse. There is no need for an external language for a small container and there is no need for a container at all. You are asked to refactor the existing code, not predict the future. This is one of the biggest mistakes that engineers that take the test make. Does it ask you to future proof a legacy app? No, it asks you to take the existing code and make it better, and that's that. Remember, you cannot change the Harness project so initializing a container in a "proper" way is impossible here and it will cause more harm than good.
Hi Nick, you extracted that UserValidator @8:43 and injected it as a Concrete implementation instead of an interface. Wouldn't it be more correct if you actually created the interface (and of course had to mock it in the UserService tests) and tested the validator logic separately in its own implementation? Doesn't passing concretions go a bit against Dependency Inversion in SOLID?
When a class have only pure methods and you don't need to mock them, then an Interface is not needed. Exemple : A class that does additions, substraction, multiplication etc... You'll never need to mock them. That's why Math in C# is static :)
What was the reasoning behind specifying the credit and over age requirements in the method names? Something like MeetsCreditThreshold and MeetsAgeRequirement instead, perhaps?
The reasoning is that those methods are not supposed to be reused with different parameters so for readability you can have the requirements in the name. You could also have them as parameters there is nothing wrong with that either
Great video, thank you :-) I have a small query on if you would consider any sort of mapping/automapping between User and Client (on the presumption that this mapping would probably be commonplace throughout the solution)?
@@nickchapsas Makes, sense. Thanks again. Really enjoyed the refactoring content - I found that it seems far more instructive to see bad code being turned into good opposed to seeing clean code being written from scratch. Keep up the good work!
Not agreed with that credit limit provider, yeah you have deal with OCP some smart guy long time ago came up, in some overengineered way, but in the end this code is hard to read, hard to understand, hard to see the full picture and quite easy to write some clash or duplicated providers. Having that there not so much rules in the end it would be better just move it in own method or even class and make it there straightforward. Nowedays in code like that it is not a big deal if you need to make an addition to some class, but it is a big deal to keep code clean and readable which now it is not.
Any Idea where I can find refactoring tests like these to play around with? I'm just getting into dotnet, and it's really cool, and not as difficult as I imagined. I mean I have experience with a lot of other languages and frameworks, so picking up these things goes really quick by now.
Although it didn't break any tests, when you wrote HasValidEmail, you changed the conditions from the original method. The original check ensured that the email address contained both an @ as well as a period. When you implemented HasValidEmail, you checked for an @ OR a period.
Very nice Nick. One question, how would you do that factory class with the build in .Net Core DI, that is implement ICreditLimitProvider multiple times and the mechanism to retrieve the implementations afterwards? What would look like a clean implementation using build in DI containers?
With the built in DI, you would inject the IServiceProvider and use the ActivatorUtilities class instead of the Activator which allows you to resolve all the required instances dynamically from the CI container.
@@nickchapsas Or you could just inject an IEnumerable where the implementations have been registered as ISingleton and then create your dictionary from those as before.
Shouldn't the User entity itself be responsible for protecting it's invariants? I.e. with a constructor which does the firstname/last name guards. Otherwise if you forget to call the UserValidator somewhere, you'll end up with users with no name.
That would be truer if it was a domain object but in this case it is not. Domain and Data layers are coupled so using a separate validator is better here
Why don't you use automock insted creating second constructor that take concrete class, What if this concrete class need another services. Creating them in test will take a lot of work
Nick, thank you for your video. A question from me. Did you write unit tests for each and every condition in the "if" statements? If not, you can potentially (by chance) remove second part of "or" and your tests will be still green. Thank you for an answer.
@@mohankumarsantha1770 Because doing it manually means that every time a new one is added I would also have to manually register it, violating open-closed
@@nickchapsas but each provider should have parameterless or with only user credit service constructor. The factory dictates the conditions for services. Looks very unclear. If you add new client provider with some additional parameter the factory will crash the app at runtime. It's better to use DI container for this. If you have restrictions to adding nuget package so its pretty simple to create your own container in only 21 lines of code stackoverflow.com/questions/15715978/simple-dependency-resolver/15717047#15717047 Actually you can modify this container to create keyed mappings and register each credit service keyed (for important and VIP) and non keyed for default realizations dotnetfiddle.net/2CZ9Px
Excellent video, thank you, Nick! Would it be correct if we also add an additional method overload like `public Result AddUser(CreateUserDto dto)` which implements object parameter pattern and returns Result that may contain some Error message, and we can decorate the old method with an Obsolete attribute so that not to break our clients but inform them about our new more friently method?
I had better write validator witch returns (bool isValid, int? creditLimit). AddUser method would look like: var (isValid, creditLimit) = _validator.Validate(params); If (!isValid) return; var user = new User { ..... HasCteditLimit = creditLimit.HasValue, Limit = creditLimit.HasValue ? creditLimit.Vale : default }; _dal.Save(user); 1. Current method shouldn't know about validation. Validation logic must be unit-tested separately. 2. Use object initializer As a result that method would follow pattern: Validate Create Save Sorry, I'm writing it from phone
Could you inherit the VeryImportantProvider and ImportantProvider from DefaultProvider? Than you would need to mark the methods as virtual so they can be overwritten for these two providers. However lets say in the future you get another request for a NormalProvider which has the same implementation for DefaultProvider, but obviously with is own ClientName and not String.Empty.
Inheritance is evil. I never use it when I can avoid it. Always use composition over inheritance. I would need to override anyway so there would be no reason to use it. Also YAGNI. Designing around a hypothetical future requirement is terrible for a test. You’re not asked to predict the future. You’re ask to improve the present. Also also, even if that’s the case I don’t need to do anything because the provider for that new client name won’t be found and it will just default to the right one.
Nice job overall Nick. A couple of comments regarding other issues that are still in the code (pretending this is the production code that should be maintainable and easy to reason about):
* Code organization. You have folders for Models, Services, Repositories, DataAccess that define the design responsibility of the classes within those folders. And then you add CreditProviders which breaks this pattern as CreditProvider is a business concern. I would suggest not to mix design and business concerns. You could have created a CreditService and put it under the Services directory. That would keep the top-level folder structure clean and consistent.
* Naming sometimes is odd. The method name should say what it is doing and not how. For instance, IsUserIsAtLeast21YearsOld could be something like ‘IsAgeRequirementsMet’. The reason being, if you change the logic inside this method to not allow people over 65 years old, for instance, would you rename the method to IsUserIsAtLeast21YearsAndNotMoreThan65yearsOld? That would be strange. Things will get even scarier when you add several validations inside that method - there is unlikely you will try to squeeze all of them into the method name :)
* User class should have a method ‘SetCreditLimit(credit)’ where hasCredit=true, credit = creditValue get set. Otherwise, good luck troubleshooting situations when somebody does this user.hasCredit=False, user.credit=200. Which property (hasCredit or credit) should the user of this class trust? If ‘SetCreditLimit’ is the only way of updating the credit-related properties, this method will ensure that they are always in sync (if credit > 0, hasCredit = true).
* The dynamic resolution of credit provider is cool and fancy, but in a real application, one would rather prefer things to be more simple and straightforward. Creating a dictionary with all CreditProvider initialized explicitly lets the code reader understand which providers are being used. And what if I create a new provider, and I don’t want it to be used in production yet. Do I have to comment it out to ensure it does not get picked up accidentally?
* A bunch of things like CreditProviders, the constructor for UserService that takes params should be marked as internal.
* The business case was a bit strange here. Using ‘Clients’ and ‘Users’ together. Kind of hard to tell the difference and the relations between them. For instance, method ApplyCreditLimits(client, user)… there is no way a code reader will guess what this method is doing without looking at the implementation.
Again, good job, and I think many people will find this information useful and I didn’t mean to sound negative. I am just talking from 15+ years of personal experience dealing with code of different quality…
Your comments are on point. Thank you, it helps as much as this video.
Some very good points. I don't see this as being negative. Good constructive dialogue
Incredible job Nick but this comment is really useful,
constructive and valuable.
Here's my input on your observations ;)
*Folder structure - nice observation; I keep seeing more and more the domain structuring, instead of code structuring patterns inside the same assembly.
*Naming - a method name should show intention. If you want to extend it then it's time for a rename, or even take the min and max values from a configuration (probable database). Depends if your component is afferent or efferent as well... it may break the design.
*User class should... - totally agree on this one
*The dynamic resolution - agree to disagree... I keep doing this for a few years at least and works like a charm. What Nick didn't cover here is the fact you can have a feature toggle on it that is configured externally (probable database). I'm always trying to make all the stuff configurable so I can disable them if those are not production-ready (yet).
*A bunch of ... - by default all classes should be sealed and internal, except the fact you want to expose them. So yeah... not only ctors... If somebody dares to send a code review request without this covered then it's clear that has an undisputable rejection from me.
*The business case - it is weird... but sometimes this happens even on bigger houses. When you're saying client I'm thinking of the direct consumer (app), that's used by an operator, or even by the final user. So, you may offer extra "stuff" based on the consumer...
I agree with absolutely everything you wrote. Sounds like we’ve been in similar battles and have similar battle scars.
I need more refactoring videos. This way is way easier to understand clean code principles. Thank you!
I'd watch 100s of these refactoring exercises.
This is what I do all day in my day job, and I never get tired of it.
Nick uses the same processes I use, with lots of subtle improvements. I wouldn't have thought of using reflection in a Factory like that, for example.
If I did something like that in enterprise code, I'd leave comments explaining how it works and how another engineer can extend the pattern. It won't be obvious to every maintainer that they get free registration. They might try to head to Startup.cs and add a services.AddScoped() registration.
Nick, thank you for pedagogic and clear tutorial. I am a senior developer with almost 20 years of experience in .NET, but I know for a fact that experience is not only a strength, it also means a challenge. Challenge to evolve, re-learn and reconsider things taken for granted as software tools, paradigms and methodogies change and evolve. Choosing what to keep as part of the past experiences and what to revise is what is the biggest challenge as far as I'm concerned.
I have since quite some time acknowledged the usability of all the patterns and "features" you mention in this video but since there are equally many ways to solve things as there are tools and developers and scenarious - it's often hard to extract the core, the essence of what is what. Details that are features of a specific package or pattern, and what is reasoning on a higher level. It blends together so easily and can be very confusing.
What you manage to do in these two episodes is to provide a compact, pedagogic and chrystal clear explanation on many important refactoring and pattern-related considerations and techniques. You are also doing it in a modest way which increase the credability in what you say: many programmers tend to be overconfident when evangilizing something. While this is showing confidence in what's beaing "preached" it sometimes tend to lead to biased and non-constructive discussions. You leave options for doing more OR less and also reasons why one might choose either way. And you pick one path yourself, motivating it.
I have (many years ago) been educating in programming, methods of programming (not as lecturer but still) and I find it quite hard to keep up with all new patterns, all new technologies etcetera. Still, the core of what they try to accomplish often seem sound and logic one I understand what they try to solve - but it is often hard to get there. Things move too fast, different flavors of patterns (as in different tools using them slightly different) makes it a challenge to see what's what.
What I'm trying to say here is that you are a talent in explaining and showing these concepts and it was fun and informative to watch. I kinda could think "oh, that's a problem" or "you surerly want to do something like..." along the way. Still, there was a constant feel of "ahh, yes, THAT's what they mean when doing like that" or "ahhh, this is why I would do it like that instead if like this". Sort of. In other words - you did precisely what I tried to explain is hard (for me): by concrete and simple example showing the essence of multiple refactoring and testability methods.
Thank you and I'll look forward to see more videos from you!
//Jens
Thank you so much for your comment Jens, I really appriciate you sharing your perspective. Making those videos some times feels like shouting to the void and hoping for someone to listen so it's lovely to know that I am having an impact.
@@nickchapsas You are not shouting to the void indeed! Thanks for the great content.
@@nickchapsas Your videos are very valuable. Please continue making them!
That reflection part went over my head. I guess I need to reflect more on it...
RUclips: Recommends this video
Me: I'm SW professional, I don't need this...
Me, 32 minutes later: wow, thanks I really needed this!
This is exactly the videos I needed as I am working on refactoring (my own) legacy code. Thank you very much.
This is a very nice "by the book" refactoring, so thumbs up for that!
However, I must point out that an 85 line sweet little method now has expanded out to a dozen classes and interfaces and probably close to 500 lines of code including reflection and other goodies.
And it can be argued that it is way less readable now than before for someone unfamiliar with the code. Because you would now need to go through all the interfaces and classes (most of them pretty empty of logic and functionality) and see what is going on to understand it fully. Just to find some complex reflection logic hidden in a factory method somewhere - that points you towards needing to implement an Interface of the right type in order to add a new card type check (if so required).
Before it was pretty simple - expand the if/else statement if required - done. You could see it right away.
So while you have done it "by the book" the KISS principle is in my opinion thoroughly violated here. Because if 85 lines of code gets bloated to 500 - then 10.000 lines of code gets bloated to 50.000. And I rather have 10.000 "not so great" lines to read than 50.000 super advanced "by the book" stuff to read and wrap my head around.
My friend, your are either an old-timer or you *really* didn't understand KISS...
It is now, by any metrics possible, more readable than before. You say that it's more difficult if you want to see every part of the system.
Yes it is, but why should you ? It's called abstraction. You write high-level statements, so that *only* if needed you go see the details.
Before, you were attacked by those same details ^^
Simplicity is not the number of lines total. Simplicity is the cognitive charge needed to do the simpliest change in a method.
When that method goes to 2k lines, trust me, this is better. Because you'll understand the macro functioning way faster.
More info in the book "Clean Code"
Finally someone who brings something TOTALLY DIFFERENT FROM TRADITIONAL VIDEOS.
Thanks a lot. You're amazing
Thanks Nick! It's really refreshing to see a tutorial focused on refactoring. I much appreciate this.
One of the best videos on the topic of refactoring!
Really appreciated. Thanks mate!
Wow! You are the real deal. This video was highly educational. As someone who’s been programming for years but somewhat new to .Net development, this refactoring approach is exactly what I need to fill in the gaps of my design principals knowledge. The reflection portion was something I would’ve never though of and I had to watch it a few times to fully understand how it works. Very useful!
This is the kind of video that helps to C# community. I really appreciate your work, thanks !
One of the best youtube channel about how .NET works. I really recommend to all .NET developers watch his videos, you'll find many helpful info. It doen't matter are u Junior,Middle or Senior.
Great video. Gonna learn more about providers because this part was hard for me to understand.
What I like about this video is that it shows practically how tests can help your workflow.
You are such a guru Nick. Ive never seen anyone else explain these things in such a easy to understand way. I will definately start implementing these teachings more and more to my code. That reflection topic needs to be covered on a different video since it just blew me away because of that syntax alone
Great series, excellent channel. Just a small suggestion for others watching this video. Try to avoid using “var” when on the right hand side you have a method that returns something. Because if someone is reading the code outside of an IDE he wouldn’t know what the return type is without opening the definition of that method. Example:
Worse:
var client = _something.GetClientById(id);
var someName = “John”;
Better:
UserClient client = _something.GetClientById(id);
var someName = “John”;
Notice that I didn’t change the second var because it’s obvious what type it is by just reading the code, but the first is not.
I disagree. Unless you need to use an implicit operator you shouldn't use var. For example what you described as "Better" to me is way worse and redundant. I can already see what the var is. It is a client. I can tell both by the name of the variable and the name of the method it's calling. Having it in the declaration too is pointless.
@@nickchapsas how do you know it’s a *user* client from the first example? Trust me I’ve read a lot of code like this and I had to jump across the code base for something that should’ve been obvious just by a glance. Now maybe the example I quickly wrote on my phone is not the best, but i hope you get the idea of where I’m going with it. Also if you’re using an IDE and can just hover on things and get a description, it’s a non-issue, but if you’re reading code (which is what you do most of the time) on GitHub (for example) it becomes a lot harder. Anyway let’s agree to disagree :) I like your content, keep up the good work!
Great video (as always)
What I would change, however, is transfer the logic of chosing the right ICreditLimitProvider on the ICreditLimitProvider itself.
So, instead of having a string NameRequirement property, I'd add a bool IsAvailableForClient(string) method (or even Client object as parameter).
That way, you wouldn't need to add a new provider for each new unlimited client for example.
You could even (depending on performance requirements, api calls and all), have multiple available providers, call GetCreditLimits for each of them and take the best one.
Wow, just wow Nick! Your knowledge in coding blows my mind away. Thanks for being kind and sharing with us.
These are the best refactoring videos in youtube. Thank u Nick. Please continue this series. want to learn more from u.
wow that was some codegasm. Thanks for your awesome videos. 23 years of coding, managing project and running my own business, your videos teach me something new every time.
Great Video series Nick. learned a great deal from these two videos. thanks a lot.
Great video and honestly I would love more videos like this. I know that these videos can take a long time to make, but I really appreciate that you go over and don't spare anything just for time. Great job! It really shows a realistic scenario of refactoring code in a real codebase.
As a student (who hasn't been programming in C#, but Java is close enough I suppose :) ) this was very enlightening. Really appreciate the way you explain your thought process while going over the code and the way you structure your approach.
Great videos Nick. I am .net developer of many years experience and I find your vids very clear, very informative and very helpful. Keep up the good work
I just love your videos nick. Your explanation is awesome!
Excellent video. The refactoring series are gold.
Once you mentioned 'it is hard to understand a Not check for something with negative in the name' I noticed just how often I was doing that in my own code, and how much I hated that.
Good points all around. BTW I am watching to see if my interview questions were too hard!
great job, i need to learn more about factories
Haha nice effort (especially the first vid on testing), but I bet you got a bit too enterprisy at the end 😂😂😂
I know it's really hard to hit the sweetspot between testable modularity and expressiveness, so no judgenent. But a little domain model of "RequestForCredit" would have really helped to encapsulate the real business logic and do the service / provider / validator / factory mess somewhere else 😂
But anyways, the outlined skills get C# devs hired, so thank you very much for sharing this vid 😂
Looking forward to rest of this series, gotta say your thumbnail game is on point!
This series finally convinced me to drop my patreon sub to my other favourite .NET content creator and instead sign up to yours
Reflection is cool! We are using this method in our projects, since 8 years.
Why so many developers "hate" refactoring is beyond me. This is super interesting! Thanks for making these videos!
It is great. I love it. You do it in a great way. It is fun to learn from you. Thanks that you do that.
My first time to see the factory pattern implemented end to end.
Now I have to be able to do it on my own.
loved this! subscribed!
Great video!
Just one more annotation how even without using named parameters changing the parameter name might be breaking:
If in some other part of the system reflection is used on this class for some reason, that would also potentially be breaking.
Nice. I would have injected the ICreditLimitProviders as an IEnumerable and just find and use the right provider based on ClientName. Although this would mean that the AddUser method would need to know how to select the right provider, which could be inappropriate intimacy code smell.
Oh yeah. Thank you Nick. I also use that reflection method to get all interfaces prior to O/C principles, though reflection might go slower during loading of all class with that interfaces, it will generate you an instance by name will get faster
That original startup time is so negligable that to be isn't even a point of discussion but I thought I would mention it in the video because people sometimes think that reflection is as slow as it was years ago
agree agree Nic :D , believe to the speed of .NetCore today
Amazing Nick. Very nice and detailed as usual and this comes from real experience. Still keep them coming
Happy to find your channel on my feed. Thanks
Amazing skill … I enjoy your videos but this one is a hit… thank you.
Awasome job with Assembly coding. Thanks for ideas!
Thank you Nick! Great video!
I would avoid using reflection when I can, because it makes code harder to follow. But sometimes, it can be really useful.
Exactly what I would do. Thumb up mate.
Hey Nick can you do a project without using the LID of SOLID to identify how you dont need LID to build maintainable dotnet project and see if you notice a lack of code pollution
These are great because they apply to my gamedev code as well!
After watching this part I have a couple of comments. First one is about a placing credit logic inside credit validator. For me it looks wrong that validator decides what is a user credit (single or twice). I would say it should be a part of user credit service. The second one is credit validation should be a part of all other validation happens before adding user. Plus I would prefer to design validation logic as method decorator or interceptor pattern. It would be cool to see it in your next videos
Which credit validator? There is no credit validator in this video. The credit logic (none, double, single) is in the CreditLimitProviders which are classes specifically designed to provide the correct credit limits for a specific client and user. Decorator and interceptor usage was out of the questions due to how the Consumer is calling the _userService.AddUser method directly. It would still be possible to implement without modifying the Consumer but it would come across as more hacky than elegant.
26:43 I am gonna stop you right there. I am officially lost. What are we doing again?
Anyway, great video, love it.
I love these videos! Quick question about the method extraction at the end on ApplyCreditLimits. Would the assignments to the properties on "user" be considered side-effects? If so, should that be something to avoid? Been struggling a bit with that, especially with all of the F# guys yelling about it. :) Thank you so much for putting these videos together. They are very enjoyable!
Technically we could move that logic up, yeah, and turn the User properties immutable. That being said, it’s not a big issue here at least not as big as it would be in a functional language
hi @nick Did you ever create videos for the rest of the SOLID (LID?) I watched the first two, and they were excellent. I didn't see the rest though,
I don't know if I'm right, but In my mind the reflection initialization of a factory is a violation of YAGNI principle, because now you definitely don't need it and in the future, you don't know much about how this interface is going to be implemented, so your Activator.CreateInstance can fail anyway.
Folders: You've spread code that is very close over different folders because of using a technological distinction (services/model/repos etc).
If you'd group your code based on function (Client, User, Ordering, Shipping) you would keep that together and get an easier to understand layout IMO.
It is very rare that I would split code based on domain because codebases like this are only focusing on one domain only so there is no point to split by domain in this case.
@@nickchapsas Functional split is not only related to domain. There are functional groups that are smaller than domain.
To my mind splitting by tech is like having an Enums, Interfaces and Classes folder... (yes, I have seen people actually doing that :-)
@@obiwanjacobi I think I will have to disagree with this one. Obviously splitting by object type is bad, no question about that, but thwne the project is focused on the User Domain only then there is 0 value in sub splitting thing by domain since the domain is infered from the proejct, and having them all top level just because of that, is equally bad.
Nice video Nick! Thanks a lot for your work.
I personally would never write that reflection logic.
I would either initialize the dictionary explicitly
or use a DI Container.
You could argue that explicit initialisation also validates the open-closed principle, but or me it is way easier to read und to understand.
As this factory is an internal class, I would not value the open closed principle for that class as much.
What do you think about that?
Yeah explicitly populating the dictionary would technically be an open-close violation but by no means would I fail the test if someone used it instead of using reflection to load them. Reflection in this case is doing DI's job since we don't have DI in this Legacy .NET Framework 4 codebase. Ideally you would use the IServiceProvider or the IServiceScopeFactory (depending on what you wanna do) to resolve the dependencies by using the ActivatorUtilities class instead of Activator. Internal or Public, it is technically still a violation but not one that would cause you to lose any points. You would just gain more by completely eliminating it.
@@nickchapsas thanks for your reply :)
A note on your technique on multiple userservice ctor's using the constructor as a composition root:
I had to use something similar to create Powershell Commandlets from C#. They excpect your Commandlet class to provide a parameterless ctor from which an instance is created.
So I used (almost) the same pattern as you did with the userservice.
I had acess to a DI Container, so i just resolved the dependencies from there instead of newing them up.
So there are definitly use cases for this pattern outside from learning examples
@@nickchapsas Isn't part of the difficulty here that the Open-Closed Principle very much by itself in a way violates the YAGNI principle? If we shouldn't look ahead and solve future problems, then we also needn't provide future extensibility, do we? And the difficulty is very much to strike the right balance between the two. Here you are offering extensibility in the way that someone could write another provider and stick it into the same assembly. But tomorrow, someone will want to write a provider that sits outside. Or maybe not. This is the very difficulty of YAGNI.
Oh when you said you were gonna flex a bit I don't know why but I figured you were just gonna use a chain of responsibility to effectively run all the checks in order but blown out into their own classes. Did not see the factory with reflection coming at all lol
absolutely amazing
Do you ever work on legacy desktop apps (such as WinForms)? I am curious what principles you would apply, and how you would apply them in such a project.
I do not. I almost exclusively work with microservices in .NET Core+. The principles can be applied but the way of application can vary.
Goosebumbs when code is refactored and all tests are passing.
Hey Nick! wouldn't reflection factory become a bit messy when different providers will require more different services for instantiating? What could be other way of solving it?
It would and that's where you'd be introducing a simple dependency injection mechanism/framework instead of the solution that I presented, but the test is about the present not the future. Spending time to solve a problem that we don't have is wasted time.
@@nickchapsas I'm glad you responded to this message. When I saw you developing factory class, I thought it may get messy as well. I too would like to see a better method. I've seen the use of using abstract classes but I'm wondering if there's a way to use interfacing in the same way.
Great videos, definitly got my subscribtion to see new stuff, i honestly wouldnt have done the last part like you did and i would like to see another implementation for it. What do you think of a static validator class instead of injecting? Thanks
Great videos. I would also like to see more on refactoring, including class and folder structure.
Would it be an anti-pattern or a bad practice to create a dependency which contains dependencies?
For example, creating a LoggingAndMetrics dependency which is a collection containing the individual Logging and Metrics dependencies?
dependency
This would be bad yeah because each class should be doing one and only one thing. If just having And in the name means that you're violating that principle
Great demo.
Just a little bit of my feedback.
I don't think the CreditLimitProviderFactory needs to be injected. It can be created inside the class of UserService.
If it is to be injected, then we'd better make it an interface.
Awesome video, I’ve recently been recommend your videos, and they are great! Some really useful and applicable tips!
I have one question regarding the reflection usage in the factory class you demoed -
Would this make the implementation of the factory class brittle?
For example, if you wanted to introduce a new implementation of ICreditLimitProvider, which didn’t require UserCreditService, but required another injected dependency instead, then would this cause a runtime exception when that new ctor was activated, due to it being passed the UserCreditService?
When you used reflection to catch all implementations of the interface, but assumed, that only two constructor variants would be used by them, that was sketchy. Assuming you would have used a dependency injection provider, that could have maybe injected itself into the factory so that the provider can resolved the implementation dependencies. But thinking back, using a dependency injection provider would not have been compatible with the example code, right? Great video!
Answered this a few times already. You cannot use a DI container in this case ins a proper way because of the harness project. Also this is not about predicting the future but refactoring the present. Yes, in an ideal world you would use a DI container but in this test it’s impossible because a test sure says that you can’t break the UserService
Btw after refactoring the logic of VeryImportantClient is changed a little. In the original version user object changes HasCreditLimit and then added. After refactoring it changes 2 properties HasCreditLimit and the CreditLimit. But we don’t know what was in the user object originally and it may cause side-effect if it changed to 0 value :) Not a big deal but might be nightmare during production deployment or warranty. The implementation should return original user.CreditLimit instead of forcing value to 0 for a back compatibility
The logic is the same and the changes are completely intentional and backwards compatible. The CreditLimit is an int in the object that defaults to 0, so there is no change in the logic. There is also a test guarding against any changes for that and it would fail if the logic was different. When a value isn’t set it’s default is used and the default I was that we used in the test as well. I could use the default keyword instead of 0 but it’s the exact same thing and I find 0 more explicit and clear
@@nickchapsas Right. It is a bit difficult to remember all code lines in mind. I forget that the user object is created inside the AddUser. Sorry by that
Great content thanks
Hi Nick, first and foremost, thank you for the amazing video!
I have a question, during the analysis, I have noticed that variable instatiation often starts with the keyword "var",
for instance:
var stuff = new Stuff();
other times, the interface is used, for example:
IStuff stuff = new Stuff();
Rarely, the implementation class comes in place.
Are there any guidelines? When do we need to use the "var" keyword? When the interface?
Should I use autofac instead of creating my own factory or should I create a factory to show ability?
do u provide the code for beginners to play around? Or we must be a patreon ?
Love it.
really good video but @26:20 where you conditionally pass `activator.create-instance` the `user-credit-service`, my concern would be that you loose compile time checks
So at 15;07 you're using a tuple, as an alternative to making a dedicated object. I've been looking online and, as per usual, there are examples but they look nothing like this. Any chance of further explanation here? Thanks
I have a problem with logic inside private methods.....
I mean if you wrap the method logic in an interface, you will be able to test the private methods logic, without initializing the dependencies of the current class.
You will be forced to deal with mocking the dependencies of the entire class. Even when your private method does not use all properties on your class.
Basically, wrapping the private method in an interface on and DI in the behaviour method into the class, will allow you to test the private method, and compose new behaviour into private methods, when change requests hit you.
Also simply using an interface on the factory, allows you to extend the factory, without using all the reflection mess. Which makes the code more difficult to read, less efficient to execute, and harder to extend (Say new constructor parameter required).
I see what you are doing, but I don't like it for that reason. Especially when you are demoing how and why to extend rather than modify.
great video
Hi,
Thank you for giving a video.
I'm working on .net MVC , .net core , api but my question was what is the roadmap of .net developer? What should I learn for next level?
Why and when do you use var instead of a more concrete type?
Great video series Nick,
Question: Could you also have used "Fluent validator" in the "User validator" class? I could imagine you didn't want to use fluent validator in this video, because the focus of this video is refactoring and not how to use Fluent validator.
You absolutely could but I didn't wanna introduce it because I would need to talk about it as well. Same way that I would use MediatR instead of writting my own Factory. When I'm doing tests with such narrow scope, I try to show that I know how to solve a problem myself and don't let a package do it for me since it shows that you understand the problem at its root.
@@nickchapsas Thanks for your reply. It makes sense.
Now I'm trying to get my head around on how to convert the factory to using MediatR. :D
I think you need a request and request handler for each 'CreditLimitProvider'. For example a 'Default Credit limit Request(handler)' and a 'Important Client Credit Limit Request(handler)'.
But how would you write the code that gets the correct request using the 'clientName' string without using the switch statement? Or do you have another approach on using MediatR?
@@Timlaare You can just use a dictionary to get rid of that switch or if you want to be fancy use a strategy pattern.
Really cool video, I am curious if anybody would have received only the tests and empty classes for the rest of the project, if there was a way to make all tests pass but still have missing logic.
The tests were added by me in the previous video to ensure that the logic didn’t break. You are judged on the refactoring not the tests
@@nickchapsas i understand, and the refactoring was very elegant
If you had more time, I think using a DI library like SimpleInjector would have gone a long way in improving the maintainability of your code. Looking at your UTs for example, by the end you had a massive constructor with 6-7 parameters. Using a DI library, you could have instead registered all of your mocks so that everything "news" up for you.
Similarly, in your factory class, you're doing a lot of fragile reflection work that will break as soon as you add new provider type with a different constructor pattern (e.g. what if one of your providers needs both the user service and some new service?). That would also be trivially solved using SimpleInjector or a similar library since you'd be able to say "get me all concrete implementations of my interface" to the DI container.
Introducing a DI container would be impossible on this one due to the harness project that prevents us from changing the interface so it’s not a matter of time but rather project limitation. I could technically introduce one and I would probably write a simple one from scratch since this is supposed to be a .NET Framework 4.0, but ultimately this is about passing the test not making something that is 100% perfect.
@@argamon2025 Using a static container would violate DIP in the worst possible way. I would fail someone who would do that. Not only it would use the service locator antipattern but it would introduce it in a static way which is way worse. There is no need for an external language for a small container and there is no need for a container at all. You are asked to refactor the existing code, not predict the future. This is one of the biggest mistakes that engineers that take the test make. Does it ask you to future proof a legacy app? No, it asks you to take the existing code and make it better, and that's that. Remember, you cannot change the Harness project so initializing a container in a "proper" way is impossible here and it will cause more harm than good.
Hi Nick, you extracted that UserValidator @8:43 and injected it as a Concrete implementation instead of an interface. Wouldn't it be more correct if you actually created the interface (and of course had to mock it in the UserService tests) and tested the validator logic separately in its own implementation? Doesn't passing concretions go a bit against Dependency Inversion in SOLID?
When a class have only pure methods and you don't need to mock them, then an Interface is not needed.
Exemple :
A class that does additions, substraction, multiplication etc... You'll never need to mock them.
That's why Math in C# is static :)
You're amazing
What was the reasoning behind specifying the credit and over age requirements in the method names? Something like MeetsCreditThreshold and MeetsAgeRequirement instead, perhaps?
The reasoning is that those methods are not supposed to be reused with different parameters so for readability you can have the requirements in the name. You could also have them as parameters there is nothing wrong with that either
Great video, thank you :-) I have a small query on if you would consider any sort of mapping/automapping between User and Client (on the presumption that this mapping would probably be commonplace throughout the solution)?
Not in this use case no. Mapping shouldn’t be related to business logic and if introduced here it could hide domain related details.
@@nickchapsas Makes, sense. Thanks again. Really enjoyed the refactoring content - I found that it seems far more instructive to see bad code being turned into good opposed to seeing clean code being written from scratch. Keep up the good work!
I think the email check you wrote is wrong :D
Not agreed with that credit limit provider, yeah you have deal with OCP some smart guy long time ago came up, in some overengineered way, but in the end this code is hard to read, hard to understand, hard to see the full picture and quite easy to write some clash or duplicated providers. Having that there not so much rules in the end it would be better just move it in own method or even class and make it there straightforward.
Nowedays in code like that it is not a big deal if you need to make an addition to some class, but it is a big deal to keep code clean and readable which now it is not.
Any Idea where I can find refactoring tests like these to play around with? I'm just getting into dotnet, and it's really cool, and not as difficult as I imagined. I mean I have experience with a lot of other languages and frameworks, so picking up these things goes really quick by now.
Although it didn't break any tests, when you wrote HasValidEmail, you changed the conditions from the original method. The original check ensured that the email address contained both an @ as well as a period. When you implemented HasValidEmail, you checked for an @ OR a period.
Very nice Nick. One question, how would you do that factory class with the build in .Net Core DI, that is implement ICreditLimitProvider multiple times and the mechanism to retrieve the implementations afterwards? What would look like a clean implementation using build in DI containers?
With the built in DI, you would inject the IServiceProvider and use the ActivatorUtilities class instead of the Activator which allows you to resolve all the required instances dynamically from the CI container.
@@nickchapsas Or you could just inject an IEnumerable where the implementations have been registered as ISingleton and then create your dictionary from those as before.
What is the desin pattern called that you use to refactor LimitProviders?
It's the factory method creational design pattern.
Shouldn't the User entity itself be responsible for protecting it's invariants? I.e. with a constructor which does the firstname/last name guards. Otherwise if you forget to call the UserValidator somewhere, you'll end up with users with no name.
That would be truer if it was a domain object but in this case it is not. Domain and Data layers are coupled so using a separate validator is better here
Why don't you use automock insted creating second constructor that take concrete class, What if this concrete class need another services. Creating them in test will take a lot of work
Nick, thank you for your video. A question from me. Did you write unit tests for each and every condition in the "if" statements? If not, you can potentially (by chance) remove second part of "or" and your tests will be still green. Thank you for an answer.
Hey, which part exactly are you refering to by timestamp?
@@nickchapsas thank you for a quick reply, ruclips.net/video/Yd4GnWeEkIY/видео.html, loc 100
User creation can also move in user class itself with new createuser method?
insted of reflection why don't we create a simple dictionary for getting matched provider?
You mean a manually populated dictionary?
@@nickchapsas yes
@@mohankumarsantha1770 Because doing it manually means that every time a new one is added I would also have to manually register it, violating open-closed
@@nickchapsas agree 👍 your awesome 👏👏
@@nickchapsas but each provider should have parameterless or with only user credit service constructor. The factory dictates the conditions for services. Looks very unclear. If you add new client provider with some additional parameter the factory will crash the app at runtime.
It's better to use DI container for this. If you have restrictions to adding nuget package so its pretty simple to create your own container in only 21 lines of code stackoverflow.com/questions/15715978/simple-dependency-resolver/15717047#15717047
Actually you can modify this container to create keyed mappings and register each credit service keyed (for important and VIP) and non keyed for default realizations dotnetfiddle.net/2CZ9Px
Hello, thank for the video.
Is it okay to change parameter properties like you do in "ApplyCreditLimits" method (31:09)
Excellent video, thank you, Nick! Would it be correct if we also add an additional method overload like `public Result AddUser(CreateUserDto dto)` which implements object parameter pattern and returns Result that may contain some Error message, and we can decorate the old method with an Obsolete attribute so that not to break our clients but inform them about our new more friently method?
There is no reason to do that in this use case since we can’t change the Consumer project at all, so adding that would violate YAGNI
I had better write validator witch returns (bool isValid, int? creditLimit).
AddUser method would look like:
var (isValid, creditLimit) = _validator.Validate(params);
If (!isValid) return;
var user = new User
{
.....
HasCteditLimit = creditLimit.HasValue,
Limit = creditLimit.HasValue ? creditLimit.Vale : default
};
_dal.Save(user);
1. Current method shouldn't know about validation. Validation logic must be unit-tested separately.
2. Use object initializer
As a result that method would follow pattern:
Validate
Create
Save
Sorry, I'm writing it from phone
Gr8
Could you inherit the VeryImportantProvider and ImportantProvider from DefaultProvider? Than you would need to mark the methods as virtual so they can be overwritten for these two providers. However lets say in the future you get another request for a NormalProvider which has the same implementation for DefaultProvider, but obviously with is own ClientName and not String.Empty.
Inheritance is evil. I never use it when I can avoid it. Always use composition over inheritance. I would need to override anyway so there would be no reason to use it. Also YAGNI. Designing around a hypothetical future requirement is terrible for a test. You’re not asked to predict the future. You’re ask to improve the present. Also also, even if that’s the case I don’t need to do anything because the provider for that new client name won’t be found and it will just default to the right one.
@@nickchapsas Thanks for the detailed reply, and food for thought
Decorator pattern is perfect for that.
I would agree with Nick that inheritance is hell