Getting a single value from the DB

Question

This is supposed to get a single value from the DB.

What should I consider next? How can I upgrade it?

public static String GetConnectionString()
 {
 return ConfigurationManager.ConnectionStrings["xxx"].ConnectionString;
 }
 public static String GetProviderName()
 {
 return ConfigurationManager.ConnectionStrings["xxx"].ProviderName;
 }
 private Object GetValue(string cmdTxt) 
 {
 try
 {
 using (SqlConnection connection = new SqlConnection(GetConnectionString()))
 using (SqlCommand command = new SqlCommand(cmdTxt, connection))
 {
 connection.Open();
 return command.ExecuteScalar() ?? null;
 }
 }
 catch (Exception e)
 {
 try
 {
 log.Error(string.Format("SELECT User: {0} Base: {3} SQL:{2} Error:{1}", (Session["UserData"] as UserData).id_user, e.Message, cmdTxt, Session["ClientBASE"].ToString()));
 }
 catch { log.Error(string.Format("SELECT User: {0} SQL:{2} Error:{1}", (Session["UserData"] as UserData).id_user, e.Message, cmdTxt)); }
 return null;
 }
 }
 public bool GetValueAsBoolean(string cmdTxt) 
 {
 return Convert.ToBoolean(this.GetValue(cmdTxt) ?? false);
 }
 public string GetValueAsString(string cmdTxt)
 {
 return (this.GetValue(cmdTxt) ?? String.Empty).ToString();
 }
 public int GetValueAsInt(string cmdTxt)
 {
 return Convert.ToInt32(this.GetValue(cmdTxt) ?? 0);
 }
 public double GetValueAsDobule(string cmdTxt)
 {
 return Convert.ToDouble(this.GetValue(cmdTxt) ?? 0.0);
 }

• Snowbody: I note the misspelling in GetValueAsDobule

Answer 1

public static String GetConnectionString()

You shouldn't have to use this outside of the class so make it private. Furthermore I would turn it into a property and set it from the static constructor -- that way you avoid having to query your configuration each time you want to make a new call.

This is assuming you don't do it deliberately like this so you can change the connectionstring without halting the application.

---

cmdTxt

What's that, commandtext? Something more descriptive would be helpful, perhaps just "command" or "query" would be okay.

---

return command.ExecuteScalar() ?? null;

"if execution returns null, return null".

Might as well just return command.ExecuteScalar();.

---

string.Format("SELECT User: {0} Base: {3} SQL:{2} Error:{1}"

Keep the indices in ascending order -- it's what everybody expects and you won't have silly mistakes because the arguments passed in expected 0-1-2-3 instead.

---

Why use a try-catch and not a simple if(Session["ClientBase"] == null)? Exception handling is expensive and shouldn't be used for normal application flow.

---

GetValueAsBoolean
GetValueAsString
GetValueAsInt
GetValueAsDobule

I would consider making this a generic GetValue<T> and do the conversion inside that method while throwing a NotSupportedException for types that you don't want to support. The above idiom smells too much like Java and we're better than that.

Notice also the typo Dobule.

• Piotr Nawrot: Great feedback, thanks! I definitely use your advices. But I'd like to explain myself :-) 1. return command.ExecuteScalar() ?? null; ExecuteScalar returns DBNull not null. What is interesting ??-operator treats this as null but Convert throws InvalidCastException: Object cannot be cast from DBNull to other types. This was the most compact sollution what I could think of. 2. public static String GetConnectionString() is public because I'm in the middle of refactoring. Earlier value was hardcoded, in many classes. I made method public as first step.
• Piotr Nawrot: I was mistaken about ?? handling DBNull. It's passed through and have to be explicitly handled.