Wrapper for commonly used database classes

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 call

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.

StackExchange Code Review Q#127887, answer score: 13