Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following C# method to split a path string. The only thing I know is a fixed string in the path and I have to split the full path into two parts, first part should be one level below the fixed string and the rest should be the second part.

For example, if I have the following path:

 string mainText = @"C:\Abc\Fixed\MyTemp\Japan\Tokyo";

then my firstPart would be "C:\Abc\Fixed\MyTemp" and second part "Japan\Tokyo"

I want to improve this method in terms of memory and speed.

private static void SplitPath(string mainText, out string firstPart, out string secondPart)
        {
            firstPart = string.Empty;
            secondPart = string.Empty;
            if (!string.IsNullOrEmpty(mainText))
            {
                string strConstatnt = "Fixed";
                List<string> splitted = mainText.Split(new char[] { '\\' }).ToList();
                int indexToFixed = splitted.IndexOf(strConstatnt);
                StringBuilder sbFirst = new StringBuilder();
                StringBuilder sbSecond = new StringBuilder();
                if (indexToFixed >= 0)
                {
                    for (int i = 0; i < splitted.Count; i++)
                    {
                        if (i < (indexToFixed + 2))
                        {
                            sbFirst.Append(splitted[i] + "\\");
                        }
                        else
                        {
                            break;
                        }
                    }

                    for (int i = (indexToFixed + 2); i < splitted.Count; i++)
                    {
                        sbSecond.Append(splitted[i] + "\\");
                    }
                }
                if (sbFirst.Length > 0)
                {
                    firstPart = sbFirst.Remove(sbFirst.Length - 1, 1).ToString();
                }
                if (sbSecond.Length > 0)
                {
                    secondPart = sbSecond.Remove(sbSecond.Length - 1, 1).ToString();
                }
            }
        }
share|improve this question
2  
Is there a reason you're focussed on performance? Without knowing the wider context of how this is being used, I'd guess it's not something performance-critical. –  Ben Aaronson Aug 31 at 10:21

3 Answers 3

I took the liberty to make the exercise as well to see how our approaches differ. I don't think there are many remarks in terms of performance although StringBuilder.Remove() is a red flag in that area.

All together I think my code provides a more easily readable solution which should help in terms of human-reading efficiency. In the end I pretty much abstracted the things you do under library features.

public void SplitPath(string mainText, out string firstPart, out string secondPart)
{
  const string pivot = "Fixed";
  const char delimiter = '\\';

  var entries = mainText.Split(delimiter);
  int pivotIndex = entries.Length;
  for (var i = 0; i < entries.Length; i++)
  {
      if (entries[i] == pivot)
      {
          // Take index + 2 because linq.Take/Skip is 1-based
          // Optionally take index + 1 to avoid out-of-bounds when the pivot is the last element
          pivotIndex = (i == entries.Length) ? i + 1 : i + 2;
          break;
      }
  }

  firstPart = string.Join(delimiter.ToString(), entries.Take(pivotIndex));
  secondPart = string.Join(delimiter.ToString(), entries.Skip(pivotIndex));
}

I have tested this code with inputs C:\Abc\Fixed\MyTemp\Japan\Tokyo, C:\Abc\Fixed and C:\Abc.

If you'd want to take your approach after all, a few remarks:

  • Try to early return by using a negated if-statement condition.
  • Put your delimiter in a variable
  • Avoid magic values
  • Avoid removing data from the StringBuilder
share|improve this answer

Using String.Substring() makes this pretty easy:

    public static void SplitPath(string mainText, out string firstPart, out string secondPart)
    {
        firstPart = mainText.Substring(0, mainText.LastIndexOf('\\'));
        firstPart = firstPart.Substring(0, firstPart.LastIndexOf('\\'));

        secondPart = mainText.Substring(firstPart.Length + 1);
    }

Input:

string mainText = @"C:\Abc\Fixed\MyTemp\Japan\Tokyo";

Output:

  firstPart = "C:\Abc\Fixed\MyTemp"   
  secondPart= "Japan\Tokyo"
share|improve this answer
    
Instead of assigning firstPart twice I'd use a temporary variable instead. Also, if the input string does not have any backslashes at all LastIndexOf will return -1 and as a consequence Substring will throw an ArgumentOutOfRangeException. My C# is rusty, but I think OP's version just doesn't assign anything to the two output variables in this case. –  DarkDust Aug 31 at 15:49
    
@DarkDust Temporary variable? The goal is make it more efficient. Reusing variables work towards that goal. If anything, maybe just string the two calls together firstPart = mainText.Substring(0, mainText.LastIndexOf('\\')).Substring(0, mainText.LastIndexOf('\\')). No duplicate assignment. No temporary variable creations. –  WernerCD Aug 31 at 16:53
    
Additionally... where does this answer check for the location of fixed? Just because the example is primary four deep and secondary the next two... doesn't mean the real world example will be. What if it's C:\Abc\Xyz\Fixed\Foo\Bar\Baz? or C:\Abc\Fixed\Foo? –  WernerCD Aug 31 at 16:59
    
@WernerCD: Introducing a temporary variable will not change the number of objects allocated, it's just one additional reference which quickly goes out of scope. I personally think it's more important to be able to read/maintain code easily than trying than trying to do premature optimizations. And your contraction is wrong: your second call to Substring will not modify the string returned from the first Substring any more as your cutting it to the exact same length again. But then again, you're absolutely right about the missing "Fixed" so the whole code is wrong anyway. –  DarkDust Aug 31 at 17:22

You can use regular expressions:

static void Main(string[] args)
{
    var input = @"C:\Abc\Fixed\MyTemp\Japan\Tokyo";
    foreach (var item in Regex.Split(input, @"(?<=Fixed\\(?=[^\\]+))"))
        Console.WriteLine(item);
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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