HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Yield statement in a using block from a clarity point of view

Submitted by: @import:stackexchange-codereview··
0
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.

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.