7
\$\begingroup\$

The main idea behind this class is to create a wrapper for commonly used Database classes. I'm new to SOLID principles and I'm trying to implement them in this library. Does this code adhere to SOLID principles?

//This is the opens the SQL Connection and is placed in DbConnectionSetup.cs   

 public interface IConnectionProvider
{
    SqlConnection OConnection(string ConnectionString);
}

public abstract class DatabaseAttributes
{
    private string _connectionString = null;

    private string _command = null;

    public virtual string ValidateConnectionString
    {
        get
        {
            return this._connectionString;
        }
        set
        {
            if (!String.IsNullOrEmpty(value))
            {
                _connectionString = value;
            }
            else
            {
                throw new Exception("No Connection String property detected.");
            }
        }
    }

    public virtual string ValidateSqlCommand
    {
        get
        {
            return this._command;
        }
        set
        {
            if (!String.IsNullOrEmpty(value))
            {
                _command = value;
            }
            else
            {
                throw new Exception("No Command String detected.");
            }
        }
    }
}

[Description("This opens the Sql Connection")]
public class SqlOpenConnection : DatabaseAttributes, IConnectionProvider
{
    public SqlConnection OConnection(string ConnectionString)
    {
        ValidateConnectionString = ConnectionString;

        var _dbConConnection = new SqlConnection(ValidateConnectionString);

        try
        {
            if (_dbConConnection.State != System.Data.ConnectionState.Open)
            {
                _dbConConnection.Open();
            }
        }
        catch (SqlException ex)
        {
            throw new Exception("SqlException thrown", ex);
        }

        return _dbConConnection;
    }
}

/* I have a different class called SqlOps.cs which has the following code:*/

public interface ICommandWithParams
{
    int ExecuteCommandWithParams(string ConnectionString, string Command, Dictionary<string, SqlParameter> Params);
}

//have one function to Insert,Update and delete

public class ExecuteComplexCud : DatabaseAttributes, ICommandWithParams
{
    public int ExecuteCommandWithParams(string ConnectionString, string Command, Dictionary<string, SqlParameter> Params)
    {
        int rc;
        IConnectionProvider _dbCon = new SqlOpenConnection();//Get OConnection from DbConnectionSetup.cs

        ValidateSqlCommand = Command;//Check for Empty Command
        try
        {
            using (SqlCommand _sqlCmd = new SqlCommand(ValidateSqlCommand, _dbCon.OConnection(ConnectionString)))
            {
                _sqlCmd.CommandType = CommandType.Text;
                foreach (var keyValuePair in Params)
                {
                    _sqlCmd.Parameters.Add(keyValuePair.Value);
                }
                rc = _sqlCmd.ExecuteNonQuery();
            }
        }
        catch (SqlException)
        {
            throw;
        }
        catch (Exception)
        {
            throw;
        }
        finally
        {
            //Close the connection and dispose the object
            IConnectionProvider _dbCloseCon = new SqlCloseConnection();
            _dbCloseCon.OConnection(ConnectionString);
        }

        return rc;
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – BCdotWEB May 9 '16 at 11:49
14
\$\begingroup\$

First of all, it's unclear to me what exactly you want to achieve with this code. Database access, yes, but what benefits should your code provide? Why don't you want to use SqlConnection (or IDbConnection) and its associated classes directly?

Without knowing what sort of abstraction level you want to provide (and why), I think adherence to SOLID principles is a moot point. But here's what I think:

Overall design

It looks like you're taking the 'S' in SOLID a bit too far. Writing a separate class for each possible operation can be useful in certain situations, but here it only seems to complicate things. It also looks like your code doesn't actually work correctly: the SqlConnection returned by SqlOpenConnection.OConnection is never passed to that SqlCloseConnection instance, so how can it close that connection? And why would you need to create a new object to close an existing connection anyway? That's not intuitive.

Note that using a SqlConnection directly allows you to use a using statement, which would result in cleaner (and correct) code. I don't see any added value in those SqlOpenConnection and SqlCloseConnection classes. A single Database class that offers various methods for storing and retrieving data would be less complicated, and still serve a single purpose (database access).

I don't see any code that's using your interfaces (except inside ExecuteComplexCud.ExecuteCommandWithParams, but that seems to be an implementation detail of your library), so it's hard to tell how useful those interfaces really are - and thus how well you're taking the 'I' in SOLID into account.

Exception handling

You don't need to catch an exception if you're just going to throw it.

The type of an exception is a useful piece of information. SqlException indicates that the problem is related to database access. By wrapping that in a generic Exception, calling code is no longer able to distinguish database related exceptions from other exceptions. Wrapping can be useful, for example if you want to hide implementation details. In that case, your library may want to provide its own exception type, such as DatabaseException. Also think about what information the caller wants or needs in order to handle such exceptions.

Other notes

Use methods instead of properties to validate something. Properties are used to store or retrieve information. Using them otherwise will make your code harder to understand and maintain.

Another reason to remove the ValidateConnectionString and ValidateSqlCommand properties is that there doesn't seem to be any reason to store that information - and certainly not in multiple locations (SqlOpenConnection, SqlCloseConnection, ExecuteComplexCud).

Note that validation implies that the syntax or structure of the connection or command string will be checked. A simple null-or-empty check does not live up to that expectation. Either perform a full validation, or just remove those validation methods and use string.IsNullOrEmpty instead. And since the connection and command strings are passed as arguments, it's better to throw an ArgumentException or ArgumentNullException if they're invalid.

OConnection is not a descriptive name. I assume it's short for OpenConnection? If so, SqlCloseConnection.OpenConnection doesn't make any sense.

You may also want to add documentation. Describe the purpose of each interface and method, what input they consider invalid (and how they will react to that), what edge cases need to be taken into account and which exceptions they can throw.

Regarding '//have one function to Insert,Update and delete', do those methods have their own class or interface, or are they part of one of the interfaces you have shown here?

Conclusion

Without knowing the reasoning behind your design decisions, I'd say this code is overly complicated. It's making writing a correct program harder instead of easier. As for SOLID principles, I think there's too little context shown here to determine how well this code is adhering to them.

I hope you found this helpful. Feel free to add more context or explanation to your post if you want to receive more accurate feedback.

\$\endgroup\$
  • 2
    \$\begingroup\$ Great points in your "other notes" Consider this comment an additional +1 \$\endgroup\$ – Robert Snyder May 9 '16 at 13:02
5
\$\begingroup\$

Typical Conventions

Parameters to methods and local variables have a lower case first letter. Methods normally have all words spelled out completly. (specially calling out IConnectionProvider).

S - Single Responsibility Principle

With the very focused view point of the definition your classes adhear to the principle. Is it executed properly? No. DatabaseAttributes for instance. The name suggests it is a collection of attributes pertaining to a database. I suppose things like size, file, database type, version would match that type of qualification. Instead you have two properties that are validating the connection string and command string. Having a single validation class could arguably violate SRP, but maybe with some rewording and architecting maybe it coudl change to NotNullOrEmpty and could be used kind of like NotNullOrEmpty.Validate(connectionString); On the point of that class I think reversing your test logic woudl be in order. example:

set
{
    if (string.IsNullOrEmpty(value))
    {
        throw new Exception("No Connection String property detected.");
    }
    _connectionString = value;
}

L - Liskov substitution principle

Possible that I don't understand this one as well as I should, but you have 1 cited class and 1 mentioned class that implement IConnectionProvider. Naming aside, just by guessing what SqlOpenConnection and SqlCloseConnection does they are not able to be substituted one for the other. Now if you had an interface IOpenConnectionToDatabase and ICloseConnectionToDatabase and make each child class of those be able to work with any different type of provider then you'd have it.

D - Dependency Inversion principle

Entities must depend on abstractions not on concretions. It states that the high level module must not depend on the low level module, but they should depend on abstractions. You have code that relies on SqlConnection, SqlParameters, and all things Microsoft Sql Server. What if you wanted to switch to MySql, or Postrges, or NoSql.. etc etc. You wouldn't be able to because you are not relying on the abstraction. (hint, look at the System.Data.Common namespace..specifically System.Data.Common.DbProviderFactories.GetFactory("provider name here") )

\$\endgroup\$

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.