patterncsharpMinor
Return Dictionary object having key/value pair of keywords and URLs
Viewed 0 times
returnhavingvaluekeywordsanddictionaryobjectpairkeyurls
Problem
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 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 keyWordsWithURLs = new Dictionary();
// 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("{2}", singularKeyword, entityName, urlText);
// Add key/value in dictionaly.
keyWordsWit
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 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 keyWordsWithURLs = new Dictionary();
// 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("{2}", singularKeyword, entityName, urlText);
// Add key/value in dictionaly.
keyWordsWit
Solution
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)
-
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?
-
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.
Why not just set pluralKey to 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.
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?
-
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?
Code Snippets
string singularKeyword = keyWord.Keyword.singularKeyword.Replace(' ', '-');
string singularKey = string.Format("{0} {1}", keyWord.Keyword.ToString(), entityName);singularKey = string.Format("{0} in {1}", keyWord.Keyword.ToString(), entityName);pluralKey = string.Format("{0}", entityName);pluralKey = entityName;//Code starts from here.
// Add key/value in dictionary.Context
StackExchange Code Review Q#2246, answer score: 3
Revisions (0)
No revisions yet.