3
\$\begingroup\$

Based on the feedback I have got from the guys reviewing the code as posted in my previous question.

Create database connection and run the insert, delete, update queries class

I have used interfaces and wrote the following code

/* */

namespace SampleInterfaceT1
{
public interface IDbConSetup
{
    DbConnection OpenConnection(string connectionString);

    void CloseConnection();
}

public interface ISqlDataProvider : IDbConSetup
{
    int ExecuteCmd(string dbCmd, SqlParameter param);
}

public class SqlClient : ISqlDataProvider
{
    private SqlConnection _dbconnect;

    private string _conString;

    public DbConnection OpenConnection(string connectionString)
    {
        if (String.IsNullOrWhiteSpace(connectionString))
        {
            throw new ArgumentException("Invalid Connection String");
        }
        _conString = connectionString;

        Console.WriteLine("Sql Connection Opened.");

        _dbconnect = new SqlConnection(_conString);

        _dbconnect.Open();

        return _dbconnect;
    }

     public int ExecuteCmd(string dbCmd, SqlParameter param)
    {
        int _rc;

        if (String.IsNullOrWhiteSpace(dbCmd))
        {
            throw new ArgumentException("Invalid Connection String");
        }
        using (SqlCommand _sqlCmd = new SqlCommand(dbCmd, _dbconnect))
        {
            _sqlCmd.CommandType = CommandType.Text;

            _sqlCmd.Parameters.Add(param);

            _rc = _sqlCmd.ExecuteNonQuery();
        }

        Console.WriteLine("Command executed");

        return _rc;
    }

    public void CloseConnection()
    {
        _dbconnect.Close();
        Console.WriteLine("Sql Connection Closed.");
        _dbconnect.Dispose();
    }


   }
}

/*  Main Program execution */

namespace SampleInterfaceT1
{
 public class Program
 {
    private static void Main(string[] args)
    {
        string conString = @""; /* Connection String*/

        string dbCommand = @"INSERT INTO [dbo].[TestTable]([Name]) VALUES (@Name)";

        ISqlDataProvider SqlDataProviderObject = new SqlClient();

        SqlDataProviderObject.OpenConnection(conString);

        SqlDataProviderObject.ExecuteCmd(dbCommand, new SqlParameter("Name", "HellofromnewDictionary"));

        SqlDataProviderObject.CloseConnection();

        Console.ReadLine();
    }
}
}

I'm a newbie to the concept of interfaces. I'm I on the right path or did I got it completely wrong?

\$\endgroup\$
1
\$\begingroup\$

You won't gain any advantage by using abbreviations for your parameter/variable names but you will loose readability. Therefor I would like to suggest to use more descriptive names. Otherwise you will come back in a couple of weeks/months and you will be wondering what e.g int _rc; should represent.

This

if (String.IsNullOrWhiteSpace(dbCmd))
{
    throw new ArgumentException("Invalid Connection String");
}

in your ExecuteCmd() method shows why copy&pasta should be done in a wise way.

In addition you never check if the passed in SqlParameter para is null. In a public method you should always validate all passed in parameters.

As DbConnection is implementing IDisposable you should enclose its usage into a using block. This would also remove the need to have a CloseConnection() method. Hence this would result in removing the IDbConSetup interface.

Instead of having a public OpenConnection method you could pass the connection string to the constructor and make the method private. In addition you won't really need the class level field private SqlConnection _dbconnect;.

Methodlevel variables shouldn't be prefixed with an underscore. This is usually only done for class level variables.

A sample of a simplified DAL which still needs to be expanded.

Note this isn't tested

using System;
using System.Collections.Generic;
using System.Data;
using System.Data.Common;

public class SimplifiedDal
{
    private readonly DbProviderFactory dbFactory;
    private readonly string connectionString;
    public SimplifiedDal(string connectionString, string providerFactory)
    {
        if (string.IsNullOrWhiteSpace(connectionString))
        {
            throw new ArgumentException("Value may not be null or whitespace", "connectionString");
        }
        if (string.IsNullOrWhiteSpace(providerFactory))
        {
            throw new ArgumentException("Value may not be null or whitespace", "providerFactory");
        }
        dbFactory = DbProviderFactories.GetFactory(providerFactory);
        this.connectionString = connectionString;
    }
    public int ExecuteNonQuery(string sql, CommandType commandType, Dictionary<string, object> parameters)
    {
        if (string.IsNullOrWhiteSpace(sql))
        {
            throw new ArgumentException("Value may not be null or whitespace", "sql");
        }

        using (var connection = dbFactory.CreateConnection())
        {
            connection.ConnectionString = connectionString;
            connection.Open();
            using (var command = connection.CreateCommand())
            {
                AddParameters(command, parameters);
                return command.ExecuteNonQuery();
            }
        }
    }
    private void AddParameters(DbCommand command, Dictionary<string, object> parameters)
    {
        if (parameters == null || parameters.Count == 0) { return; }
        string dbFactoryName = dbFactory.GetType().Name;
        if (dbFactoryName == "OleDbFactory" || dbFactoryName == "OdbcFactory")
        {
            AddOleDbOdbcParameters(command, parameters);
            return;
        }

        string parameterPrefix = GetPrefixByFactoryName(dbFactoryName);
        foreach (var kvp in parameters)
        {
            var parameter = command.CreateParameter();
            parameter.ParameterName = parameterPrefix + kvp.Key;
            parameter.Value = kvp.Value;
            command.Parameters.Add(parameter);
        }
    }
    private string GetPrefixByFactoryName(string factoryName)
    {

        switch (factoryName)
        {
            case "SqlClientFactory":
                return "@";
            case "OracleClientFactory":
                return ":";
            case "MySqlClientFactory":
                return "?";
        }
        throw new NotImplementedException();
    }
    private void AddOleDbOdbcParameters(DbCommand command, Dictionary<string, object> parameters)
    {
        foreach (var kvp in parameters)
        {
            var parameter = command.CreateParameter();
            parameter.ParameterName = "?";
            parameter.Value = kvp.Value;
            command.Parameters.Add(parameter);
        }

    }

}  

which can be used like so

//MS Access
var msConnectionString = "A MS Access connection string";
var msDal = new SimplifiedDal(msConnectionString, "System.Data.OleDb");

//MySql
var mysqlConnectionString = "A MySQL connection string";
var mysqlDal = new SimplifiedDal(mysqlConnectionString, "System.Data.OleDb");

//MS Sql
var sqlConnectionString = @"A Sql connection string";
var sqlDal = new SimplifiedDal(sqlConnectionString, "System.Data.SqlClient");  

you can easily check which factories are installed by using

DataTable table = DbProviderFactories.GetFactoryClasses();
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This is an improvement but still... the three providers are hard-coded in the switch ;-] and... you rely on automatic data type mapping (I cannot find the SO question now but it can go wrong) - you should always use the type explicitly when setting parameters. IMO it's an overkill to abstract every bit of the providers and the switch... well it's a no-go as far as SOLID is concerned. As I said, it's a lot of work to make it really good ;-) \$\endgroup\$ – t3chb0t Aug 9 '16 at 11:49
  • \$\begingroup\$ This is my answer ;-) \$\endgroup\$ – t3chb0t Aug 16 '16 at 15:30

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.