6
\$\begingroup\$

I am doing the following about 10 times with other strings, so the code is duplicated. How can I refactor this?

queryWhere is a Dictionary<string,string> that contains parameters that will be passed to a query.

string account = string.Empty;

if (queryWhere.ContainsKey("account") 
&& queryWhere["account"] != null 
&& !string.IsNullOrEmpty(queryWhere["account"].ToString()))
    account = queryWhere["account"].ToString();

string customer = string.Empty;

if (queryWhere.ContainsKey("customer ") 
&& queryWhere["customer "] != null 
&& !string.IsNullOrEmpty(queryWhere["customer "].ToString()))
    customer = queryWhere["customer "].ToString();


string balance = string.Empty;

if (queryWhere.ContainsKey("balance ") 
&& queryWhere["balance "] != null 
&& !string.IsNullOrEmpty(queryWhere["balance "].ToString()))
    balance = queryWhere["balance"].ToString();
\$\endgroup\$
3
  • 5
    \$\begingroup\$ There's probably a lot to say about this, but the root of your problem may not be in this specific piece of your code. For example, why are you in a situation at all where you may have empty or null values that you don't consider valid? We could probably give a proper review if you included more of the code. \$\endgroup\$ Apr 17, 2015 at 15:27
  • \$\begingroup\$ What is queryWhere? \$\endgroup\$
    – BCdotWEB
    Apr 17, 2015 at 15:30
  • \$\begingroup\$ @BCdotWEB - queryWhere is just a Dictionary<string,string> that contains parameters that will be passed to a query. \$\endgroup\$
    – xaisoft
    Apr 17, 2015 at 15:31

2 Answers 2

17
\$\begingroup\$

Each of these:

string account = string.Empty;

if (queryWhere.ContainsKey("account") 
   && queryWhere["account"] != null 
   && !string.IsNullOrEmpty(queryWhere["account"].ToString()))
{
    account = queryWhere["account"].ToString();
}

Should be reduced to:

string account;
if(!queryWhere.TryGetValue("account", out account))
{
   account = string.Empty; // if you need the string to be empty // default is null
}

Dictionary.TryGetValue

But that logic can then be moved to a method:

private string GetValue(string key)
{
   string returnValue;
   if(!queryWhere.TryGetValue(key, out returnValue))
   {
      returnValue= string.Empty;
   }
   return returnValue;
}

string account = GetValue("account");
string customer = GetValue("customer");

I really don't see the point of your original code, BTW. For instance, the .ToString() is completely superfluous, since you're working with a Dictionary<string,string>. It is always going to return a string.

But why do you even check for string.IsNullOrEmpty()? You already know it isn't null from the previous line's check -- queryWhere["account"] != null -- so at worst it is empty which is your default value anyway!

\$\endgroup\$
2
  • \$\begingroup\$ I like this, much more concise, so if I don't care about the default, is all that is required is queryWhere.TryGetValue(key, out returnValue) \$\endgroup\$
    – xaisoft
    Apr 17, 2015 at 15:53
  • \$\begingroup\$ @xaisoft Indeed, plus it is the recommended way to handle such lookups if you're not certain the key is present in the dictionary. And it is more efficient. \$\endgroup\$
    – BCdotWEB
    Apr 17, 2015 at 19:16
4
\$\begingroup\$

Extract a method. Excuse the names here, but naming is hard.

public string GetIfValid(string fieldName)
{
    if (queryWhere.ContainsKey(fieldName) 
        && queryWhere[fieldName] != null 
        && !string.IsNullOrEmpty(queryWhere[fieldName].ToString())
        ) 
    {
        return queryWhere[fieldName].ToString();
    }

    return string.Empty;
}

string account = GetIfValid("account");
string customer = GetIfValid("customer");
string balance = GetIfValid("balance");
\$\endgroup\$
0

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