patterncsharpMinor
Yield statement in a using block from a clarity point of view
Viewed 0 times
statementpointblockclarityviewusingyieldfrom
Problem
I want an opinion of the community about readability and maintainability of the following code.
This is quite a rare case when yield statement included in a using construction.
I know that this code will work as intended and dispose will be called eventually.
But I'm worried it might be difficult for other developers to understand and reuse it.
This is quite a rare case when yield statement included in a using construction.
I know that this code will work as intended and dispose will be called eventually.
But I'm worried it might be difficult for other developers to understand and reuse it.
public IEnumerable GetUsers()
{
using (SqlDataAdapter adapter = new SqlDataAdapter())
{
DataSet ds = new DataSet();
string sql = @"SELECT lead FROM teams";
using (var sqlCnn = new SqlConnection(connetionString))
{
sqlCnn.Open();
using (var sqlCmd = new SqlCommand(sql, sqlCnn))
{
adapter.SelectCommand = sqlCmd;
adapter.Fill(ds);
foreach (DataRow row in ds.Tables[0].Rows)
{
var login = row[0].ToString();
yield return login;
}
}
}
}
}Solution
public IEnumerable GetUsers()
{
using (SqlDataAdapter adapter = new SqlDataAdapter())
{
DataSet ds = new DataSet();
string sql = @"SELECT lead FROM teams";
using (var sqlCnn = new SqlConnection(connetionString))
{
sqlCnn.Open();
using (var sqlCmd = new SqlCommand(sql, sqlCnn))
{
adapter.SelectCommand = sqlCmd;
adapter.Fill(ds);
foreach (DataRow row in ds.Tables[0].Rows)
{
var login = row[0].ToString();
yield return login;
}
}
}
}
}Usually you shouldn't worry about having a
yield return inside a using block but this isn't needed here at all. This is because the DataSet is disconnected from the database. The only thing you would buy would be that for an exception the SqlDataAdapter, SqlConnection and SqlCommand will be properly disposed if for instance the row[0] contains DBNull. I would extract the
foreach loop over the Rows out of the using blocks which I would also use different. The most outer one should enclose the SqlConnection followed by the SqlCommand and the last should be the SqlDataAdapter. By stacking the
using blocks you reduce the horizontal spacing which makes it easier to read. Because you are only interested in one
DataTable you should use the overloaded adapter.Fill(DataTable) method.Next to mention is that you should declare your variables as near to their usage as possible.
Applying this points will lead to
public IEnumerable GetUsers()
{
DataTable dataTable = new DataTable();
string sql = "SELECT lead FROM teams";
using (var sqlCnn = new SqlConnection(connetionString))
using (var sqlCmd = new SqlCommand(sql, sqlCnn))
using (SqlDataAdapter adapter = new SqlDataAdapter())
{
sqlCnn.Open();
adapter.SelectCommand = sqlCmd;
adapter.Fill(dataTable);
}
using (dataTable)
{
foreach (DataRow row in dataTable.Rows)
{
var login = row[0].ToString();
yield return login;
}
}
}Edit based on @t3chb0t comment
The data table is being created before the sql connection, if something goes wrong there, using(dataTable) will not be executed
nevertheless I am not sure it is needed.
public IEnumerable GetUsers()
{
string sql = "SELECT lead FROM teams";
using (var sqlCnn = new SqlConnection(connetionString))
using (var sqlCmd = new SqlCommand(sql, sqlCnn))
using (SqlDataAdapter adapter = new SqlDataAdapter())
using (DataTable dataTable = new DataTable())
{
sqlCnn.Open();
adapter.SelectCommand = sqlCmd;
adapter.Fill(dataTable);
sqlCnn.Close();
foreach (DataRow row in dataTable.Rows)
{
var login = row[0].ToString();
yield return login;
}
}
}Code Snippets
public IEnumerable<string> GetUsers()
{
using (SqlDataAdapter adapter = new SqlDataAdapter())
{
DataSet ds = new DataSet();
string sql = @"SELECT lead FROM teams";
using (var sqlCnn = new SqlConnection(connetionString))
{
sqlCnn.Open();
using (var sqlCmd = new SqlCommand(sql, sqlCnn))
{
adapter.SelectCommand = sqlCmd;
adapter.Fill(ds);
foreach (DataRow row in ds.Tables[0].Rows)
{
var login = row[0].ToString();
yield return login;
}
}
}
}
}public IEnumerable<string> GetUsers()
{
DataTable dataTable = new DataTable();
string sql = "SELECT lead FROM teams";
using (var sqlCnn = new SqlConnection(connetionString))
using (var sqlCmd = new SqlCommand(sql, sqlCnn))
using (SqlDataAdapter adapter = new SqlDataAdapter())
{
sqlCnn.Open();
adapter.SelectCommand = sqlCmd;
adapter.Fill(dataTable);
}
using (dataTable)
{
foreach (DataRow row in dataTable.Rows)
{
var login = row[0].ToString();
yield return login;
}
}
}public IEnumerable<string> GetUsers()
{
string sql = "SELECT lead FROM teams";
using (var sqlCnn = new SqlConnection(connetionString))
using (var sqlCmd = new SqlCommand(sql, sqlCnn))
using (SqlDataAdapter adapter = new SqlDataAdapter())
using (DataTable dataTable = new DataTable())
{
sqlCnn.Open();
adapter.SelectCommand = sqlCmd;
adapter.Fill(dataTable);
sqlCnn.Close();
foreach (DataRow row in dataTable.Rows)
{
var login = row[0].ToString();
yield return login;
}
}
}Context
StackExchange Code Review Q#109230, answer score: 6
Revisions (0)
No revisions yet.