Real Life work with SOLID development

Question

Lately I learned SOLID development, now I am facing some challenges with when it is a good practice and when it is not.

For example, I developed a website with members. I built an Authentication Business Logic class that should work out the authentication scenarios, which are:

1. Login user
2. Get logged in user
3. Register user
4. Recover user password

This class has 4 dependencies:

1. DB/Services
2. HttpContext (for user state)
3. ValidationRules (some other business logic rules in order to login user)
4. SMTP - for sending

Now it feels like some of the code have bad smell, because of not use dependencies injection.

Some issues that I am not sure about:

1. I can inject SMTP dependency directly to Restore password & register methods, because all other methods are not related to it. Should I?
2. Some methods such as Logout are not using the DB, why should I initiate db dependencies when I know that I will not need to use it.
3. This is for some of my business logic. In my controllers this issue is much bigger since I need to load more then one BL and all my business classes have same bad smell...

I feel like I am doing something wrong, please help!

Answer 1

an Authentication Business Logic class that should work out the authentication scenarios, which are: Login user, Get logged in user, Register user, Recover user password.

Your Authentication Business Logic class is already violating the Single Responsibility Principle. Although you might think that the only responsibility of that class is 'authentication', this can hardly be called one responsibility, since you could implement hundred use cases that deal with authentication, which leads to one ugly big ass class. Would that class still have one responsibility? Instead, implement one use case per class. In that case your classes will have a very clear and narrow responsibility.

HttpContext (for user state)

Your business logic should not know anything about the used technology (except that it's .NET of course, we can't really abstract .NET away) and it should therefore not depend on a HttpContext. Depending on HttpContext violates both the Dependency Inversion Principle (DIP) and the Interface Segregation Principle (ISP).

The DIP says that "High-level modules should ... depend on abstractions.". Your business layer is the "higher-level module", but it doesn't depend on an abstraction but on a lower-level module (the HttpContext). This tightly couples the business layer class with actual web logic.

Furthermore, even if you would use the System.Web.HttpContextBase abstraction instead, you will still depend on an abstraction that is defined by a lower-level module, while according to the DIP "the abstracts are owned by the upper/policy layers". The idea behind this is that you don't want your code to take a strong dependency on a lower layer, because this makes the higher-level layer more dependent on this, and besides this the lower level layer can not define an abstraction that is correct for your application. This issue is expressed next.

The ISP states that "no client should be forced to depend on methods it does not use". In other words, abstractions should be tailored for the special needs of a client and should be narrow. "Such shrunken interfaces are also called role interfaces". Both HttpContext and HttpContextBase violate the ISP, because they are very wide and meant for general use. They are one big state bad where everything is a string. This causes your code to work with a very wide API, which makes that API harder to use. It also hinders testability because it is impossible to fake a HttpContext and even the abstract HttpContextBase is unpleasant to fake. Both your production code and test code will become much cleaner if you apply ISP. It also hinders the re-usability of your application, because you might want to run some parts of your business logic in a background process later on, where there is no notion of a web request. And it hinders flexibility, because at one time you might want to intercept or decorate some operations that you execute on that HttpContext. For instance you might want to log any time user details are requested from the HttpContext. But when your code takes a direct dependency on the HttpContext, it means that you are forced to make sweeping changes throughout your code base. Having to do sweeping changes throughout your code is an indication that you are violating the Open/Closed Principle.

Instead, define your own IUserContext abstraction in the BL and create an AspNetUserContext implementation (that implements IUserContext) in your web application. Note that you shouldn't define an IHttpContext abstraction, because that would still violate SRP and ISP and is basically a leaky abstraction (and a leaky abstraction is a DIP violation), because the business layer shouldn't have to know about the existence of a web request.

I can inject SMTP dependency directly

When doing this you'll again violate the Dependency Inversion Principle. Your business layer doesn't depend on an abstraction but on a lower-level module (your SMTP class). This tightly couples the business layer class with actual SMTP logic. Instead, you should define your own abstraction for this, such as IMailSender.

This might sound odd and you might feel you will never change the way messages are sent, since e-mail is the only way to do this. But even if e-mail is sticking around for many years, you will likely want to change the way e-mail messages are being processed, since in your current implementation, e-mail messages aren't send atomically with the business transaction. What this means is that you might have pushed the message to the SMTP server (an operation that can't be rolled back), but the total operation could still fail after that. When it fails, it means that the open database transaction is rolled back, but at that point the mail is still sent. You can't call it back and you'll end up sending that mail message again later. This will annoy your receivers and can even cause you to send information (like urls containing database ids) that no longer exist after the transaction is rolled back.

To mitigate this, you will have to write the mail messages to a transactional queue (for instance a database table with messages that should be sent). This operation should run in the same transaction as the business logic runs. This means you can queue multiple messages in the same transaction and they all get rolled back when the operation fails. When the operation succeeds, all messages become available and some other (background) process can pick them up and send them.

This is just one possible way. The way mail is sent can change in the future, but without an IMailSender abstraction, this will be much harder to fix.

Some methods such as Logout are not using the DB, why should I initiate db dependencies when I know that I will not need to use it.

In general its not a problem when a dependency doesn't always get used, because creating the object graph (a service with all its direct and indirect dependencies) should be very fast. But in your case, what you're witnessing is that your methods aren't very cohesive. This is an indication of a Single Responsibility Violation. Instead you should give the Logout operation its own class. Since this operation doesn't need the db, you don't need the db dependency here.

In my controllers this issue is much bigger since I need to load more then one BL and all my business classes have same bad smell..

Your controllers are probably violation the Single Responsibility Principle as well. A good indication of a SRP violation is the number of dependencies a class has. The heuristic is 4 or 5. With over 5 dependencies, it is very likely that your class has too many responsibilities.

When you follow SOLID you'll get many small and focused classes. This might seem like a lot of work and code at first, but in fact this results in a system that is much easier to maintain. If you're struggling about how to create your business logic, take a look at this article.