3
\$\begingroup\$

First, this is my first post on this stack exchange site. So please be patient with me. If there is something wrong or you miss something, please let me know. I will add it asap.

I am currently working with the SOLID principles and trying to achieve a practical implementation. I have seen Tim Corey's practical videos on YouTube, which explain the principles well. Now I have a project where I would like to implement this and I am struggling a bit with the implementation of the dependency inversion principle in the context of SQL queries to a SQL server. Here, several instances of different classes are often needed, e.g. SqlConnection, SqlDataAdapter or DataSet. Perhaps in advance. I know that Dependency Injection could make things easier. But first I wanted to understand the principle of dependency inversion.

I've created now a (mini) project, where my approach becomes clear (the project with the complete code is also available on Github https://github.com/dnsnx/Dependency_Inversion_SQL).

I first created an interface IDatabase:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Dependency_Inversion_SQL.Interfaces
{
    public interface IDatabase
    {
        void AddParameters((string name, object value)[] parameters);

        System.Data.DataSet GetData(string commandText, System.Data.CommandType commandType = System.Data.CommandType.Text, string sourceTable = null);
    }
}

For simplicity, I just want to read data at the moment - no updates yet. So I defined a method for setting parameters (AddParameters) and one method for reading the data (GetData). First difficulty here: Keep the interface non-specific, i.e. not directly designed for SQL Server. In the future, the DB system could be changed! Then I created a class SQLDatabase which implements this interface:

using Dependency_Inversion_SQL.Interfaces;
using System;
using System.Collections.Generic;
using System.Data;
using System.Data.SqlClient;
using System.Linq;
using System.Text;


namespace Dependency_Inversion_SQL.Classes
{
    public class SQLDatabase : IDatabase
    {
        #region Variables
        private readonly string _SqlServer = "Integrated Security=true;Initial Catalog=Test;Server=localhost\\localhost;";
        private SqlConnection _SqlConnection;
        private SqlCommand _SqlCommand;
        private SqlDataAdapter _SqlDataAdapter;
        private DataSet _DataSet;
        #endregion

        #region Properties
        public string SqlServer
        {
            get
            {
                return _SqlServer;
            }
        }
        #endregion

        #region Constructors
        public SQLDatabase(SqlConnection sqlConnection, SqlDataAdapter sqlDataAdapter, DataSet dataSet)
        {
            _SqlConnection = sqlConnection;
            _SqlConnection.ConnectionString = _SqlServer;
            _SqlCommand = _SqlConnection.CreateCommand();
            _SqlDataAdapter = sqlDataAdapter;
            _DataSet = dataSet;
        }
        #endregion

        #region Methods
        public void AddParameters((string name, object value)[] parameters)
        {
            for (var i = 0; i < parameters.Length; i++)
            {
                _SqlCommand.Parameters.AddWithValue(parameters[i].name, parameters[i].value);
            }
        }

        public System.Data.DataSet GetData(string commandText, System.Data.CommandType commandType = System.Data.CommandType.Text, string sourceTable = null)
        {
            if (sourceTable == null)
            {
                sourceTable = "someName";
            }
            _DataSet.Clear();
            _SqlCommand.CommandText = commandText;
            _SqlCommand.CommandType = commandType;
            _SqlDataAdapter.SelectCommand = _SqlCommand;
            _SqlDataAdapter.SelectCommand.Connection.Open();
            _SqlDataAdapter.Fill(_DataSet, sourceTable);
            _SqlDataAdapter.SelectCommand.Connection.Close();
            _SqlCommand.Parameters.Clear();
            _SqlCommand.CommandText = string.Empty;
            return _DataSet;
        }
        #endregion
    }
}

Now, to implement dependency inversion, we need a factory class that defines which instances are created:

Factory:

using Dependency_Inversion_SQL.Classes;
using Dependency_Inversion_SQL.Interfaces;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Dependency_Inversion_SQL
{
    public static class Factory
    {
        public static ICar CreateCar()
        {
            return new Car(CreateDatabase());
        }

        public static IDatabase CreateDatabase()
        {
            return new SQLDatabase(CreateDatabaseConnection(), CreateDatabaseAdapater(), CreateDatabaseDataSet());
        }

        public static System.Data.SqlClient.SqlConnection CreateDatabaseConnection()
        {
            return new System.Data.SqlClient.SqlConnection();
        }

        public static System.Data.SqlClient.SqlDataAdapter CreateDatabaseAdapater()
        {
            return new System.Data.SqlClient.SqlDataAdapter();
        }

        public static System.Data.DataSet CreateDatabaseDataSet()
        {
            return new System.Data.DataSet();
        }
    }
}

Then I needed a class, which queries SQL data. I created a simple class Car, which "reads" a car name.

using Dependency_Inversion_SQL.Interfaces;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Dependency_Inversion_SQL.Classes
{
    class Car : ICar
    {
        IDatabase _Database;

        public Car(IDatabase database)
        {
            _Database = database;
        }

        public string GetCarName()
        {
            var result = _Database.GetData("SELECT 'Audi R8' as CarName");
            return result.Tables[0].Rows[0].ItemArray[0].ToString();
        }
    }
}

Now my console application:

using Dependency_Inversion_SQL.Interfaces;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Dependency_Inversion_SQL
{
    class Program
    {
        static void Main(string[] args)
        {
            ICar car = Factory.CreateCar();
            Console.WriteLine($"My dream car is an { car.GetCarName() }");
            Console.ReadLine();
        }
    }
}

As I wrote above, I was wondering, if there would be a more easy way to implement the SQLDatabase class. It needs all the stuff in the constructor (SqlConnection, SqlDataAdapter, DataSet) and so on. What I also don't know, if it is correct, how I implemented the generation of these classes in the Factory class.

public static System.Data.SqlClient.SqlConnection CreateDatabaseConnection()
{
    return new System.Data.SqlClient.SqlConnection();
}

public static System.Data.SqlClient.SqlDataAdapter CreateDatabaseAdapater()
{
    return new System.Data.SqlClient.SqlDataAdapter();
}

public static System.Data.DataSet CreateDatabaseDataSet()
{
    return new System.Data.DataSet();
}

What I understood is, that the return type, should be a interface as best. This is in my eyes not possible with the SQL classes.

If this question is too large or I misunderstood the stack exchange site, please let me know. I will delete the question then. Many thanks in advance for any comment on this.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to the Code Review Community. This site is specifically about improving ones coding ability. While we may recommend some best practices while reviewing the code, the point is to review the code. It is not clear that you are looking for a code review. We may review design as part of a code review, but for true design reviews you might want to try Software Engineering instead. Please see our help center for what we discuss here, start with How do I ask a good question?. \$\endgroup\$
    – pacmaninbw
    Feb 24 at 13:05
7
\$\begingroup\$

I think you have a reasonable grasp to understand the technical ins and out of design patterns, but you are struggling to use them sensibly. So this answer focuses on intent rather than specific syntax.

I just want to take some time here to point out something that you may wrongly infer about my feedback. Just because I call your implementations bad, doesn't mean I'm calling you a bad developer. You have clearly put a lot of thought and effort into this, and I'm most definitely not accusing you of not doing your due diligence or questioning your ability to write code.

You know how to lay bricks, but you're not working off of the right blueprint. I often use the example of Forrest Gump playing American football. He's amazing at putting one foot in front of the other, but he doesn't realize where he should and shouldn't be going or when to stop.

You've done the same thing. Your code is good if you look at it line by line, but it misses the bigger picture of what it tries to achieve.


The factory pattern

A factory is a place that creates things. You've nailed that part.

However, why would we use a factory instead of just a constructor? Because we can just as well call new Foo() without needing to create a FooFactory.CreateFoo.

There are three main purposes to the factory pattern:

  1. If the construction of the object is more complex than the consumer needs to care about, the factory acts as a consumer-friendly simplification.
  2. If the consumer doesn't get to decide which concrete class should be used, and instead the factory decides that for them. This is what I call a "smart" factory, it makes the choice for the consumer.
  3. Specific to DI, when a class needs to be able to repeatedly create new instances of an injected dependency, rather than having one instance injected in its constructor.

Your code, which is a factory wrapper around not even your own code (they're System.Data classes), doesn't actually add anything meaningful. It does nothing, other than call an empty constructor. This doesn't match the purposes of a factory, and therefore the factory is not necessary.

Some quick examples of how factories can be useful:

Purpose 1

public class CarFactory
{
    public ICar CreateCar()
    {
        return new Car()
        {
            Engine = new PetrolEngine(),
            Wheels = new[] { new Wheel(), new Wheel(), new Wheel(), new Wheel() }
        }
    }
}

In this example, you don't want your consumer to know how to put a car together, so you have the factory do it for them.

Purpose 2

public class CarFactory
{
    public ICar CreateAppropriateCar(CarGoal goal)
    {
        switch(goal)
        {
            case CarGoal.Racing:
            case CarGoal.ShowingOff:
                return new SportsCar();
            case CarGoal.Transport:
            case CarGoal.LivingIn:
                return new Van();
            case CarGoal.Offroading:
                return new Jeep();
            case CarGoal.Tourism:
                return new TourBus();
            default:
                return new Sedan();
        }
    }
}

You don't want the consumer to have to decide which car is most appropriate, the consumer only knows what they intend to do with the car (CarGoal). The factory makes the correct decision based on the reported goal.

Purpose 3 isn't easily demonstrable.


What I understood is, that the return type, should be a interface as best.

For purpose 2, it is essential that your return type is not the concrete type, or it would otherwise defeat the purpose.

For purposes 1 and 3, it's not necessary but still a general nice-to-have in terms of clean coding. Not so much for the validity of the factory pattern in and of itself, but rather because it implies that you're using your interfaces correctly in your codebase.

This is in my eyes not possible with the SQL classes.

Since they don't implement an interface, it isn't.

There are cases where you'd wrap a library class in one of your own, and you can implement an interface on that custom class, but don't start doing this blindly as it leads to a bunch over unused over-engineered boilerplate code.


When all you have is a hammer, everything looks like a nail.

public static ICar CreateCar()
{
    return new Car(CreateDatabase());
}

You've gone overboard on using the factory pattern to the point trying to wrap everything in a factory. This relates back to my initial paragraph in this answer: you know how to implement a factory, but you don't know when or why to implement a factory.

The larger issue here is that the instantiation of a car should not trigger the instantiation of a database connection (or the object which will eventually make the connection). Creating an in-memory object shouldn't be inherently tied to connecting to a database. It defeats the purpose of doing things in-memory. Conceptually these are very separate steps.


Is-a versus has-a

The goal is to use SqlDatabase in your codebase, but without tightly coupling yourself to that specific data provider. This much is clear.

But it seems like the only attempts you've made at doing this has been through using interfaces.

I can't and won't explain it in its entirety here, but you need to introduce yourself to the concept of composition over inheritance. While inheritance isn't quite the same as interface implementation, in context of comparing composition and inheritance, you can lump interfaces in with inheritance.

Interfaces implementation and class inheritance are a "is-a" relationship. Car is an ICar. SQLDatabase is an IDatabase.

But compositions uses a "has a" relationship. Take the example of me, a person. I have a car. How would you reflect that in code? Like this?

public class Person : Car { }

No. It's not done by using inheritance or interfaces, because "A Person is a Car" is not correct.

public class Person
{
    public Car Car { get; set; }
}

This is the correct approach, because "A Person has a Car"

Note: since cars have interfaces, it's indeed correct to use the interface instead of the concrete type here, so a better version would be:

public class Person
{
    public ICar Car { get; set; }
}

But in cases where there is no interface, using the concrete type would still be a correct usage of composition over inheritance.

I suggest you look into the repository pattern to understand how to use composition to work with your SqlDatabase, while at the same time hiding it from view from the consumer.

While I generally don't use repositories myself anymore, it's a really good start for a beginner to grasp basic layer separation and encapsulation. With more experience, you'll learn about other possible (and more advanced) approached.


Dependency inversion

But first I wanted to understand the principle of dependency inversion.

I can't go into this in full detail, but I'll use an analogy to help you understand the intent. Suppose we run a sandwich shop:

public class SandwichShop
{
    public Sandwich MakeSandwich()
    {
        var bread = new WhiteBread();
        bread.Cut();

        var toppings = new List<Topping>() { new Bacon(), new Lettuce, new Tomato() };

        bread.Add(toppings);
        bread.Wrap();

        return bread;          
    }
}

The customer depends on the sandwich shop, and the sandwich shop depends on the white bread. That mean that the customer indirectly depends on the white bread. Similarly, the customer can only have the toppings that the sandwich shop has hardcoded.

If we give the customer control over which bread to use, that would be inverting the normal order of dependency.

This is the same class, but now made with dependency inversion in mind:

public class SandwichShop
{
    public Sandwich MakeSandwich(Bread bread, List<Topping> toppings)
    {
        bread.Cut();
        bread.Add(toppings);
        bread.Wrap();

        return bread;          
    }
}

Now, the customer (i.e. the actor who calls MakeSandwich gets to decide the bread and the toppings). The downside is that the customer is now responsible for providing those dependencies as well.

This example uses a method and method parameters. In software development, the more common implementation of dependency inversion is related to the constructor argument, but it's the same principle: the caller (i.e. the actor who calls the constructor) provides the dependencies of the object it's creating.

Just to disambiguate:

  • Dependency injection generally refers to using a framework or container to automatically provide the correct dependency to any object, based on what it asks for in the constructor.
  • Inverted dependencies are (annoyingly) a completely different thing than dependency inversion, and something significantly more advanced that I'm not going to get into here.
\$\endgroup\$
1
  • \$\begingroup\$ Many thanks for your detailed feedback. I will carefully read your answer and then give you a feedback. \$\endgroup\$
    – dns_nx
    Feb 24 at 14:28
0
\$\begingroup\$

Some quick remarks:

  • IDatabase is IMHO a bad name, and you can see that with a method name like CreateDatabase. The same problem also applies to other things, e.g. SqlServer which is in reality a connection string (which you should never hardcoded, it should be part of a configuration file). And so on.

  • I haven't checked your github, but I suspect you are not properly disposing of your DB connections.

  • GetCarName() is wrong on so many levels. The proper way is to retrieve a Car object from the DB, and then look at its Name property, e.g. var carName = car.Name;.


But all of that is pointless criticism of details. My advice: throw away all of this code. You're writing your own DB framework, and it will never ever be as good as Entity Framework or Dapper. You're wasting your time; what you should be doing is learning how to properly use Entity Framework or Dapper.

WRT SOLID etc., please understand that these are guidelines, not unbreakable laws.

WRT dependency injection etc.: follow the numerous guides on how to combine dependency injection and Entity Framework that are available. These things have been solved long ago, and no company will ever require you to write your own implementation, they'll simply want you to follow their way of working (which will likely be the general way as documented in numerous guides).

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you for your feedback. I know, that the connection should be stored in a configuration file and also I know, that GetCarName method is a bad method. It was just to demonstrate the issue I have. I would never implement this method in a production code. As I wrote, I created this project to demonstrate my issue. But many thanks for your hint for Entity Framework. I already implemented it in different project, but I was wondering, if I can use the SqlConnection classes with DI, too. \$\endgroup\$
    – dns_nx
    Feb 24 at 14:27

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.