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();
Improve this question
\$\endgroup\$
3
  • 4
    \$\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\$ – Ben Aaronson Apr 17 '15 at 15:27
  • \$\begingroup\$ What is queryWhere? \$\endgroup\$ – BCdotWEB Apr 17 '15 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 '15 at 15:31
15
\$\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!

Improve this answer
\$\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 '15 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 '15 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");
Improve this answer
\$\endgroup\$
0
0
\$\begingroup\$ 
var result= YourDictionaryName.TryGetValue(key, out var value) ? value : "";

If the key is present in the dictionary, it returns the value of the key otherwise it returns a blank object.

Hope, this code helps you.

Improve this answer
\$\endgroup\$
8
  • \$\begingroup\$ I'd replace YourDictionaryName[key] with value - TryGetValue already gives you the value, so there's no need for an extra dictionary lookup. Either way, here on Code Review you're expected to review the given code. Presenting an alternative approach is fine, but you should explain why it's better, so the OP can improve their code and programming skills. \$\endgroup\$ – Pieter Witvoet May 31 '19 at 8:26
  • \$\begingroup\$ @PieterWitvoet - if the dictionary does not contain a specific key 'YourDictionaryName[key]' it gives an error, so first I check that the key is present or not, then fetch value otherwise gives blank value. \$\endgroup\$ – Nitika Chopra May 31 '19 at 9:22
  • \$\begingroup\$ I understand what you're doing, yes. But it's doing the same work twice (both TryGetValue and [key] do a lookup), which is not very efficient. var result = dict.TryGetValue(key, out var value) ? value : ""; gives the same result with only a single lookup. \$\endgroup\$ – Pieter Witvoet May 31 '19 at 9:48
  • \$\begingroup\$ yup.. you are right... Now, I edit my answer.. please check.. @PieterWitvoet \$\endgroup\$ – Nitika Chopra May 31 '19 at 10:08
  • \$\begingroup\$ That's better. Still, your post is not a review of the original code, so it's not a good answer here on Code Review (this 'sub-site' has different rules than the 'main' StackOverflow site). \$\endgroup\$ – Pieter Witvoet May 31 '19 at 11:29

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.