On the very last mention of the video, about whether or not to give advice on sealing classes, I think it's a rather controversial but very good point to make, not just on library code but any code. Not sealing non abstract classes implies they're ok to inherit from, which in turn implies the inheritance model is ok to favor over the composition model. The only time I think this can be tolerated is when writing prototypical code (composition model generally requires a bit more effort to set up, which you may not have if you're coding against time). But since this video is about writing better code (aka non prototypical), the advice to always seal non abstract classes is a good one that should be widely adopted.
I like good XML doc but the issue with them is that they're hard to keep maintained and small teams will have trouble keeping them up to date. (Especially the list of exceptions a method may throw: if you change the implementation of a method, it may invalidate its own exception list as well as everything that calls it, and nothing will tell you about it.) You can't forget a failing test, but it's very easy to forget to go back and update the XML doc after you've made a change.
@@sonicbhoc Agreed, I work on philosophy now of being mindful of what I'm documenting. For example a long list of boilerplate exceptions can safely sit outside of documentation. I feel its sufficient to say 'this can not be null or whitespace' in the property definition.
Regarding the access to methods/classes in your library, Challenge is how should a developer determine which one to make public? The answer that I have to that is that you write TDD. That will automatically guide you what needs to be made public. Once a developer becomes efficient with TDD, you'd get many many benefits and having a proper scope is one of them.
For the access keywords you can make to much private or internal too. It's so frustrating when a library does something like make a public interface that returns a public type, but the type has an internal constructor, and no other kind of factory etc to make it so you can't really mock it out even though the interface is public because you can't create the object to be returned without reflection. It's really just about thinking carefully about what should be public and what shouldn't. Also what the extensibility points should be, what should be virtual and what should be sealed if inheritance is used, etc.
You should think carefully about it and take great care when designing your library's public surface area. It is safer to start with private/internal/sealed than it is to start with public because you change your mind. Unsealing/making something public is not a breaking a change and can be done in a minor release. Going the other way around can be very painful for you and the users of your library with breaking changes. You can get stuck where your unable to do some important refactoring/optimizations/fixes because too much of your library is public. I advise that if there is a part of your library that you are unsure about it being public/unsealed, you can err towards less accessibility and then listen closely to your user's feedback.
@@Denominus yes, that's a good idea to avoid breaking changes. The thinking about it is the important part though. If your unit test project has internal access then you might be making a project that can't actually be used or can't be tested by a consumer that only has public access. You can make things public later, but it should be both usable and testable all the time. Library authors can also be understandably reluctant to make things public later, which can cause problems for consumers. There is no substitute for just taking the time to design a good public API.
Hi nick, could you create an episode regarding the libraries for importing and exporting excel. For enterprise work, excel reading, manipulation and save it to db is ever presence scenario
IMO, some developers get in a habit of using 'public' as a way to make it easy to unit test methods/classes. I do not agree with that approach but I can see how it becomes a (bad) habit.
@@nickchapsas Wouldn't that be wrong to do? If you are testing how others will interact with our api then you use should rely on public interface of your object ( public methods). Also assuming one is writing TDD, you'd automatically end up having the interaction in you test case that is public. Making internals visible to your test takes away a very big benefit of testing which is see the friction in your code when you are writing tests. Writing tests isn't just about making sure it does what it supposed to do but also make your code clean and see the friction in code. With internal you can very easy skip that friction. Others using your api won't be able to and therefore I think your dlls should be public to test project and not internal.
Thanks for talking about library code as well. most stuff out there only talks about application code and structure. While talking about structure would you know any good sources on that? Bcs that is what I am mostly struggling with, structuring my libraries
Just place yourself in the library consumer role and think how you would like the public API be, so it is easy to start with and use. You shouldn't worry much about anything else at least at start.
Hi Nick, I’m currently programming a rather small library, which uses DI and Microsoft.Extensions.DependencyInjection.Abstractions to let the consumers use the DI Container of their choice. Is that a valid approach or are there some downsides I’m not aware of? Best Regards
Hi, great video, as always! Do you have any plan to make a video about different dependency injection containers and why would you use one over the other?
Absolutely. There are so many that I’m currently filtering out the ones that are actually popular and worth talking about but there is a video about that coming
Hey Nick, Firstly love the video. I, however, disagree with not wrapping "common" libraries that use statics (@2:15 NewtonSoft.Json). IMO these need to always be wrapped, so that during my unit test of the object under test (in your example the Controller Action) i can return any value. This makes it so that I am not reliant on valid input to get an output from that dependency. IMO even NewtonSof.Json should be interfaced.
I disagree. There is no scenario where I don’t want json.net to return what it will actually return. Mocking it is you just lying in your tests and your tests will be less reliable because of it
Are you lying in the test or testing a different area of code than intended? If you call a static method from within your code (regardless of who created the static method) you are leaving the code under test. You are also placing responsibility on the test to understand how the static method works, so that the static method operates as expected. Adding a simple one line wrapper around the static method, allows the unit test to control the input and return objects. This makes it so that the test isn't dependent on an external concrete implementation. An added benefit of wrapping is during verification the test can verify the correct input was passed in. Also, if applicable, the test can use a reference check to make sure the object was passed around appropriately within the method (from one dependency to another). Wrapping up common packages gives several benefits (which i think you covered in another video if I remember correctly). Firstly, it allows easier substitution if the backing package no long suits your needs. It also allows for your application to "Trust" the wrapped package (and hope the developer had good testing practices).
I noticed that your code scrolling is very smooth while at the same time your mouse pointer is always very precise and on target, especially when selecting code, your pointer remains very straight horizontally, like you would be using a mouse. A trackpad is more difficult to draw a straight line with. I'm curious, if you're using a mouse, what mouse is that? I've been searching for a mouse that scrolls as smooth as a trackpad on Windows for ages and I think I might never find one.
Since the name of the video is "Better library code", I'd like to point out that your code is taking a dependency and not checking if its null. I understand that you are sharing different concepts but I think the topics that deal with showing better code should proper code. I'm sure in your Prod code, you are not taking a dependency and not checking for null :-). This code will blow up if I write a test case for it by passing null to constructor on the method and not on constructor.
I have a video about this. You can mark the internals of a project as visibly on the assembly level for unit testing purpose only, which is a perfectly acceptable case.
Just as an FYI, unit tests are tests that cover a "module" of code. You've probably heard that it should cover a single class, but this is actually not accurate, it's simply one interpretation of "module." Your unit tests should describe the external behavior of a piece of code and how it is implemented is irrelevant. As far as a test is concerned, whether your code is implemented as a single class or many internal classes isn't important: just what the code does is.
@@nickchapsas Ah cool, I didn't know that. Thanks!
3 года назад
Everything should be public with a good documentation, how many times I needed to use reflection to call a private method inside an internal class or had to copy a lot of code from github/disassembly because I could not call a method that does exactly what I needed. Just document what can be changed, let me use WHAT I WANT, man that sucks.
If you needed reflection to call internal or private methods then you used the library wrong. There is a reason why things are internal or private and using reflection to call them is a really bad practice.
3 года назад
@@nickchapsas I know what I am doing because I read the code and I manage my library versions, The overall advice is ok but not always true.
@@nickchapsas I must agree with Márcio -- let me use any code, I am adult enough to understand that the surface of any API is its Interfaces not its classes. But we all are notorious bad at predicting how things are going to be used (or there impact). I do not want to reinvent all your code just because you did not think of my use case -- happened with me with WebApi2 OData code.
@@CabbageYe Rarely should comments (i.e. // or */) be needed. You should always strive to write code that is self-explanatory. However, code documentation (i.e. ///) is another thing. The name of public classes, methods, properties and events do not always contain the necessary information required to fully understand what they do. To save the user from peeking into the source, or look up some public documentation on a website, please write short and insightful documentation on all public things in your API. This is also good practise if you are working on a team with others developers. I have also often found myself benefitting from my own documentation given enough time has passed where I have forgotten the details.
I like most of what you say, but I disagree with your assertion about most things in a library should be "private" or "internal". There's no way your library can anticipate every use-case that a downstream user will have. If you hide the building blocks, your users lose the ability to compose new solutions that you hadn't thought of before. Think very long and hard before you make something inaccessible to the user, you might be making life a lot harder for them. I far prefer approaches where we either (a) mention alternatives in the comments so basic users don't accidentally use something for advanced users, or (b) use namespaces to separate advanced use-cases. The "MyLibrary" root namespace should include all the abstractions and interfaces for basic use, where nested "MyLibrary.AdvancedTopic" namespaces can contain the more advanced building blocks. That way users can choose which parts of the library they want to see, and your documentation to guide them.
I would revert your answer completely and I completely disagree ok every level. If you don’t anticipate how your library should be used by your user then you shouldn’t ship it at all. Anything exposed as part of the public api are part of your contract with the user. If you can’t tunnel users to a specific guided path then you should design your library to do so. Think very long and hard before making something accessible to the user. Making it accessible is a commitment by you to them that this will not change.
@@nickchapsas You can't anticipate every use-case, it's foolish to try. You can and should try to guide a user to the correct path, but that doesn't mean you should cut off every other path. You should prefer composition over inheritance, and the only way to have composition is if you give users access to the building blocks that you used to compose solutions yourself. I see a lot of problems in libraries that didn't expose something necessary because of over-aggressive use of "private" and "internal", and those kinds of problems stop the library from being used at all.
@@wknight8111 Every single good library I've ever used does that very same thing and I can guarantee that it's libraries you are using too and you don't even know about it. Anything public in libraries like MediatR, Automapper, FluentAssertions, FluentValidator and tens of others are only exposing specifically what they need to. These are well designed libraries. If they wanted to seal something, there is a reason. If they marked something as private, there was a reason and if something was internal, there was a reason. There are really no points to be made here. If you think something should be exposed in a well designed library, then you are using the library wrong.
You summarized some of the most important things I've learned during my years of working with .NET services in 10 minutes. Really great video!
Thank you for advocating XML docs. All libraries should have these.
On the very last mention of the video, about whether or not to give advice on sealing classes, I think it's a rather controversial but very good point to make, not just on library code but any code. Not sealing non abstract classes implies they're ok to inherit from, which in turn implies the inheritance model is ok to favor over the composition model. The only time I think this can be tolerated is when writing prototypical code (composition model generally requires a bit more effort to set up, which you may not have if you're coding against time). But since this video is about writing better code (aka non prototypical), the advice to always seal non abstract classes is a good one that should be widely adopted.
In one line, I LOVE YOUR VIDEOS. YOU ARE THE BEST.
Very good vid. I believe youtube is lacking the topic of Kestrel and IIS. I would love to hear something on that from you :)
I like good XML doc but the issue with them is that they're hard to keep maintained and small teams will have trouble keeping them up to date. (Especially the list of exceptions a method may throw: if you change the implementation of a method, it may invalidate its own exception list as well as everything that calls it, and nothing will tell you about it.) You can't forget a failing test, but it's very easy to forget to go back and update the XML doc after you've made a change.
It needs to be part of the process. I'd have someone give them a once over during the code review.
@@sonicbhoc Agreed, I work on philosophy now of being mindful of what I'm documenting. For example a long list of boilerplate exceptions can safely sit outside of documentation. I feel its sufficient to say 'this can not be null or whitespace' in the property definition.
Excellent tips for library writing! Thanks.
Hey thanks mate! i try to learn the best i can from you, however im still a little fresh and some things go over my hear. keep it up buddy, cheers
Regarding the access to methods/classes in your library, Challenge is how should a developer determine which one to make public? The answer that I have to that is that you write TDD. That will automatically guide you what needs to be made public. Once a developer becomes efficient with TDD, you'd get many many benefits and having a proper scope is one of them.
For the access keywords you can make to much private or internal too.
It's so frustrating when a library does something like make a public interface that returns a public type, but the type has an internal constructor, and no other kind of factory etc to make it so you can't really mock it out even though the interface is public because you can't create the object to be returned without reflection.
It's really just about thinking carefully about what should be public and what shouldn't.
Also what the extensibility points should be, what should be virtual and what should be sealed if inheritance is used, etc.
You should think carefully about it and take great care when designing your library's public surface area.
It is safer to start with private/internal/sealed than it is to start with public because you change your mind. Unsealing/making something public is not a breaking a change and can be done in a minor release.
Going the other way around can be very painful for you and the users of your library with breaking changes. You can get stuck where your unable to do some important refactoring/optimizations/fixes because too much of your library is public.
I advise that if there is a part of your library that you are unsure about it being public/unsealed, you can err towards less accessibility and then listen closely to your user's feedback.
@@Denominus yes, that's a good idea to avoid breaking changes.
The thinking about it is the important part though.
If your unit test project has internal access then you might be making a project that can't actually be used or can't be tested by a consumer that only has public access. You can make things public later, but it should be both usable and testable all the time. Library authors can also be understandably reluctant to make things public later, which can cause problems for consumers.
There is no substitute for just taking the time to design a good public API.
Hi nick, could you create an episode regarding the libraries for importing and exporting excel. For enterprise work, excel reading, manipulation and save it to db is ever presence scenario
Thank you for the video Nick. As always great content.
Can you do a video on best practices for distributed transaction.
IMO, some developers get in a habit of using 'public' as a way to make it easy to unit test methods/classes. I do not agree with that approach but I can see how it becomes a (bad) habit.
You don't need them to be public to me unit testable. You can just make the internals visible to your unit test project
@@nickchapsas exactly! :-)
@@nickchapsas Wouldn't that be wrong to do? If you are testing how others will interact with our api then you use should rely on public interface of your object ( public methods). Also assuming one is writing TDD, you'd automatically end up having the interaction in you test case that is public. Making internals visible to your test takes away a very big benefit of testing which is see the friction in your code when you are writing tests.
Writing tests isn't just about making sure it does what it supposed to do but also make your code clean and see the friction in code. With internal you can very easy skip that friction. Others using your api won't be able to and therefore I think your dlls should be public to test project and not internal.
Thanks for talking about library code as well. most stuff out there only talks about application code and structure.
While talking about structure would you know any good sources on that? Bcs that is what I am mostly struggling with, structuring my libraries
Just place yourself in the library consumer role and think how you would like the public API be, so it is easy to start with and use.
You shouldn't worry much about anything else at least at start.
Me: I wonder if Nick would do a review of nuget library code to tell me what I'm doing wrong.
Nick: I gotchu.
Great video!
Hi Nick, I’m currently programming a rather small library, which uses DI and Microsoft.Extensions.DependencyInjection.Abstractions to let the consumers use the DI Container of their choice. Is that a valid approach or are there some downsides I’m not aware of? Best Regards
Hi, great video, as always! Do you have any plan to make a video about different dependency injection containers and why would you use one over the other?
Absolutely. There are so many that I’m currently filtering out the ones that are actually popular and worth talking about but there is a video about that coming
Hey Nick,
Firstly love the video.
I, however, disagree with not wrapping "common" libraries that use statics (@2:15 NewtonSoft.Json). IMO these need to always be wrapped, so that during my unit test of the object under test (in your example the Controller Action) i can return any value. This makes it so that I am not reliant on valid input to get an output from that dependency. IMO even NewtonSof.Json should be interfaced.
I disagree. There is no scenario where I don’t want json.net to return what it will actually return. Mocking it is you just lying in your tests and your tests will be less reliable because of it
Are you lying in the test or testing a different area of code than intended?
If you call a static method from within your code (regardless of who created the static method) you are leaving the code under test. You are also placing responsibility on the test to understand how the static method works, so that the static method operates as expected.
Adding a simple one line wrapper around the static method, allows the unit test to control the input and return objects. This makes it so that the test isn't dependent on an external concrete implementation. An added benefit of wrapping is during verification the test can verify the correct input was passed in. Also, if applicable, the test can use a reference check to make sure the object was passed around appropriately within the method (from one dependency to another).
Wrapping up common packages gives several benefits (which i think you covered in another video if I remember correctly). Firstly, it allows easier substitution if the backing package no long suits your needs. It also allows for your application to "Trust" the wrapped package (and hope the developer had good testing practices).
I noticed that your code scrolling is very smooth while at the same time your mouse pointer is always very precise and on target, especially when selecting code, your pointer remains very straight horizontally, like you would be using a mouse. A trackpad is more difficult to draw a straight line with. I'm curious, if you're using a mouse, what mouse is that? I've been searching for a mouse that scrolls as smooth as a trackpad on Windows for ages and I think I might never find one.
Nice tips. Although I really don't see the point in using MediatR if CQRS is not used and/or there is no external messaging bus
MediatR is used as an example to talk about the tips. The video/usecase has nothing to do with the usage of MediatR
Since the name of the video is "Better library code", I'd like to point out that your code is taking a dependency and not checking if its null. I understand that you are sharing different concepts but I think the topics that deal with showing better code should proper code. I'm sure in your Prod code, you are not taking a dependency and not checking for null :-). This code will blow up if I write a test case for it by passing null to constructor on the method and not on constructor.
I need to use more cancellation tokens... All those wasted CPU cycles even on small things add up `~`
I agree with the point about marking classes as internal in principle, however I see a problem - how can your unit tests access these classes?
I have a video about this. You can mark the internals of a project as visibly on the assembly level for unit testing purpose only, which is a perfectly acceptable case.
@@nickchapsas which video are you referring to
Just as an FYI, unit tests are tests that cover a "module" of code. You've probably heard that it should cover a single class, but this is actually not accurate, it's simply one interpretation of "module." Your unit tests should describe the external behavior of a piece of code and how it is implemented is irrelevant. As far as a test is concerned, whether your code is implemented as a single class or many internal classes isn't important: just what the code does is.
@@nguyengiahoang71 I don't know what video either, but look up InternalsVisibleToAttribute in the C# reference.
@@nickchapsas Ah cool, I didn't know that. Thanks!
Everything should be public with a good documentation, how many times I needed to use reflection to call a private method inside an internal class or had to copy a lot of code from github/disassembly because I could not call a method that does exactly what I needed. Just document what can be changed, let me use WHAT I WANT, man that sucks.
If you needed reflection to call internal or private methods then you used the library wrong. There is a reason why things are internal or private and using reflection to call them is a really bad practice.
@@nickchapsas I know what I am doing because I read the code and I manage my library versions, The overall advice is ok but not always true.
@@nickchapsas I must agree with Márcio -- let me use any code, I am adult enough to understand that the surface of any API is its Interfaces not its classes. But we all are notorious bad at predicting how things are going to be used (or there impact). I do not want to reinvent all your code just because you did not think of my use case -- happened with me with WebApi2 OData code.
Nick I think you said in another video we shouldn't use comments like that
No. Comments and code summary aren't the same.
Code summary is very important.
In my projects all code should have code summary
@@antonmartyniuk I'll have to rewatch his old video..
I said that's one of the best ways to use comments in that video because it's not comments, it's documentation
@@CabbageYe Rarely should comments (i.e. // or */) be needed. You should always strive to write code that is self-explanatory. However, code documentation (i.e. ///) is another thing. The name of public classes, methods, properties and events do not always contain the necessary information required to fully understand what they do. To save the user from peeking into the source, or look up some public documentation on a website, please write short and insightful documentation on all public things in your API. This is also good practise if you are working on a team with others developers. I have also often found myself benefitting from my own documentation given enough time has passed where I have forgotten the details.
@@ThomasAngeland Thanks for the advice.
I like most of what you say, but I disagree with your assertion about most things in a library should be "private" or "internal". There's no way your library can anticipate every use-case that a downstream user will have. If you hide the building blocks, your users lose the ability to compose new solutions that you hadn't thought of before. Think very long and hard before you make something inaccessible to the user, you might be making life a lot harder for them. I far prefer approaches where we either (a) mention alternatives in the comments so basic users don't accidentally use something for advanced users, or (b) use namespaces to separate advanced use-cases. The "MyLibrary" root namespace should include all the abstractions and interfaces for basic use, where nested "MyLibrary.AdvancedTopic" namespaces can contain the more advanced building blocks. That way users can choose which parts of the library they want to see, and your documentation to guide them.
I would revert your answer completely and I completely disagree ok every level. If you don’t anticipate how your library should be used by your user then you shouldn’t ship it at all. Anything exposed as part of the public api are part of your contract with the user. If you can’t tunnel users to a specific guided path then you should design your library to do so. Think very long and hard before making something accessible to the user. Making it accessible is a commitment by you to them that this will not change.
@@nickchapsas You can't anticipate every use-case, it's foolish to try. You can and should try to guide a user to the correct path, but that doesn't mean you should cut off every other path. You should prefer composition over inheritance, and the only way to have composition is if you give users access to the building blocks that you used to compose solutions yourself. I see a lot of problems in libraries that didn't expose something necessary because of over-aggressive use of "private" and "internal", and those kinds of problems stop the library from being used at all.
@@wknight8111 Every single good library I've ever used does that very same thing and I can guarantee that it's libraries you are using too and you don't even know about it. Anything public in libraries like MediatR, Automapper, FluentAssertions, FluentValidator and tens of others are only exposing specifically what they need to. These are well designed libraries. If they wanted to seal something, there is a reason. If they marked something as private, there was a reason and if something was internal, there was a reason. There are really no points to be made here. If you think something should be exposed in a well designed library, then you are using the library wrong.
SECOND!
FIRST!