Read from SQL database table, store in list of objects

Question

 IList<WebSite> wsl = new List<WebSite>();
 login1 l = new login1();

 string q = "select Id, DomainUrl, IsApproved, Date from website where UserId = @UserId";

 using (MySqlConnection con = new MySqlConnection(WebConfigurationManager.ConnectionStrings["MySqlConnectionString"].ToString()))
 {
      using (MySqlCommand cmd = new MySqlCommand(q, con))
      {
           cmd.Parameters.Add("@UserId", MySqlDbType.Int32).Value = userId;
           con.Open();

           var reader = cmd.ExecuteReader();
           while (reader.Read())
           {
                var ws = new WebSite();
                ws.Id = reader.GetInt32("Id");
                ws.DomainUrl = reader.GetString("DomainUrl");
                ws.IsApproved = reader.GetBoolean("IsApproved");
                ws.User.Id = reader.GetInt32("UserId");
                ws.Date = reader.GetDateTime("Date");
                wsl.Add(ws);
           }
           reader.Close();
           return wsl;
      }
 }

I want to make this code as elegant as possible.

For this particular project I cannot use NHibernate as I have been instructed.

Answer 1

This code is very tightly coupled with the specific database engine. That makes it hard to change if you decide to go with different database engine. And it's not possible to unit test it at all.

Instead of using the specific MySql classes for connections, etc., change it to use the base interfaces, and then pass the connections and the queries from outside.

Also, I generally avoid using "return" inside a code block like "using" and if/switch, if it does not make the code too clunky. That way the code path is much easier to follow.

Bellow is an example of this particular method as part of some class, which can be reused in many different situations, incl web pages, services, etc. and with different databases, w/o changing the implementation of this class itself.

private IDatabaseProvider _dbProvider; //initialized in ctor, etc.
private IConnectionStringProvider _connectionStringProvider;
private IWebSiteMapper _mapper; //the db schema may change, or you may need to map from other source

public IList<WebSite> GetWebSitesForUser(int userId)
{
    var wsl = new List<WebSite>();
    var con = _dbProvider.GetNewConnection(); //returns IDbConnection
    var cmd = _dbProvider.GetWebSiteListQueryCommand(con, userId); //IDbCommand, and addParam goes there

    conn.Open(); // no need of try/catch or using, as if this fails, no resources are leaked
    using (var reader = cmd.ExecuteReader(CommandBehavior.CloseConnection)) //close con on reader.Close
    {
         while (reader.Read())
         {
             wsl.Add(_mapper.CreateFromReader(reader));
         }
    }

    return wsl;
}

Answer 2

I would

• reduce the visibility of the variable wsl as it's not required outside the seconds using scope (unless you listen to peer)
• reduce the visibility of the variable q as it's not required outside the first using scope
• better yet I'd probably externalize the query string altogether
• encapsulate the construction of the connection (new MySqlConnection(WebConfigurationManager.ConnectionStrings["MySqlConnectionString"].ToString())) to a separate function createConnection() as the details of the process is not relevant in this context. The createConnection is then available for reuse and helps you to not repeat yourself
• define a builder for the WebSite instance especially if all the attributes are required e.g. 'WebSite.Builder().Id(reader.getInt32("ID").DomainUrl(...)...build()'. This would make it easier for you to ensure that each WebSite is correctly formed.
• separate iterating over the reader and constructing of the WebSite
• not set ws.User.Id as it seems somehow wrong to me.

It might look something like this:

 using (MySqlConnection con = Connect())
 {
      string q = "select Id, DomainUrl, IsApproved, Date from website where UserId = @UserId";
      using (MySqlCommand cmd = new MySqlCommand(q, con))
      {
           cmd.Parameters.Add("@UserId", MySqlDbType.Int32).Value = userId;
           con.Open();

           IList<WebSite> wsl = new List<WebSite>();
           using(var reader = cmd.ExecuteReader()) {
             foreach (WebSite webSite in CreateWebSites(reader)) 
             {
                wsl.Add(webSite);
             }
           }
           return wsl;
      }
 }

 private IEnumerable<WebSite> CreateWebSites(MySqlDataReader reader)
 {
    while (reader.Read())
    {
        yield CreateWebSite(reader);
    }
 }

 private MySQLConnection Connect() 
 {
    return new MySqlConnection(
       WebConfigurationManager.ConnectionStrings["MySqlConnectionString"].
       ToString());
 }

 private WebSite CreateWebSite(Reader reader) 
 {
    return WebSite.Builder().
                Id(reader.GetInt32("Id")).
                DomainUrl(reader.GetString("DomainUrl")).
                IsApproved(reader.GetBoolean("IsApproved")).
                User(reader.GetInt32("UserId"))
                Date(reader.GetDateTime("Date")).
                Build();
 }

Disclaimer: I don't know C# at all.