5
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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?

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Binary Worrier May 5 '11 at 12:41
  • \$\begingroup\$ @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. \$\endgroup\$ – Brian Reichle May 5 '11 at 12:46
  • \$\begingroup\$ Indeed they will. I find the TextWriter write methods more convenient. Each to his own I suppose :) \$\endgroup\$ – Binary Worrier May 5 '11 at 13:34
  • \$\begingroup\$ 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? \$\endgroup\$ – Kashif May 5 '11 at 14:30
  • \$\begingroup\$ @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. \$\endgroup\$ – Brian Reichle May 6 '11 at 2:50

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.