3
\$\begingroup\$

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);
        }
\$\endgroup\$
  • 1
    \$\begingroup\$ I note the misspelling in GetValueAsDobule \$\endgroup\$ – Snowbody Apr 13 '15 at 17:01
3
\$\begingroup\$
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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Piotr Nawrot Apr 14 '15 at 9:54
  • \$\begingroup\$ I was mistaken about ?? handling DBNull. It's passed through and have to be explicitly handled. \$\endgroup\$ – Piotr Nawrot Apr 14 '15 at 11:18

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.