3
\$\begingroup\$

Here is a short and simple Ajax method that returns "True" or "False" if an entity exists in a database via a stored procedure that returns just "Y" or "N" (the details of this entity and database are not relevant to my question though).

This is the first time I've used the C# using() statement, and was wondering if anyone would be kind enough to review this and give me feedback.

[WebMethod]
public string ValidateEntity(string EntityType, string EntityName)
{
    string connstr = (from c in Companys where c.Name.Equals(company, StringComparison.OrdinalIgnoreCase) select c.ConnectionString).FirstOrDefault();
    if (connstr == null) { return "False"; }
    using (SqlConnection conn = new SqlConnection(connstr))
    {
        using (SqlDataAdapter da = new SqlDataAdapter())
        {
            using (da.SelectCommand = new SqlCommand("ValidateEntity", conn))
            {
                da.SelectCommand.CommandType = CommandType.StoredProcedure;
                da.SelectCommand.Parameters.AddWithValue("@EntityType", EntityType);
                da.SelectCommand.Parameters.AddWithValue("@EntityName", EntityName);
                using(DataSet ds = new DataSet())
                {
                    da.Fill(ds, "result_name");
                    DataTable dt = ds.Tables["result_name"];
                    if ( dt.Rows.Count > 0){
                        if (dt.Rows[0]["Valid"].ToString()=="Y") { return "True"; }
                    }                             
                }
            }
        }
    }
    return "False";
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the helpful and informative responses, ckuhn203 and Carlos, and thank you for fixing the formatting, Jamal! \$\endgroup\$ – ChrisGNZ Jul 25 '14 at 1:39
5
\$\begingroup\$
  • I may be pointing out the obvious here, but the ValidateEntity Method returns a string. I would expect it to return a Boolean.
  • There are a number of one and two letter variable names. We've not been restricted in the size of names for many years now. It would be good to give them more descriptive names and we can take advantage of C#'s case sensitivity.
    • da = adapter
    • ds = dataSet (but perhaps something even more meaningful)
    • dt = table or resultNames
  • If you return a bool instead of a string, you could directly return here instead of using an if statement.

    if (dt.Rows[0]["Valid"].ToString()=="Y") { return "True"; }
    

Becomes

return (dt.Rows[0]["Valid"].ToString() == "Y");
\$\endgroup\$
4
\$\begingroup\$

It's possible to omit the {} in the using statements, much like with an if statement. If you're not doing anything until an inner block you can just write

using (bla1 = ...)
using (bla2 = sth(bla1))
using (bl3 = sth(bla2))
{
//Do the real work here
}
//Stuff is cleaned up here as if you'd done lots of {}

It's a matter of style, mainly. Since it isn't an if condition, probably most readers can figure out what's happening.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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