2
\$\begingroup\$

As of now, I am using this code to open a file and read it into a list and parse that list into a string[]:

string CP4DataBase = "C:\\Program\\Line Balancer\\FUJI DB\\KTS\\KTS - CP4 - Part Data Base.txt";
CP4DataBaseRTB.LoadFile(CP4DataBase, RichTextBoxStreamType.PlainText);
string[] splitCP4DataBaseLines = CP4DataBaseRTB.Text.Split('\n');
List<string> tempCP4List = new List<string>();
string[] line1CP4Components;

foreach (var line in splitCP4DataBaseLines)
    tempCP4List.Add(line + Environment.NewLine);

string concattedUnitPart = "";
foreach (var line in tempCP4List)
{
    concattedUnitPart = concattedUnitPart + line;
    line1CP4PartLines++;
}

line1CP4Components = new Regex("\"UNIT\",\"PARTS\"", RegexOptions.Multiline)
                    .Split(concattedUnitPart)
                    .Where(c => !string.IsNullOrEmpty(c)).ToArray();

I am wondering if there is a quicker way to do this. This is just one of the files I am opening, so this is repeated a minimum of 5 times to open and properly load the lists.

The minimum file size being imported right now is 257 KB. The largest file is 1,803 KB. These files will only get larger as time goes on as they are being used to simulate a database and the user will continually add to them.

So my question is: is there a quicker way to do all of the above code?

\$\endgroup\$
1
  • \$\begingroup\$ What are you trying to accomplish? Perhaps you are trying to replace one type of newlines with another? Or perhaps you want to read a text file into some more useful format (like objects?). Please give some sample input text also. \$\endgroup\$
    – user9154
    Dec 14, 2011 at 14:31

4 Answers 4

5
\$\begingroup\$

The part of the code that makes it really slow is this:

string concattedUnitPart = "";
foreach (var line in tempCP4List)
{
  concattedUnitPart = concattedUnitPart + line;
  line1CP4PartLines++;
}

You should not concatenate large strings like that. The string gets longer and longer for each iteration, and it's copied into a new string each time.

If you Read a file that is 1.8 MB, that consists of lines which varies between 50 and 100 characters, you will have been copying about 10 000 MB of data before you have the result.

Also, it scales very badly, so when the files grow it will grow slower at an exponential rate. To handle a file that is 5 MB you will be copying about 80 000 MB of data.

James suggestion to do a replace seems to be a good option. If you want to split and join, you can use the String.Join method:

string[] splitCP4DataBaseLines = CP4DataBaseRTB.Text.Split('\n');
string concattedUnitPart = String.Join(Environment.NewLine, splitCP4DataBaseLines) + Environment.NewLine;
\$\endgroup\$
3
  • 5
    \$\begingroup\$ Another option is to use a StringBuilder to build up concattedUnitPart. \$\endgroup\$
    – Adam Lear
    Dec 14, 2011 at 6:08
  • \$\begingroup\$ Why would you split a string and the next line stick it all back together again? isn't that like my niece who cuts paper so she can sticky tape it together again? \$\endgroup\$ Dec 15, 2011 at 4:57
  • 1
    \$\begingroup\$ @JamesKhoury: I don't know, possibly if you want the option to do something to each line in between. I just wanted to show that there is a short and efficient way of doing that too. \$\endgroup\$
    – Guffa
    Dec 15, 2011 at 6:20
2
\$\begingroup\$

If i read this correctly you are reading in some text with newline characters, Splitting on new line characters, adding new line characters back in and then concatenation the lines back together?

why not instead:

CP4DataBaseRTB.Text.Replace('\n', Environment.NewLine);
\$\endgroup\$
2
\$\begingroup\$

There are a few points I would make on this - some with regards to performance and some simply regarding better coding practices.

First off, when hard coding a file path and when you don't require any escape characters, it is easier to simply declare the string with a prefixed '@'. This means you don't need to escape your backslash characters.

string CP4DataBase = @"C:\Program\Line Balancer\FUJI DB\KTS\KTS - CP4 - Part Data Base.txt";

Next, where you have constructed tempCP4List, you could have taken two alternate approaches. The first makes use of the capacity constructor parameter or the List<string>.Capacity property, which explicitly specifies the initial size of the List. This is useful as when building a list using Add(), if the contents of the list exceeds the capacity, a new array will be created within the List to accommodate the new element. When working with large collections of data, this could cause an issue with performance. So here, I would use:

List<string> tempCP4List = new List<string>(splitCP4DataBaseLines.Length);

- or -

List<string> tempCP4List = new List<string>();
tempCP4List.Capacity = splitCP4DataBaseLines.Length;

Alternately, you could use LINQ to build the list straight away, using:

List<string> tempCP4List = new List<string>(splitCP4DataBaseLines.Select(line => line + Environment.NewLine));

Another point, which has already been mentioned by Guffa is concatenating strings in the way you have with concattedUnitPart = concattedUnitPart + line; is a very memory-hungry process. In this case, both Guffa and Anna Lear have made good points with regards to using string.Join(...) or StringBuilder. If you run a small test program with large amounts of string data, you will see just how drastic a performance improvement this can deliver.

Finally, where you have performed your final string split, you have also used the LINQ extension method to check for empty strings. There is no need to do this, as it is an option that you can specify in the string.Split(...) method. So you would instead write the final line as:

line1CP4Components = new Regex("\"UNIT\",\"PARTS\"", RegexOptions.Multiline)
                      .Split(new string[1] { concattedUnitPart }, StringSplitOptions.RemoveEmptyEntries);

Notice the use of the StringSplitOptions.RemoveEmptyEntries option. This tells the string.Split(...) method to do exactly what your Where(...) clause does.

\$\endgroup\$
1
\$\begingroup\$

Here my 5 cents to all answers. When you use regular expression more than once it always helps to declare your regular expression as a field with Compiled option:

private static readonly Regex splitRegex = new Regex("\"UNIT\",\"PARTS\"", RegexOptions.Multiline | RegexOptions.IgnoreCase | RegexOptions.Compiled);
\$\endgroup\$

Your Answer

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

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