c# foreach loop written poorly

Question

This foreach loop works fine while testing and only returning 5 rows of data, but I am well aware of how porly it is written , is there a better way, possibly using stringbuilder to re-write this more efficiently?

               SqlConnection conn = new SqlConnection(ConfigurationManager.ConnectionStrings["connstring"].ConnectionString);
    SqlCommand comm = new SqlCommand("SELECT Title, StartDate FROM tblEvents JOIN eo_UserEventWatch ON eo_UserEventWatch.EventID=tblEvents.ID WHERE eo_UserEventWatch.UserID = @GUID ;", conn);
    comm.Parameters.AddWithValue("GUID", userID);
    conn.Open();
    SqlDataAdapter da = new SqlDataAdapter(comm);
    DataTable dt = new DataTable();
    da.Fill(dt);
    string result ="{ \"event\" :[";
    foreach (DataRow dr in dt.Rows)
    {
        result += "{\"title\" : \"" + dr[0].ToString() + "\" , \"start\" : \"" + dr[1].ToString() +"\"} ,";
    }
    result = result.TrimEnd(',');
    result += "] }";
    return result;

Don't build `Json` using string operations. Use a json parser like Json.Net, JavaScriptSerializer, DataContractJsonSerializer etc... Otherwise, It is too easy to end up with an invalid json — L.B, Jun 23 '12 at 18:40
@TheGeekYouNeed have you measured it? The line is rewritten to **one** call to concatenate. It's unlikely to be particularly slow compared to upto four calls to append — Rune FS, Jun 23 '12 at 18:47
As @JesseC.Slicer said, you really need to dispose of the connection, command, and data adapter objects. Either wrap them in using blocks or else close them and wrap the entire block in a try/catch/finally the disposes of them in the finally block. — dodexahedron, Jun 23 '12 at 18:54
@RuneFS StringBuilder is way more memory efficient than string concatenation. — TheGeekYouNeed, Jun 23 '12 at 19:00
@TheGeekYouNeed but I wasn't talking about string concatenation but a call to string.Concat which the line in question is translated as and since that uses StringBuilder internally there's going to be no decernable difference in speed however the line in question is a lot easier to read than a series of calls to append — Rune FS, Jun 24 '12 at 19:55

Jon Skeet · Accepted Answer · 2012-06-23T19:17:35.107

5

Well yes, it would be more efficient to use StringBuilder:

StringBuilder builder = new StringBuilder("{ \"event\" :[");
foreach (DataRow row in dt.Rows)
{
    // Alternatively, use AppendFormat
    builder.Append("{\"title\" :\"");
           .Append(row[0])
           .Append("\", \"start\" : \"")
           .Append(row[1])
           .Append("\"} ,");
}
if (builder[builder.Length - 1] == ',')
{
    builder.Length -= 1;
}
builder.Append("] }");
string result = builder.ToString();

However, it's still not nice code - because you've got all that horrible literal JSON. I would suggest using Json.NET or another JSON library. You can probably use LINQ at that point, e.g.

var result = new { event = dt.AsEnumerable()
                             .Select(r => new { 
                                 title = r.Field<string>(0),
                                 start = r.Field<DateTime>(1))
                             .ToArray() };
// Or whatever, depending on the library you use
var json = JsonSerializer.ToJson(result);

Aside from anything else, now you don't need to worry about the format of the start value, or whether the title contains quotes etc.

(EDIT: As noted, you should absolutely have using statements for SQL connections etc. That's outside the main point of the question, which is why I didn't mention it here.)

edited Jun 23 '12 at 19:17

answered Jun 23 '12 at 18:41

Jon Skeet

1,421,763
867
9,128
9,194

(DataRow row in dt.Rows) -> .Append(dr[0]) , its cool though, you gotta be fast on SO , it worked , i'll accept when it lets me – Scott Selby Jun 23 '12 at 18:45
I wasn't going to upvote, but I can't *downvote* with the new bit about using a proper JSON library... – Jun 23 '12 at 18:47
1

also "if (builder.EndsWith(","))" - there is no .EndsWith in System.Text.StringBuilder ?? – Scott Selby Jun 23 '12 at 18:48
1

I never liked the trick where you retroactively delete the last separator from the string builder. Although it is correct it just seems not right. I like to encapsulate concatenation loops into an extension method. I understand that this is specialized demo code though. – usr Jun 23 '12 at 18:53
1

@ScottSelby And as a bonus, if I you were developing under me and wrote the code using Json.NET, chances are *much* higher that I wouldn't have you rewrite it ;-) – Jun 23 '12 at 18:55
I have used json.NET , I'm having trouble with it turning out right - that's a whole other issue though – Scott Selby Jun 23 '12 at 18:57
@usr: I don't typically like doing that, either. I was only trying to translate the original code as clearly as possible. – Jon Skeet Jun 23 '12 at 19:02

score 1 · Answer 2 · edited Jun 23 '12 at 18:58

1

Here is how you would use the StringBuilder

StringBuilder sb = new StringBuilder();
sb.Append("{ \"event\" :[");

foreach(DataRow dr in dt.Rows)
{
    sb.Append("{\"title\" : \"");
    sb.Append(dr[0].ToString());
    sb.Append("\" , \"start\" : \"");
    sb.Append(dr[1].ToString());
    sb.Append("\"} ,");
}

sb.Remove(sb.ToString().Length - 1, 1);
sb.Append("] }");

return sb.ToString();

To do this a better way completely, I would use something like JavaScriptSerializer (just a sample):

var stuff= (from DataRow dr in dt.AsEnumerable()
            select new {
                 DataItem1 = (string)dr[0];
                 DataItem2 = (string)dr[1];
            });

JavaScriptSerializer serializer = new JavaScriptSerializer();
return serializer.Serialize(stuff);

edited Jun 23 '12 at 18:58

answered Jun 23 '12 at 18:45

TheGeekYouNeed

7,509
2
26
43

2

What if there are no rows? Now we just have *more* invalid JSON. This is why string building sucks. – Jun 23 '12 at 18:50
Agreed pst -- I was just showing how to use the StringBuilder – TheGeekYouNeed Jun 23 '12 at 18:52
+1 For showing the use of JavaScriptSerializer ... although I would much rather use Json.NET (if available). – Jun 23 '12 at 18:57

score 0 · Answer 3 · edited May 23 '17 at 12:04

string.format uses StringBuilder internally.

Is String.Format as efficient as StringBuilder

public static string GetResult()
        {
            int userId = 0;
            string connectionString = ConfigurationManager.ConnectionStrings["connstring"].ConnectionString;
            string statement = "SELECT Title, StartDate FROM tblEvents JOIN eo_UserEventWatch ON eo_UserEventWatch.EventID=tblEvents.ID WHERE eo_UserEventWatch.UserID = @GUID";

            using (var con = new SqlConnection(connectionString))          
            using (var cmd = new SqlCommand(statement, con))
            {
                con.Open();
                cmd.Parameters.AddWithValue("GUID", userId);
                using (var dataAdapter = new SqlDataAdapter(cmd))
                {
                    DataTable dt = new DataTable();
                    dataAdapter.Fill(dt);
                    string result = "{ \"event\" :[";
                    foreach (DataRow dr in dt.Rows)
                    {
                        result += string.Format(@"{\title\ : \{0}\ , \start\ : \{1}\} ,", dr[0].ToString(), dr[1].ToString());
                    }
                    result = result.TrimEnd(',');
                    result += "] }";
                    return result;
                }
            }          
        }

So this adds `using` ... but doesn't address string building at all. And there is no note to give it context/justification. — , Jun 23 '12 at 18:59
You're still concatenating in a loop, which is the inefficient bit. See http://pobox.com/~skeet/csharp/stringbuilder.html — Jon Skeet, Jun 23 '12 at 19:16

c# foreach loop written poorly

3 Answers3