4
\$\begingroup\$

I have a Web API 2 using Dapper.net, and it's accessed by various client with their own database. Currently, I'm using custom header called Client-Id to identify the client.

This Client-Id contains a ConnectionString name and I passed this connection string into my constructor.

Here's my project structure:

Api.Persentation -> Api.Service -> Api.Logic

The connection string will pass from Api.Persentation to Api.Logic as follows:

Api.Persentation

public string testApi()
{
//using the service
//consider the connection string was handled on base class
var _myService = new testApiService(base.ConnectionString); 

return _myService.HelloWorld();
}

Api.Service

public class testApiService: BaseService
{
    #region const
    //This class was from API.Logic
    private readonly testApiRepo _testApiRepo;

    //the connectionString passed into base class to use a log repo
    public testApiService(string connectionString)
        : base(connectionString)
    {
        this._testApiRepo = new testApiRepo(connectionString);
    }
    #endregion


    public string HelloWorld()
    {
        //access API.Logic
        return this._testApiRepo.GetHelloWorld();
    }
}

Api.Logic

It has two classes, where BaseConnection is an abstract class to open the connection (the class how I use ConnectionString). The second one is, testApiRepo showing how I use the connection to get a database value.

/*-------------
BaseConnection Class
-------------*/
    public abstract class BaseConnection
    {
        private string _connectionString;
        public BaseConnection(string ConnectionString)
        {
            this._connectionString = ConnectionString;
        }

        public MyDbConnection GetOpenConnection(bool mars = false)
        {
            string cs = this._connectionString;
            if (mars)
            {
                SqlConnectionStringBuilder scsb = new SqlConnectionStringBuilder(cs);
                scsb.MultipleActiveResultSets = true;
                cs = scsb.ConnectionString;
            }

            //Finally we use the connection string in here
            var connection = new SqlConnection(ConfigurationManager.ConnectionStrings[this._connectionString].ToString());
            MyDbConnection DbConnection = new MyDbConnection(connection);
            DbConnection.Open();
            return DbConnection;
        }
    }

/*-------------
testApiRepo Class
-------------*/
    public class testApiRepo : BaseConnection
    {
        #region Const
        //the connection will handled on base class
        public CommissionRepo(string connectionString)
            : base(connectionString)
        {
        }
        #endregion

        public string GetHelloWorld()
        {
        //using base class to open connection
        using (var _db = base.GetOpenConnection())
        {
            string query = "SELECT 'HelloWorld'";
            var result = _db.Query<string>(query);
            return result.FirstOrDefault();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Watch out for typos like "Persentation". \$\endgroup\$ – BCdotWEB Jan 15 '16 at 11:10
3
\$\begingroup\$

First of all I would highlight a security problem: you're directly exposing your implementation to service users. They just need to try different Client-IDs to connect to different databases. Connection string should be deduced by your application logic from client's authentication (whatever you use for this).


testApi() does not follow common .NET naming guidelines, method names should be Pascal case: TestApi() (also I would consider to use a little bit more descriptive name, TestServiceConnection(), maybe). The same apply for testApiService (TestApiService) and your local variable _myService (prefixes such as _ are controversial but - when used - they denote member fields, not local variables or parameters).

Also classes testApiService and testApiRepo has same naming issue.


  • BaseConnection is abstract, you'd better also declare its constructor as protected.
  • _connectionString is initialized only in constructor then it should be marked as readonly.
  • ConnectionString constructor parameter should be camel case: connectionString.

In GetOpenConnection() you're using a boolean flag mars, they should be used sparely because reading GetOpenConnection(true) you don't immediately understand what it means (true...what?) and whenever possible they should be replaced with enum or different methods. I'd also consider to do not have a default value: do you really call it so often you want to save those few characters?


Be careful when you create and return disposable objects, if something goes wrong creating MyDbConnection (again it should be myDbConnection, also better name is required) object then an exception is thrown and connection isn't immediately destroyed: when a SQL connection is popped from pool is an implementation detail you should not rely on.


All your classes are public and none of them are sealed. Will they be used outside assembly where you declared them? If not then they should not be public. Also TestApiRepo should be renamed and marked as sealed.


There isn't much more I can say about these snippets but I think you should change little bit your architecture. For example what you call BaseConnection in reality is not a connection! It's a factory class where you build connection objects (MyDbConnection) and testApiRepo has nothing to do with that factory (it should not be a derived class) but, instead, accept factory as parameter (or a connection string and then instantiate your ConnectionFactory class).

\$\endgroup\$
  • \$\begingroup\$ thank you for answer my question, it's good review. however I need to know about the way I passed ConnectionString name from my controller to BaseConnection class. Is it a good approach? \$\endgroup\$ – vantian Jan 16 '16 at 12:39
  • 1
    \$\begingroup\$ I don't see controller so I don't have much context but if you mean "pass connection name through a constructor parameter" then yes. Just remember to be careful it's a connection name (not a connection string) when mars is true and user is authorized to use it. If your factory caches anything shared between multiple requests/users then you may also consider to use a property (but I wouldn't start with something more complicate than necessary) \$\endgroup\$ – Adriano Repetti Jan 16 '16 at 12:44
  • \$\begingroup\$ Also check BCDOTWEB's answer, he highlighted other points I didn't see! \$\endgroup\$ – Adriano Repetti Jan 16 '16 at 12:45
  • \$\begingroup\$ actually it's not a human readable I do an encryption on my Client-Id (*it's the header request on API to get connection name) and decrypt on my MessageHandler. also, I'm going to add autofac to my API soon and I new on implementing this. I read the documentation that it has to do with constructor. I wonder wheather my connection name would affect to Dependency Injection? \$\endgroup\$ – vantian Jan 16 '16 at 12:58
  • \$\begingroup\$ If you use autofac then don't worry, it supports parameters! \$\endgroup\$ – Adriano Repetti Jan 16 '16 at 13:04
1
\$\begingroup\$

In addition to Adriano's remarks, some quick additional tips:

  • Avoid #region, especially when it's a 20 line class.

  • Comments should be used to communicate why something was implemented in that particular way. Ones like //access API.Logic or //Finally we use the connection string in here don't tell me anything the code isn't already saying.

  • Naming is hard, I know, but MyDbConnection as a class name isn't useful. I have no idea what mars means.

  • Avoid abbreviations like cs, just name it connectionString. (Same for scsb) Moreover, why is it even there? You don't do anything with cs.

  • Unless I'm wrong, _connectionString doesn't contain a connectionString, but it contains its name.

  • Code like SqlConnectionStringBuilder scsb = new SqlConnectionStringBuilder(cs); scsb.MultipleActiveResultSets = true; looks outdated to me. Why not simply write var sqlConnectionStringBuilder = new SqlConnectionStringBuilder(cs) { MultipleActiveResultSets = true };?

  • Moreover, the whole "get the connection string" logic really belongs in a separate method instead of being part of GetOpenConnection.

\$\endgroup\$
  • \$\begingroup\$ thank you for your review, I'm really curious about the separate method you talking about. How should I transfer the connection string name to my BaseConnection class? \$\endgroup\$ – vantian Jan 16 '16 at 12:42

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.