Return to Answer

###DBCommand

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.

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.

What does the abstract ConnectionManager class buys you ? Not really anything. 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 usefuul 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 are implementing that interface, handle the validation of the connectionstring themself.

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

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

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

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

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

I like that you are using readonly for the variables which aren't allowed to change. Thats 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.

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 underlaying database system.

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 ?

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

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

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.

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.

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.