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 1 down vote favorite

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();
share|improve this question
3  
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. –  Ben Aaronson Apr 17 at 15:27
    
What is queryWhere? –  BCdotWEB Apr 17 at 15:30
    
@BCdotWEB - queryWhere is just a Dictionary<string,string> that contains parameters that will be passed to a query. –  xaisoft Apr 17 at 15:31

2 Answers 2

up vote 6 down vote accepted

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!

share|improve this answer
    
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) –  xaisoft Apr 17 at 15:53
    
@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. –  BCdotWEB Apr 17 at 19:16
up vote 4 down vote

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["balance"].ToString();
    }

    return string.Empty;
}

string account = GetIfValid("account");
string customer = GetIfValid("customer");
string balance = GetIfValid("balance");
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.