6
\$\begingroup\$
 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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Two things: First, stop giving variables one-letter-names. Second, using the reader. \$\endgroup\$
    – Bobby
    Jul 22, 2011 at 6:36
  • \$\begingroup\$ move the return wsl; to the end of the function \$\endgroup\$
    – peer
    Jul 22, 2011 at 8:08
  • \$\begingroup\$ @Bobby and Peer (cannot notify two additional persons) - these comments should probably be answers as they answer the question. \$\endgroup\$ Jul 22, 2011 at 10:40

2 Answers 2

5
\$\begingroup\$

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;
}
\$\endgroup\$
4
  • \$\begingroup\$ I like this with the exception of using the reader. That cost you my +1. \$\endgroup\$
    – Chad
    Jul 22, 2011 at 16:26
  • \$\begingroup\$ @Chad: I'm not about the score. Could you elaborate more on the "reader" part? \$\endgroup\$
    – Sunny
    Jul 22, 2011 at 17:13
  • 1
    \$\begingroup\$ I dislike the reader because it keeps the connection open while doing processing. I prefer open connection - get data - close connecton - do processing. While reader works fine for small datasets, the processing time of a few milliseconds adds up when you get into thousands of records. \$\endgroup\$
    – Chad
    Jul 22, 2011 at 17:17
  • \$\begingroup\$ what alternative to the reader? \$\endgroup\$
    – Kyle
    Jul 30, 2011 at 0:21
2
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Wow..thats really good for not being familiar with c#. I going to take your advice \$\endgroup\$
    – Luke101
    Jul 22, 2011 at 14:40
  • \$\begingroup\$ Impressive for non-C# dev :). Just for the record: with .Net 4.0 there's no need of Builder, you can use var ws = new WebSite {Prop1 = x, Prop2=y}; \$\endgroup\$
    – Sunny
    Jul 22, 2011 at 15:16
  • \$\begingroup\$ Good to know if I ever venture to the .net side. It's very tempting but I'd have to use Mono. \$\endgroup\$ Jul 22, 2011 at 15:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.