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\$
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\$
  • 2
    \$\begingroup\$ You've missed the part where you add cN if resource is null. \$\endgroup\$ – Trevor Pilley Oct 16 '12 at 10:23
  • 1
    \$\begingroup\$ You can also prevent the list from having to resize by doing List<string>(columnN.Length); \$\endgroup\$ – Trevor Pilley Oct 16 '12 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\$ – Trevor Pilley Oct 16 '12 at 10:37
  • 5
    \$\begingroup\$ Now I wonder if I should start submitting my own code for review here :-D \$\endgroup\$ – Mihai Oct 16 '12 at 10:46
  • 7
    \$\begingroup\$ I think we all should :-) \$\endgroup\$ – Trevor Pilley Oct 16 '12 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\$
  • \$\begingroup\$ can you add comments to your code please :) \$\endgroup\$ – Muhammad Raja Oct 16 '12 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\$
  • \$\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 '12 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\$ – Trevor Pilley Oct 16 '12 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 '12 at 22:49

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.