There's really no reason not to enable Nullable. It's enabled by default on .NET 6.0 projects for a reason. It gently pushes you into taking into account times where your input or output could be null by warning you of potential dangers.
<Project Sdk="Microsoft.NET.Sdk.Web">
...
<PropertyGroup>
<Nullable>enable</Nullable>
</PropertyGroup>
...
</Project>There shouldn't ever be more than a single empty line before something new happens. There's no reason for the extra blank space. Next time your rebase or merge goes through without a hitch, you can thank me.
public class Test
{
public void DoThing()
{
}
}BAD
public class Test
{
public void DoThing() { }
}GOOD
Commented out code only creates confusion the longer it lingers. It doesn't add any value and pollutes the code base. If the code is supposedly so important, why is it not in use?
Also, it will forever be in Git, so don't worry too much.
// This class no longer works, here for posterity
// public class Test
// {
// public void DoThing()
// {
// }
// }BAD
Only comment when absolutely necessary and make sure it is readable by someone else jumping into the code. If you're going to comment, do it properly. And make sure the comment isn't simply explaining something you can read from the code. It should usually explain a business logic quirk or any kind of deviation from the implicit flow.
// This can removed when we untangle the encryption layer and assign all the variables to pizza
var weather = await GetTodaysWeather();BAD
// Get today's weather
var weather = await GetTodaysWeather();BAD
var weather = await GetTodaysWeather();GOOD
var guy = new Person
{
Id = Guid.Parse("b6a98cf0-1e92-4e4f-ac53-ca519ec0ffc9"),
Name = "Jens",
Age = 50
};
guy.Company = await GetCompanyByPerson(guy.Id);BAD
var guyId = Guid.Parse("b6a98cf0-1e92-4e4f-ac53-ca519ec0ffc9");
var company = await GetCompanyByPerson(guyId);
var guy = new Person(
guyId,
"Jens",
50,
company);GOOD
Very seldomly does else make sense, and it's what I would consider a code smell. Try to always check for positive things and avoid deviating from the main flow. Avoiding else in favor of safe-guards and returns vastly reduces the cyclomatic complexity of your business logic.
public void Test(string input)
{
if (!string.IsNullOrEmpty(input))
{
// Do logic with valid input
}
else
{
// Handle invalid input
}
}BAD
public void Test(string input)
{
if (string.IsNullOrEmpty(input))
{
throw new Exception("Hey, you can't do this to me");
}
// Do logic with valid input
}GOOD
Adding async/await keywords to your code creates a state-machine behind the scenes to handle the synchronization context between threads. This is unnecessary ONLY IF you are just re-directing a call to another method that returns a task. If you do ANY logic before or after that, use async/await like normal.
public async Task<int> GetSomeNumber()
{
return await GetSomeNumberInternal();
}
private async Task<int> GetSomeNumberInternal()
{
...
}BAD
public Task<int> GetSomeNumber()
{
return GetSomeNumberInternal();
}
private async Task<int> GetSomeNumberInternal()
{
...
}GOOD
While it may be tempting to use the fancy params keyword for accepting n number of values, the problem with this approach is that the input gets "boxed" as an array and allocated on the heap rather than the stack, which means the garbage collector will eventually need to come clean that up. This means worse overall performance of your application. Really think about whether you NEED a potentially infinite number of parameters or if it's acceptable to just repeat yourself a bit and create separate methods that take in multiple values.
params isn't the worst however, but often it's overkill.
public class HandlerRunner
{
public void RunHandler<THandler>(params THandler[] handlers) where THandler : IHandler
{
foreach (var handler in handlers)
{
handler.Handle();
}
}
}
public class Program
{
public static void Main(string[] args)
{
new HandlerRunner()
.RunHandler(
new MyFirstHandler(),
new MySecondHandler());
}
}BAD
public class HandlerRunner
{
public void RunHandler<THandler>(
THandler handler) where THandler : IHandler
{
handler.Handle();
}
public void RunHandler<THandler>(
THandler handler1,
THandler handler2) where THandler : IHandler
{
handler1.Handle();
handler2.Handle();
}
public void RunHandler<THandler>(
THandler handler1,
THandler handler2,
THandler handler3) where THandler : IHandler
{
handler1.Handle();
handler2.Handle();
handler3.Handle();
}
// etc.
}
public class Program
{
public static void Main(string[] args)
{
new HandlerRunner()
.RunHandler(
new MyFirstHandler(),
new MySecondHandler());
}
}GOOD
Inheritance can be useful in some specific usecases, but seldomly is a good idea. You can't inherit from multiple base classes and making a breaking change to the base class means breaking all its inheritors as well. If you use small interfaces with specific feature-sets, very seldomly do you tend to break unrelated parts of the code. Classes can implement as many interfaces as you want, and you can start writing behavior-based code rather than try to fit business goals into some abstract form of hierarchy.
I only ever use inheritance if I'm noticing a lot of repeated code between classes that implement the same interfaces. If they're similar enough it can make sense to create an abstract base class that implements some of their interfaces FOR them.
You should only ever make classes public that other libraries and projects require. You can force yourself to think about what you expose outwards by always writing your classes with the internal keyword instead of public. The compiler will then show errors if you need to expose a class to the outside world. There are some caveats, however. If you use assembly scanning libraries and such, they MIGHT not pick up your classes. FluentValidation, for instance, has a problem in this regard. But MediatR doesn't care if your handlers are internal (good boy MediatR). Always test it out.
Furthermore, add sealed to your classes means they can't be inherited which you almost ALWAYS want for 99% of cases. This also carries a slight performance benefit, so why not?
public interface IMyService
{
void DoThing();
}
public class MyService : IMyService
{
public void DoThing()
{
// Logic
}
}BAD
public interface IMyService
{
void DoThing();
}
internal sealed class MyService : IMyService
{
public void DoThing()
{
// Logic
}
}GOOD
Layers of abstraction make code harder to navigate, conceptualize, debug and test. To some extent they're necessary, but they should never get in the way of testability. A good question to ask yourself is this: If I want to unit test this class, can I just new it up without worrying about dependencies? Most likely, the answer is no. Your PersonController needs to have a PersonService injected, after which PersonService needs to have PersonRepository injected, which then calls into EntityFramework and then all the way up the tree.
If you wanted to test PersonService you would probably need to Mock PersonRepository with some fake version that returns a specific result you know PersonService either likes or dislikes (depending on the test). But is that really a unit test? Let's say you broke something internally in PersonRepository and it caused a PersonService test to fail. Shouldn't it be a PersonRepository test that actually fails? It is, after all, the unit that changed its underlying logic and no longer satisfies its exposed interface, NOT PersonService.
Also, the test shouldn't really know the internals of PersonService. All it should care about is whether valid input generates a valid result and invalid input generates an invalid result. When you start mocking its dependencies, the whole idea of black-box testing goes out the window.
The way to get around this is to split your methods into testable chunks, remove any mocking and save your tests that go through all the layers for your real integration tests. I would argue mocking is an anti-pattern and not proper unit testing.
Records are just better. They trivial to write, the allow for immutability AND serilization at the same time, you can destructure them in the consuming code. I mean, just look at this:
public class Person
{
public string Name { get; set; }
public int Age { get; set; }
}BAD
public record Person(string Name, int Age);GOOD
TODO