ALWAYS USE A STRINGBUILDER! [modified]

[PIEBALDconsult]

Everyone knows that if you are doing a lot of string concatenation in a loop, you absolutely must use a StringBuilder for performance! I found something like this today (the original is in VB.net). It formats some data for a fixed-width text file.

StringBuilder result = new StringBuilder() ;

foreach ( DataRow dr in datatable.Rows )
{
result.AppendLine ( "".PadRight ( ' ' , 9 ) + dr [ 0 ].ToString().PadRight ( ' ' , 9 ) + "".PadRight ( ' ' , 9 ) + dr [ 1 ].ToString().PadRight ( ' ' , 9 ) ... et cetera et cetera et cetera ... the statement is nearly 800 characters long ... ) ;
}

return ( result.ToString() ) ;

I suspect that each resultant line is also nearly 800 characters long. I'm also fairly sure that the data table doesn't usually contain many rows, so percentage-wise not many concatenations are eliminated here. As I'm new to the company, I chose the "it ain't broke, don't fix it" option. But I added comments suggesting that: 0) new string ( ' ' , 9 ) might perform better, is easier to read, and doesn't make the developer look stoooopid. 1) Proper use of a StringBuilder would definitely be a wise course of action. Now, I would also like to ask your opinion... This code builds one big-A string in memory and passes it back where it is simply written to a file. Personally, I would pass in a stream and write to it as I go, eliminating the memory hoggage. Another option would be to use events -- raise an event for each line (or field) and the calling code can write it to the file, but I don't really like that here. If you were writing a data to a fixed-width text file, what technique would you choose? ("Whatever the boss specifies" doesn't count.) P.S. Don't get me started on how they generate XML... :sigh:

modified on Tuesday, November 2, 2010 12:05 AM

[fjdiewornncalwe]

Agreed, but I usually like to see something like the following in these situations.

StringBuilder result = new StringBuilder() ;

foreach ( DataRow dr in datatable.Rows )
{
result.AppendLine( string.Format( "{0}{1}[2}",
new string( ' ', 9 ),
dr[0].ToString().PadRight( ' ', 9 ),
dr[1].ToString().PadRight( ' ', 9 ) );
}

return ( result.ToString() ) ;

However, if each data block is a fixed 9 character length, with a statement that is 800+ characters long, I would want to put some serious thought into an internal overridden StringBuilder that could simply handle each piece of data in a method call so that the redundant .ToString().PadRight... code could be cleaned up for readability. I would definitely fix this for a fixed-width text file to protect it.

I wasn't, now I am, then I won't be anymore.

[Samuel Cragg]

PogoboyKramer wrote:

.ToString().PadRight... code could be cleaned up for readability

Perhaps the following would work ;)

StringBuilder result = new StringBuilder();
foreach (DataRow dr in datatable.Rows)
{
result.AppendFormat("{0,9}{1,9}{2,9}", string.Empty, dr[0], dr[1]);
result.AppendLine();
}
return result.ToString();

Look at the Alignment Component section of this MSDN page

[fjdiewornncalwe]

Definitely an improvement.

I wasn't, now I am, then I won't be anymore.

[AspDotNetDev]

Or, you know, a for loop. Or maybe a function that accepts a data reader and an index. Or a data reader, an index, and a padding length (if it's not always 9). Or create an anonymous delegate to capture the data reader to avoid passing the same parameter over and over, then each function call would only require an index and a padding length. Or a function that takes an array of indexes ({0, 1, 2}) and a padding length. Or create a data type that allows indexes and padding lengths to be paired together, then functions applied to that data in one call. Or since they're all pairs and they're all the same data type (int), use a 2D array with an inline initializer rather than a custom data type.

[Forum Guidelines]

[AspDotNetDev]

Depends on the variability of the format and data, but I might go with something like this:

StringBuilder result = new StringBuilder();
int fieldCount = 40;
foreach (DataRow dr in datatable.Rows)
{
for(int i = 0; i < fieldCount; i++)
{
result.Append(" ");
result.Append(dr[i].ToString().PadRight(' ', 9 );
}
result.AppendLine();
}
return result.ToString();

If the data/format is more variable, I might come up with a more interesting solution. Also, I might change some minor things, depending on context. As a simple example, I might make the number of spaces (in the example shown, nine) a parameter in a function or a class-level constant if the scenario warrants it.

[Forum Guidelines]

[PIEBALDconsult]

No, it's not always nine, and it's worse than what I presented.

[Robert Rohde]

PIEBALDconsult wrote:

If you were writing a data to a fixed-width text file, what technique would you choose?

I would definitely pass a TextWriter into that function. This way the caller could pass either a StringWriter (which uses a StringBuilder internally) or a StreamWriter depending on the target. Together with some helper functions it would look like this:

public string GetAsText()
{
using (var writer = new StringWriter())
{
WriteTo(writer);
return writer.ToString();
}
}

public void WriteToFile(string path)
{
using (var writer = File.CreateText(path))
{
WriteTo(writer);
}
}

public void WriteTo(TextWriter writer)
{
foreach ( DataRow dr in datatable.Rows )
{
writer.WriteLine(...);
}
}

[PIEBALDconsult]

What I did, just for the sake of my own sanity, was write a FixedFileFormatter class. It can be used like so:

        using
        (       
            FixedFileFormatter f 
        = 
            new FixedFileFormatter
            (
                System.Console.Out
            ,
                new FixedFileField ( 10 , Alignment.Left )
            ,
                new FixedFileField ( 15 , Alignment.Right )
            ,
                new FixedFileField ( 15 , Alignment.Center )
                { 
                    Format="X8"
                }
            ,
                new FixedFileField ( 10 , Alignment.Left )
            ) 
        )
        {
            f.WriteLine ( new object\[\] { "Hello, World!" ,  "Hello, World!" ,  255 , 42} ) ;
        }

[richard_k]

Careful there, you are introducing seriously useful O-O concepts to a piece of code that was written by someone channeling a 'way-back' machine (a la Bullwinkle). And I totally agree.

[Member 96]

PIEBALDconsult wrote:

As I'm new to the company

PIEBALDconsult wrote:

and doesn't make the developer look stoooopid.

No matter how right you are, you need to consider the social implications of what you say.

---

“If you want to build a ship, don't drum up people together to collect wood and don't assign them tasks and work, but rather teach them to long for the endless immensity of the sea” - Antoine de Saint-Exupery

[PIEBALDconsult]

Which is why I said it here. :-D

[Member 96]

Oh, sorry, I thought you said you made a comment in the code to that effect. Something that would surely have been noticed eventually.

---

“If you want to build a ship, don't drum up people together to collect wood and don't assign them tasks and work, but rather teach them to long for the endless immensity of the sea” - Antoine de Saint-Exupery

[PIEBALDconsult]

The comment I added was much much tamer.