1
\$\begingroup\$

I came across this code in a project I inherited. I am curious if all the nested using statements make sense to anyone else. Also, wondering if this code could not be written in a better/leaner way?

    private static Dictionary<string, List<string>> GetSubscriptionKeyList(string proc)
    {
        var addResults = new List<string>();
        var deleteResults = new List<string>();

        using (var conn = new OracleConnection(conStr))
        {
            conn.Open();
            using (var cmd = new OracleCommand(proc, conn))
            {
                cmd.CommandType = CommandType.StoredProcedure;

                using (var adds = new OracleParameter
                {
                    ParameterName = "p_add",
                    OracleDbType = OracleDbType.RefCursor,
                    Direction = ParameterDirection.Output,
                    Value = DBNull.Value
                })
                {
                    using (var deletes = new OracleParameter
                    {
                        ParameterName = "p_delete",
                        OracleDbType = OracleDbType.RefCursor,
                        Direction = ParameterDirection.Output,
                        Value = DBNull.Value
                    })
                    {
                        cmd.Parameters.Add(adds);
                        cmd.Parameters.Add(deletes);

                        using (var rdr = cmd.ExecuteReader())
                        {
                            while (rdr.Read())
                            {
                                addResults.Add(rdr.GetString(0));
                            }
                            rdr.NextResult();
                            while (rdr.Read())
                            {
                                deleteResults.Add(rdr.GetString(0));
                            }

                        }
                    }
                }
            }
            conn.Close();
        }
        var result = new Dictionary<string, List<string>>
                         {
                             {"Adds", addResults},
                             {"Deletes", deleteResults}
                         };
        return result;
    }
\$\endgroup\$
3
  • \$\begingroup\$ What framework/nuget are you using? I cannot find the OracleDbType property on the OracleParameter. There's only OracleType. \$\endgroup\$
    – t3chb0t
    May 22, 2018 at 16:04
  • \$\begingroup\$ @t3chb0t Oracle.DataAccess.Client.OracleDbType \$\endgroup\$
    – ADH
    May 22, 2018 at 16:35
  • \$\begingroup\$ Could you add the stored procedure too? \$\endgroup\$
    – t3chb0t
    May 22, 2018 at 17:31

2 Answers 2

2
\$\begingroup\$

I could see using it for the connection, command and reader since they implement IDisposable and the documentation for them does say to always call Dispose().

For the parameters it seems not only unnecessary but in my opinion it really decreases readability. I would kill the using statements for the parameters and make them simple declarations.

For what it's worth, here's how I'd write it.

private static Dictionary<string, List<string>> GetSubscriptionKeyList(string proc)
{

    var result = new Dictionary<string, List<string>>();
    result.Add("Adds", new List<string>());
    result.Add("Deletes", new List<string>());

    using (var conn = new OracleConnection(conStr))
    {
        conn.Open();
        using (var cmd = new OracleCommand(proc, conn))
        {
            cmd.CommandType = CommandType.StoredProcedure;

            cmd.Parameters.Add(new OracleParameter("p_add", OracleDbType.RefCursor, ParameterDirection.Output));
            cmd.Parameters.Add(new OracleParameter("p_delete", OracleDbType.RefCursor, ParameterDirection.Output));

            using (var rdr = cmd.ExecuteReader())
            {
                while (rdr.Read())
                {
                    result["Adds"].Add(rdr.GetString(0));
                }
                rdr.NextResult();
                while (rdr.Read())
                {
                    result["Deletes"].Add(rdr.GetString(0));
                }

            }
        }
        conn.Close();
    }
    return result;
}

Obviously some things are a matter of taste, like whether to declare the parameters on a separate line and whether to keep the initializers there for the dictionary. (I thought it was more clear to separate them.)

\$\endgroup\$
1
\$\begingroup\$
using (var rdr = cmd.ExecuteReader())
{
    while (rdr.Read())
    {
        addResults.Add(rdr.GetString(0));
    }
    rdr.NextResult();
    while (rdr.Read())
    {
        deleteResults.Add(rdr.GetString(0));
    }
}

I don't think it's such a good idea to rely on the result order of your stored procedure.

You assume that the first row is always adds and the second row is deletes. I find it'd be better if you included this in the result themself and then created a dictionary based on the value in that specific column.

\$\endgroup\$
1
  • \$\begingroup\$ I agree this looks unusual. I will look into getting this changed. \$\endgroup\$
    – ADH
    May 23, 2018 at 12:09

Your Answer

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

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