Return to Revisions

What does the abstract ConnectionManager class buy you ? Nothing, really. The passed-in passedOnConstring is only used for validation, the ConnectionString property is private and isn't used at all. The only thing which is useful is that you can pass a class which extends that class around and call the abstract methods. A much better way would be to define an interface doing the same and let the classes, which implement that interface, handle the validation of the connection string themselves.

What does this class manage ? It is only opening and closing a connection but doesn't manage anything.

The other problem I see is that you have an abstract method which returns a SqlConnection which is only used by MS Sql. What if you decide to use MySql or PostgreSql ?

There is the DbConnection which is extended by SqlConnection and at least by MySqlConnection and I suggest using this instead.

That being said, let's dig through your code.

public ConnectionManager(string passedOnConstring)
{
 if (!(String.IsNullOrWhiteSpace(passedOnConstring))
 {
 ConnectionString = passedOnConstring;
 }
 else
 {
 throw new ArgumentException("Invalid string connection, it cannot be null or empty.");
 }
} 

It's much easier to read if one isn't using "negative" results and this makes the use of an else redundant as well, like so

 public ConnectionManager(string passedOnConstring)
 {
 if ((String.IsNullOrWhiteSpace(passedOnConstring))
 {
 throw new ArgumentException("Invalid string connection, it cannot be null or empty."); 
 }
 ConnectionString = passedOnConstring;
 }

and the exception message would read better "Invalid connection string...."

public override SqlConnection OpenConnection()
{
 try
 {
 _dbconnect = new SqlConnection(_conString);
 if (_dbconnect.State != System.Data.ConnectionState.Open)
 {
 _dbconnect.Open();
 }
 return _dbconnect;
 }
 catch (SqlException)
 {
 throw;
 }
} 

This try..catch is superfluous because if a SqlException is thrown you are rethrowing it and all other possible exceptions are not caught. You can simply remove that try..catch. But what bothers me more is that you never ever dispose any of the created connections.

DBCommand

I like that you are using readonly for the variables which aren't allowed to change. That's a good habit but this also results in this case that your class can be used only for one trip to the database because the dictionary you are using for the execution (削除) can't be changed (削除ここまで) can't be assigned to the variable twice. Sure that wouldn't mean that you can't change the content of that Dictionary but you could do this, having your current code, only from the outside of the class which I would consider to be a side effect because you are changing the state of the ConnectionManager class from outside of the class.

For readonly please see @EBrown's good answer.

If you would pass that dictionary to the ExecCmdwithParams() method instead you could reuse the object.

I would like to suggest that you dig into the DbConnection, DbCommand, DbProviderFactory, DbProviderFactories and DbDataAdapter classes which are all living in the System.Data.Common namespace. Using this classes will abstract away the underlying database system.