Return Dictionary object having key/value pair of keywords and URLs

Question

Can you please help me split this method to reuse the repeating code?

Any advice/comments on the code are welcome.

//This method will return Dictionary object having key/value pair of keywords and URLs.
//Keywords are single word or bunch of words from db.
//Value in dictionary is URLs against each key (keyword).
//Code starts from here.
private static Dictionary<string, string> GetKeywordsAndEntityWithURL(int eventTypeId, string entityName)
{
 // Get initial url from configuration file.
 string basewebUrl = CommonMethods.GetAppSettingsValue("Baseweb");
 // Get all keywords from db, from KeyWords table.
 KeywordsCollection keyWords = KeywordsEntity.GetKeywords(null, eventTypeId);
 // This text will be concatenated with entityname.
 // Purpose of this is to avoid replacement of EntityName again in find/replace.
 string dummyText = "TemporaryText";
 // This object will be used to create key/value pair of keys and URLs.
 Dictionary<string, string> keyWordsWithURLs = new Dictionary<string, string>();
 // There are two types of keywords in db, Singular and Plural. Get each and create key/value using them.
 foreach (KeywordsEntity keyWord in keyWords)
 {
 string singularKeyword = keyWord.Keyword;
 // Keywords are like 'stag do' in db we need to replace white space
 // with hyphen to use this in URLs.
 singularKeyword = singularKeyword.Replace(' ', '-');
 // This key is like 'stag do London'.
 string singularKey = string.Empty;
 singularKey = string.Format("{0} {1}", keyWord.Keyword.ToString(), entityName);
 // Concat entityName (London) with dummytext.
 entityName = entityName + dummyText;
 // Text of URL e.g. stag do London
 string urlText = keyWord.Keyword + " " + entityName;
 // This value is like initial url (whatever it is in config file)/stag-do/london
 string singularValue = string.Empty;
 singularValue = string.Format("<a style='text-decoration:underline;' href='" + basewebUrl + "/{0}/{1}'>{2}</a>", singularKeyword, entityName, urlText);
 // Add key/value in dictionaly.
 keyWordsWithURLs.Add(singularKey, singularValue);
 // Plural keywords are like 'stag dos' in db we need to replace space with hyphen for URLs.
 string pluralKeyword = keyWord.PluralKeyword;
 pluralKeyword = pluralKeyword.Replace(' ', '-');
 // This key would be 'stag dos london'
 string pluralKey = string.Empty;
 pluralKey = string.Format("{0} {1}", keyWord.PluralKeyword.ToString(), entityName);
 // URL would be, initial url(whatever in config file)/ stag-dos/london
 string pluralValue = string.Empty;
 pluralValue = string.Format("<a style='text-decoration:underline;' href='" + basewebUrl + "/{0}/{1}'>{2}</a>", pluralKeyword, entityName, urlText);
 // Add key/value in dictionary.
 keyWordsWithURLs.Add(pluralKey, pluralValue);
 // Singular Key = London stag do
 singularKey = string.Format("{0} {1}", entityName, keyWord.Keyword.ToString());
 // Text of anchor e.g. London stag do.
 urlText = entityName + dummyText + " " + keyWord.Keyword;
 // Singular Value = /stag-do/london
 singularValue = string.Format("<a style='text-decoration:underline;' href='" + basewebUrl + "/{0}/{1}'>{2}</a>", singularKeyword, entityName, urlText);
 keyWordsWithURLs.Add(singularKey, singularValue);
 // Plural Key = London stag dos
 pluralKey = string.Format("{0} {1}", entityName, keyWord.PluralKeyword.ToString());
 // Plural Key = /stag-dos/lonon
 pluralValue = string.Format("<a style='text-decoration:underline;' href='" + basewebUrl + "/{0}/{1}'>{2}</a>", pluralKeyword, entityName, urlText);
 // Add key/value pair in dictionary.
 keyWordsWithURLs.Add(pluralKey, pluralValue);
 urlText = keyWord.Keyword + " in " + entityName + dummyText;
 singularKey = string.Format("{0} {1} {2}", keyWord.Keyword.ToString(), "in", entityName);
 singularValue = string.Format("<a style='text-decoration:underline;' href='" + basewebUrl + "/{0}/{1}'>{2}</a>", singularKeyword, entityName, urlText);
 // Add key/value in dictionaly.
 keyWordsWithURLs.Add(singularKey, singularValue);
 pluralKey = string.Format("{0} {1} {2}", keyWord.PluralKeyword.ToString(), "in", entityName);
 pluralValue = string.Format("<a style='text-decoration:underline;' href='" + basewebUrl + "/{0}/{1}'>{2}</a>", pluralKeyword, entityName, urlText);
 // Add key/value in dictionary.
 keyWordsWithURLs.Add(pluralKey, pluralValue);
 if (keyWord.IsDefault)
 {
 pluralKeyword = keyWord.PluralKeyword.Replace(' ', '-');
 pluralKey = string.Format("{0}", entityName);
 pluralValue = string.Format("<a style='text-decoration:underline;' href='" + basewebUrl + "/{0}/{1}'>{2}</a>", pluralKeyword, entityName, entityName);
 keyWordsWithURLs.Add(pluralKey, pluralValue);
 }
 }
 return keyWordsWithURLs;
}

If you need the full class, please write a comment. I will upload it.

Answer 1

I would probably start by cleaning up the method before trying to extract anything as it will make it easier to see where to draw the line.

• There are a couple of places where you define a variable with an initial value and then override that value on the next line (singularKeyword, singularKey)

  string singularKeyword = keyWord.Keyword.singularKeyword.Replace(' ', '-');
  string singularKey = string.Format("{0} {1}", keyWord.Keyword.ToString(), entityName);
  

• You shouldn't need the call to ToString in the above line either as string.Format() will do it for you.

• Any time you find yourself appending to a string in a loop, you should consider if using StringBuilder would be better.

• I generally prefer not to modify arguments, particularly in such a large method as it tends to confuse it's meaning.

• Why pass a string literal into string.Format instead of including it in the format string?

  singularKey = string.Format("{0} in {1}", keyWord.Keyword.ToString(), entityName);
  

• I noticed that in several places you append basewebUrl to directly the format string instead of passing it in as another arg?

• The whole string.Format call in this following line seems a bit redundant.

  pluralKey = string.Format("{0}", entityName);
  

  Why not just set pluralKey to entityName.

  pluralKey = entityName;
  

• pluralKey is set but never used, you only use pluralKeyword.

• I'm probably going to get punished for saying this, but I think you may have overdone some of the comments.

  eg.

  //Code starts from here.
  // Add key/value in dictionary.
  

  Personally I find this sort of commenting more distracting than helpful. :)

The following look wrong to me, but I'm not entirely sure since it should show up quite obviously during functional testing.

• Each iteration appends another copy of dummyText to entityName.

• If appending to entityName is intended, is it also intended to happen between its first and second use within the loop?

• Binary Worrier: Prefer StringWriter to StringBuilder - It uses a string builder under the covers, but your code is now writing to a TextWriter, far more natural, and is interchangeable with all other TextWriters.
• Brian Reichle: @Binary Worrier: either will work, if it will not be exposed publicly and I don't need the ability to swap in other TextWriters then I generally tend towards using StringBuilder for the more convenient append methods.
• Binary Worrier: Indeed they will. I find the TextWriter write methods more convenient. Each to his own I suppose :)
• Kashif: sparing time for my code review is so kind of you. I am really thankful. I am going to change my code as you suggested. One question, should I try to spend time on refactoring this method or it is ok? If so then any hint?
• Brian Reichle: @Muhammad: I find that cleaning up code like this tends to be an iterative process, as you take each step the next becomes clearer. I would find it hard to see where to go after these steps until I see the result. I suspect you would be able to extract methods to add plural and singular values to the dictionary, but the details are not clear yet.