current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1
 
 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.

share|improve this question
1  
Two things: First, stop giving variables one-letter-names. Second, using the reader. –  Bobby Jul 22 '11 at 6:36
    
move the return wsl; to the end of the function –  peer Jul 22 '11 at 8:08
    
@Bobby and Peer (cannot notify two additional persons) - these comments should probably be answers as they answer the question. –  Aleksi Yrttiaho Jul 22 '11 at 10:40

2 Answers 2

up vote 4 down vote accepted

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;
}
share|improve this answer
    
I like this with the exception of using the reader. That cost you my +1. –  Chad Jul 22 '11 at 16:26
    
@Chad: I'm not about the score. Could you elaborate more on the "reader" part? –  Sunny Jul 22 '11 at 17:13
1  
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. –  Chad Jul 22 '11 at 17:17
    
what alternative to the reader? –  Kyle Hodgson Jul 30 '11 at 0:21
up vote 2 down vote

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.

share|improve this answer
    
Wow..thats really good for not being familiar with c#. I going to take your advice –  Luke101 Jul 22 '11 at 14:40
    
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}; –  Sunny Jul 22 '11 at 15:16
    
Good to know if I ever venture to the .net side. It's very tempting but I'd have to use Mono. –  Aleksi Yrttiaho Jul 22 '11 at 15:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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