current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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";
}
share|improve this question
    
Thanks for the helpful and informative responses, ckuhn203 and Carlos, and thank you for fixing the formatting, Jamal! –  ChrisGNZ Jul 25 '14 at 1:39

2 Answers 2

up vote 4 down vote

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.

share|improve this answer
up vote 5 down vote
  • 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");
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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