Implementing SOLID Principles with C# Asp.net

I have started learning about SOLID principles yesterday. I've got a detailed explanation of SOLID principles here. After reading it, and a few other articles, I have tried to apply these principles in one of my projects.

This is a tool for importing data from another system. Requirements are as follows:

• Read some settings from SQL Server database.

• Call an external system API with read settings.

• API returns the result in Json format.

• Returned results should be saved in SQL Server Database.

Following code sample is in C# with Asp.Net

I have a form that initiates the import process on a button's click event.

private void btnStart_Click(object sender, EventArgs e)
        {
            IReader _reader = new DBReader();
            Interfaces.IWriter _writer = new Classes.DBWriter();
            
            Interfaces.IExternalSystemInteraction _externalSystem = new Classes.JSonExternalSystem(_writer);

            Classes.Client _client = new PxImportApplication.Classes.Client(_reader, _externalSystem);
            _client.Invoke();
        }

-
DBReader class inherits from IReader interface and does the job to read settings from SQL server database. We can create a new class if there is a requirement to read from MySQL database instead of SQLServer.

-
Similary DBWriter writes into SQL Server database and implements IWriter interface.

-
JSonExternalSystem class hits the API and reads data into Json format. IWriter interface instance is passed to get the Json data so that it could write that data into the SQL Server DB.

Client class has the following implementation:

```
class Client
{
private IReader _reader;
private Interfaces.IExternalSystemInteraction _externalSystemInformation;

public Client(IReader reader, Interfaces.IExternalSystemInteraction externalSystem)
{
_reader = reader;
_externalSystemInformation = externalSystem;

}

public void Inv

When you say I have a form that initiates import process on a button's click event, you've coupled your UI with some "business logic". Let's look at that Click handler:

private void btnStart_Click(object sender, EventArgs e)
   {
       IReader _reader = new DBReader();
       Interfaces.IWriter _writer = new Classes.DBWriter();

       Interfaces.IExternalSystemInteraction _externalSystem = new Classes.JSonExternalSystem(_writer);

       Classes.Client _client = new PxImportApplication.Classes.Client(_reader, _externalSystem);
       _client.Invoke();
   }

Naming

This method handles the click of a button, I'd infer a button named btnStart and say that the naming convention you're using for control names is breaking the naming convention for method names - how about naming the button StartButton and handle the Click event in a StartButton_Click, or OnStartButtonClick method? In my opinion the reasons why btnStart is a bad name would be something like:

• Hungarian Notation is used, "btn" being a short for Button, which is a type.

• Disemvoweling turned "button" into "btn".

• camelCase breaks the PascalCase convention for naming methods, because the convention for event handlers is void [objectName]_eventName.

Type qualification

If, at the top of your code file, you added the following:

using PxImportApplication.Classes;
using PxImportApplication.Interfaces;

Your handler code could read like this:

private void btnStart_Click(object sender, EventArgs e)
   {
       var reader = new DBReader();
       var writer = new DBWriter();

       var externalSystem = new JSonExternalSystem(writer);

       var client = new Client(reader, externalSystem);
       client.Invoke();
   }

Fully-qualified types make the code harder to read. I noticed you used _ in front of local variables; I prefer to use the underscore for private fields, if at all.

Dependencies

The problem with this approach, is that by instantiating that form you automatically get a new DBReader and a new DBWriter, with absolutely no way of swapping these two with a mock implementation - actually you don't really have another way of testing this piece of logic, other than F5 and run the thing and click the button and see how it goes.

In fact, the UI is tightly coupled with the DBReader, DBWriter (which should be DbReader and DbWriter), Client and JSonExternalSystem. Whenever you new up something, you've identified a dependency. Most classes of the .net framework are ok to instantiate on the fly like this, other dependencies should be clearly stated, in the form's constructor. Keep the parameterless default constructor so that you won't break the designer, but make another that takes all other dependencies. Too many (4? 5? 6? Your call!) constructor parameters (/dependencies) is a sign that your class is starting to break the Single Responsibility Principle.

I think the form is doing much more than just being a UI here, and the dependencies really are, is dependencies of another type, namely PxImportApplication.Classes.Client.

What the button really needs to do, is instantiate a Client and call the Invoke() method (a confusing name that breaks POLS, Invoke is .net lingo for synchroneously calling a delegate). All this newing up is covering-up the real intent. How about having a click handler doing this:

private void btnStart_Click(object sender, EventArgs e)
{
       var client = _clientFactory.Create();
       client.Invoke(); // todo: rename this method
}

This code means the handler now only depends on a _clientFactory private field:

private readonly IFactory _clientFactory;

And this field can be instantiated from the form's parameterized constructor:

public Form1(IFactory clientFactory) : this()
{
    _clientFactory = clientFactory;
}

The factory class has its own dependencies:

private readonly IReader _reader;
private readonly IExternalSystemInteraction _externalSystem;

public ClientFactory(IReader reader, IExternalSystemInteraction externalSystem)
{
    _reader = reader;
    _externalSystem = externalSystem;
}

public IClient Create()
{
    return new Client(_reader, _externalSystem);
}

The ClientFactory class implements the IFactory interface, which simply exposes a parameterless Create() method - and that's everything the form needs to know.

Your constructor-injected private fields can be made readonly in both Client and JSonExternalSystem classes (should be JsonExternalSystem). You're doing it right in these classes - don't do DI halfway though, do it all the way. Creating instances of types is the role of factory/infrastructure classes and of the application's composition root. If you're using an IoC container you don't have to resolve the dependencies yourself. This means if the IReader that the factory class takes has dependencies of its own, you don't need to bother with

StackExchange Code Review Q#38035, answer score: 5