1

I'm searching the best way to create a string separated with another in a loop. I mean, for example, SQL reader:

StringBuilder sb = new StringBuilder();
while(reader.Read())
{
  sb.Append(reader[0]);
  sb.Append("<br />");
}
string result = sb.ToString();
result = result.Remove(result.LastIndexOf("<br />")); // <-

or creating SQL query string;

StringBuilder sb = new StringBuilder();
foreach(string v in values)
{
  sb.Append(v);
  sb.Append(",");
}
string query = sb.ToString()
query = query.Remove(query.LastIndexOf(",")); // <-
query = String.Concat("INSERT INTO [foo] ([bar]) VALUES(", query, ")");

This is the best I have found:

List<string> list = new List<string>;
while(reader.Read())
{
  list.Add(reader[0]);
}
string result = String.Join("<br />", list.ToArray());

Edit: I know about StringBuilder, I didn't used it here just for some clarity. My general idea do not use Remove / LastIndexOf !

abatishchev
  • 98,240
  • 88
  • 296
  • 433
  • He is looking for the best way to create a string by itterativly adding string components together. – Richard Slater Mar 17 '09 at 12:44
  • Ok, now your question has been edited, so the answers don't make much sense. :) But still it hurts my eye to see using StringBuilder and String.Concat in the same line.. That doesn't make much sense either. :) – vgru Mar 17 '09 at 13:18
  • Thanks for tip! I have edited – abatishchev Mar 17 '09 at 13:22

5 Answers5

8

I am not a fan of StringBuilder unless you really know that you need to worry about performance. It produces ugly code. I would write it this way...

private IEnumerable<string> ReadAllStrings(DataReader reader)
{
    while(reader.Read())
        yield return reader[0];
}


String.Join("<br />", ReadAllStrings(reader).ToArray());

If I were doing it a lot, I might consider an extension method:

public static class Extensions
{
    public static string JoinWith(this IEnumerable<string> strings, string separator)
    {
        return String.Join(separator, strings.ToArray());
    }
}

Then, my code would look like this:

ReadAllStrings(reader).JoinWith("<br />");
Brian Genisio
  • 47,787
  • 16
  • 124
  • 167
  • I agree, Join internally copies all strings to the same buffer, without new allocations. – vgru Mar 17 '09 at 12:55
  • So you're using the same String.Join(String, IEnumerable.ToArray()) too -- I think it's the best way, isn't it? – abatishchev Mar 17 '09 at 12:56
  • Yes, I think this is the best way. But in your example, you are creating TWO copies of the collection. One for List and one for ToArray(). In mine, you only create one array. – Brian Genisio Mar 17 '09 at 12:59
  • I prefer the extention method way. Added a couple of very similar extention methods to the code where I work. Makes things so much easier and readable... – Svish Mar 17 '09 at 13:07
  • Hmm.. I thought ToArray() just creates an array, which is passed to String.Join immediately. Do uou mean this array to be the second copy, yea? I don't know yet how to use 'yield' statement. I have to learn it, I'll do. – abatishchev Mar 17 '09 at 13:09
  • Yes, learn about the yield statement. It will change your world. You will find that only one collection is created when I call ToArray(). The ReadAllStrings() method does not create a collection, it creates an IEnumerable<>, which is VERY, VERY different. – Brian Genisio Mar 17 '09 at 13:12
  • It has to at least do an extra iteration to process the .ToArray() instruction. I don't know it ever creates an extra copy in memory, though, and the clarity of the code probably pays for it. – Joel Coehoorn Mar 17 '09 at 13:13
  • The one other thing I would do is add the column index as a parameter for ReadAllStrings, rather than hard code it into the method. – Joel Coehoorn Mar 17 '09 at 14:47
  • I don't create even one collection/array in my Join(IEnumerable) (see below). @Joel yes it does. – Anton Tykhyy Mar 17 '09 at 21:41
5

How's about:

StringBuilder builder;
while (reader.Read())
{
    if( builder == null )
    {
        builder = new StringBuilder(reader[0]);
    }
    else
    {
        builder.Append("<br />");
        builder.Append(reader[0]);
    }
}
string result = builder.ToString();
Rowland Shaw
  • 37,700
  • 14
  • 97
  • 166
2

This is just a combination of several of the better ideas shown:

public static class Extensions
{

    public static string JoinStrings(this DataReader reader, int ColumnIndex, string delimiter)
    {
        var result = new StringBuidler();
        var delim = String.Empty;
        while (reader.Read())
        {
           result.Append(delim).Append(reader[ColumnIndex].ToString());
           delim = delimiter;
        }
        return result.ToString();
    }
}

Now all you have to do is call it like this:

string result = reader.JoinStrings(0, "<br/>");
Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
1

Another .Net 2.0 solution - change the order:

reader.Read();
StringBuilder sb = new StringBuilder(reader[0]);
while(reader.Read())
{
  sb.Append("<br />");
  sb.Append(reader[0]);
}
string result = sb.ToString();
cjk
  • 45,739
  • 9
  • 81
  • 112
  • This will fail if the query returned no rows- but all it really needs is another if statement so it's not that bad. – Joel Coehoorn Mar 17 '09 at 13:50
  • indeed it would, but a lot of the solutions here would do the same. There is also no datatype conversion from what the reader is returning, and there is no point actually assigning the value of the StringReader to result, but it shows the point. – cjk Mar 17 '09 at 17:18
1
public class Separator 
{

    private string sep;
    private bool first = true;

    public Separator(string sep) 
    {
        this.sep = sep;
    }

    public virtual string ToString() 
    {
        string reply = first ? "" : sep;
        first = false;
        return reply;
    }
}

var sep = new Separator("<br/>");
var builder = new StringBuilder();
while (reader.Read())
{
    builder.Append (sep.ToString()) ;
    builder.Append (reader[0]) ;
}
Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
akuhn
  • 27,477
  • 2
  • 76
  • 91