4
\$\begingroup\$

I have started learning about SOLID principles from yesterday. I've got a detailed explanation of SOLID principle here. After reading it and 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:

  1. Read some settings from SQL Server database.
  2. Call an external system API with read settings.
  3. API returns the result in json format.
  4. Returned results should be saved in SQL Server Database.

Following code sample is in C# with Asp.Net

I have a form that initiates 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 SQL Server.

Client class has following implementation

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

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

        }

    public void Invoke()
        {
            List<IProperties> lstProperties = _reader.ReadData();
            _externalSystemInformation.HitAPI(lstProperties);
        }
    }

JSONExternalSystem implementation is like:

class JSonExternalSystem : Interfaces.IExternalSystemInteraction
    {

        private Interfaces.IWriter _writer;

        public JSonExternalSystem(Interfaces.IWriter writer)
        {
            _writer = writer;
        }

        #region IExternalSystemInteraction Members

        public void HitAPI(List<IProperties> properties)
        {
           foreach (IProperties property in properties)
            {
                ReadJsonFormat(property.Xproperty);
                _writer.ImportToDataBase();

            }
        }    
   }

My question is: By looking at this much code, do you think that I am on the right way to implement SOLID principles? I know that this doesn't not show all implementation. But most of the code for other classes goes more or less like above given class. I am sure I must be lacking somewhere as this is just my 2nd day. Kindly correct me.

\$\endgroup\$
3
\$\begingroup\$

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](object sender, EventArgs e).

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 them; by letting go of the control you have over the types that a type can instantiate, your code becomes very focused, cohesive. High cohesion, low coupling is what you want to achieve.

\$\endgroup\$
  • \$\begingroup\$ Ooh, I should have double-checked the tags before answering. I took "form" as in System.Windows.Forms... DI is a bit different in this case (because the page is your composition root), I'll edit my post accordingly when I have a chance, but most of it still stands. Just don't bother with "the form's constructor" :) \$\endgroup\$ – Mathieu Guindon Dec 27 '13 at 3:48
  • \$\begingroup\$ I read the points you mentioned carefully. Your point about high cohesion and low coupling is very true. I have two questions here: 1) As a programmer, how can I reduce the chances of breaking SRP, as you said above, "Too many (4? 5? 6? Your call!) constructor parameters (/dependencies) is a sign that your class is starting to break the Single Responsibility Principle." 2) At some point of time I will need to initialize IReader reader = new DbReader();, somewhere. What should be that point ? For eg; In a windows application should I initialize it in Main() method of program.cs ? \$\endgroup\$ – shrekDeep Dec 31 '13 at 14:09
  • \$\begingroup\$ Regarding 1, many times multiple dependencies are related and can be regrouped into one; 2, that should be as close as possible to your composition root.. but in asp.net that entry point isn't exactly as clear-cut as a Main() method would be. \$\endgroup\$ – Mathieu Guindon Dec 31 '13 at 17:40
  • \$\begingroup\$ So I guess idea is to keep those points to minimum where we need to initialize objects of a particular type. Right ? \$\endgroup\$ – shrekDeep Jan 1 '14 at 7:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.