Problem
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.
Solution
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;
}
I would
- reduce the visibility of the variable
wsl
as it’s not required outside the secondsusing
scope (unless you listen to peer) - reduce the visibility of the variable
q
as it’s not required outside the firstusing
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 functioncreateConnection()
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.