7
\$\begingroup\$

Its working but I don't think its the best way of doing it. I also changed variable names just to let you know,

       string [] columnN = AnotherString.Split(',');  //Another String is something like this = "HEhehehehehE heeh, aSKdjhkaaksjd, asldkhja slkdlk, asdajsdlka, asdljkasd, asdkasjdasd, asdasdasdl, askdjasd"
                AnotherString = "";
                int i = 0;

            foreach (string cN in columnN)
            {
                if (!string.IsNullOrEmpty(Res.ResourceManager.GetString(cN.ToLower().Trim())))
                    AnotherEmptyString += Res.ResourceManager.GetString(cN.ToLower().Trim());
                else
                    AnotherString += cN;
                i++;

                if (i < columnN.Length)
                    AnotherString += ",";
            }

I saved all resources in lower case, also if in case cN doesn't got any resource won't it will give an exception.

\$\endgroup\$

3 Answers 3

10
\$\begingroup\$

Your code should work ok. However, I would have used a list to store the intermediate results, and then string.Join to create the result. (Also perhaps Linq to filter the data, but then someone not familiar with Linq will have trouble maintaining the code.) Something like

string[] columnN = AnotherString.Split(',');
string[] stringList = new string[columnN.Length];

for (int i = 0; i < columnN.Length; i++)
{
    string cN = columnN[i];
    string resource = Res.ResourceManager.GetString(cN.ToLower().Trim()));
    stringList[i] = resource ?? cN;
}

AnotherString = string.Join(",", stringList);
\$\endgroup\$
5
  • 2
    \$\begingroup\$ You've missed the part where you add cN if resource is null. \$\endgroup\$ Oct 16, 2012 at 10:23
  • 1
    \$\begingroup\$ You can also prevent the list from having to resize by doing List<string>(columnN.Length); \$\endgroup\$ Oct 16, 2012 at 10:23
  • 2
    \$\begingroup\$ You can also update this stringList[i] = (resource != null) ? resource : cN; to use the null coalescing operator stringList[i] = resource ?? cN; \$\endgroup\$ Oct 16, 2012 at 10:37
  • 5
    \$\begingroup\$ Now I wonder if I should start submitting my own code for review here :-D \$\endgroup\$
    – Mihai
    Oct 16, 2012 at 10:46
  • 7
    \$\begingroup\$ I think we all should :-) \$\endgroup\$ Oct 16, 2012 at 10:56
10
\$\begingroup\$

I think using LINQ makes perfect sense here:

var strings = AnotherString.Split(',')
    .Select(s => Res.ResourceManager.GetString(s.ToLower().Trim())) ?? s);
AnotherString = string.Join(",", strings);
\$\endgroup\$
1
  • \$\begingroup\$ can you add comments to your code please :) \$\endgroup\$ Oct 16, 2012 at 22:18
7
\$\begingroup\$

Ok a few things:

  1. Do this once and cache the result Res.ResourceManager.GetString(cN.ToLower().Trim()), the call .ToLower() creates a new string and then the .Trim() call creates yet another so you save the creation of 2 extra strings for each cN.
  2. Use a string builder instead of concatenation - every time you do += on a string, a new string is created in memory.

That gives you something like this:

string[] columnN = AnotherString.Split(',');  //Another String is something like this = "HEhehehehehE heeh, aSKdjhkaaksjd, asldkhja slkdlk, asdajsdlka, asdljkasd, asdkasjdasd, asdasdasdl, askdjasd"

var stringBuilder = new StringBuilder();

int i = 0;

foreach (string cN in columnN)
{
    var resourceText = Res.ResourceManager.GetString(cN.ToLower().Trim());

    if (!string.IsNullOrEmpty(resourceText))
    {
        stringBuilder.Append(resourceText);
    }
    else
    {
        stringBuilder.Append(cN);
    }

    i++;

    if (i < columnN.Length)
    {
        stringBuilder.Append(",");
    }
}

AnotherString = stringBuilder.ToString()
\$\endgroup\$
3
  • \$\begingroup\$ A personal preference but I would probably reverse your if statement to remove the need for the !. Just for me makes it easier to read although (i.e. don't need to think about the not) but I know there are some arguments for this way. \$\endgroup\$
    – dreza
    Oct 16, 2012 at 20:00
  • 1
    \$\begingroup\$ @dreza that's a fair comment, I wouldn't actually write it this way myself either however I've noticed that sometimes people answer with a better implementation but without explaining why it's an improvement. My intention was to point out what the issues with the original code were rather than to re-write the method so that the OP could actually learn what the problems were rather than just blindly copying a smaller implementation. \$\endgroup\$ Oct 16, 2012 at 20:14
  • \$\begingroup\$ Fair enough. I definitely agree that copying code tends to not lead to actual learning and potentially makes life more difficult int he long . \$\endgroup\$
    – dreza
    Oct 16, 2012 at 22:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.