What do you think of this code?

  • Thread starter Thread starter Michael Culley
  • Start date Start date
M

Michael Culley

I found this code in a project:

StringBuilder sb = new StringBuilder();
sb.Append("SELECT * FROM SomeTable")
.Append("WHERE SomeField = SomeValue")
.Append("ORDER BY etc etc etc")

at first I thought it was using a using statement but it's not. It works
because sb.Append returns an instance of itself. I'd be interested to hear
what everyone thinks of it.
 
Michael said:
I found this code in a project:

StringBuilder sb = new StringBuilder();
sb.Append("SELECT * FROM SomeTable")
.Append("WHERE SomeField = SomeValue")
.Append("ORDER BY etc etc etc")

at first I thought it was using a using statement but it's not. It works
because sb.Append returns an instance of itself. I'd be interested to hear
what everyone thinks of it.
Click to expand...

I see nothing particularly wrong with it. If you look at the examples
for the various overloads of .Append() in MSDN you will see that same
style syntax used there.
 
My honest opinion as a long-time developer? It's cute, but unnecessarily so.

Why go to the trouble of using this unintuitive construct, which requires
that EVERYONE that sees it go the documentation to remember or discover that
sb.Append() happens to return an instance of itself? Life's full enough of
unnecessary noise, without our introducing it ourselves just to show how
clever we are.

How much more difficult would it be for a competent typist to put the 'sb.'
at the beginning of each of the lines that follow the first one and make it
perfectly clear to the reader what's going on? Remember: the guy who wrote
it is not the one who's going to have to maintain it.

Oh, and by the way, as I'm sure you noticed after posting the message, there
would be some crucial missing spaces in the SQL statement that the example
you supplied would generate.

Regards,
Tom Dacon
Dacon Software Consulting
 
Tom Dacon said:
My honest opinion as a long-time developer? It's cute, but unnecessarily so.

Why go to the trouble of using this unintuitive construct, which requires
that EVERYONE that sees it go the documentation to remember or discover that
sb.Append() happens to return an instance of itself? Life's full enough of
unnecessary noise, without our introducing it ourselves just to show how
clever we are.

How much more difficult would it be for a competent typist to put the 'sb.'
at the beginning of each of the lines that follow the first one and make it
perfectly clear to the reader what's going on? Remember: the guy who wrote
it is not the one who's going to have to maintain it.
Click to expand...

My sentiments exactly :-)
Oh, and by the way, as I'm sure you noticed after posting the message, there
would be some crucial missing spaces in the SQL statement that the example
you supplied would generate.
Click to expand...

It was just an example.
 
Michael Culley said:
I found this code in a project:

StringBuilder sb = new StringBuilder();
sb.Append("SELECT * FROM SomeTable")
.Append("WHERE SomeField = SomeValue")
.Append("ORDER BY etc etc etc")

at first I thought it was using a using statement but it's not. It works
because sb.Append returns an instance of itself. I'd be interested to hear
what everyone thinks of it.
Click to expand...

Assuming the next line uses "ToString", the developer would have been
better writing:

string x = "SELECT * FROM SomeTable "+
"WHERE SomeField=SomeValue "+
"ORDER BY etc etc etc";

That would actually be much faster than using StringBuilder, as:

1) If the value is actually a constant, it will be very fast indeed.
Concatenated string constants are concatenated by the C# compiler,
not at runtime.
2) If it's not, it ends up as one call to String.Concat, which is able
to allocate the right amount of space to start with - the
StringBuilder version will involve at least one reallocation.
 
Hi,

It's ok , the Append method return the same instance so you are not
creating a new object

Now I assume that you replace the parameter in the Append() method, if not
it has an overhead, as you don't need to use StringBuilder for that. it
would be simpler to just do:

string s= "SELECT * FROM SomeTable WHERE SomeField = SomeValue .....";


Cheers,
 
[concatenation] would actually be much faster than using StringBuilder,
as:

2) ... it ends up as one call to String.Concat, which is able
to allocate the right amount of space to start with - the
StringBuilder version will involve at least one reallocation.
Click to expand...

Although, if the StringBuilder had been constructed with a buffer larger
than the default initial size of 16 bytes in the first place, it would also
avoid allocation. I mention this because passing an initial size and/or
string to the StringBuilder ctor is perhaps one of the most often-overlooked
optimizations, at least in the code I've seen. Quite often you know about
how big the final result will be, or you at least have an idea of the
average, and it's almost always way more than 16 bytes. By default,
StringBuilder will start with 16, then jump to 32, 64, 128, etc. In the
OP's contrived example, a starting size of perhaps 512, depending on the
expected length of the SQL statement, would probably be better and would
usually keep any new allocations to zero or one rather than perhaps a half
dozen.

--Bob
 
Bob Grommes said:
[concatenation] would actually be much faster than using StringBuilder,
as:

2) ... it ends up as one call to String.Concat, which is able
to allocate the right amount of space to start with - the
StringBuilder version will involve at least one reallocation.
Click to expand...

Although, if the StringBuilder had been constructed with a buffer larger
than the default initial size of 16 bytes in the first place, it would also
avoid allocation. I mention this because passing an initial size and/or
string to the StringBuilder ctor is perhaps one of the most often-overlooked
optimizations, at least in the code I've seen. Quite often you know about
how big the final result will be, or you at least have an idea of the
average, and it's almost always way more than 16 bytes. By default,
StringBuilder will start with 16, then jump to 32, 64, 128, etc. In the
OP's contrived example, a starting size of perhaps 512, depending on the
expected length of the SQL statement, would probably be better and would
usually keep any new allocations to zero or one rather than perhaps a half
dozen.
Click to expand...

Absolutely. Still less efficient than doing it in a single set of
string concatenations (with no temporary strings involved), but better
than it currently is :)
 
Ignacio Machin ( .NET/ C# MVP ) said:
string s= "SELECT * FROM SomeTable WHERE SomeField = SomeValue .....";
Click to expand...

But the actual sql was much too long for one line.
 
Jon Skeet said:
Absolutely. Still less efficient than doing it in a single set of
string concatenations (with no temporary strings involved), but better
than it currently is :)
Click to expand...

I don't think the efficiency is the issue in the case, generally the string
is built once, a call is made across the network and a loop is executed
retrieving records. If it was updated from the least efficient to the most
efficient method of building the string I doubt any user would notice the
speed difference even if they were looking for it. Readability is what is
important. I probably do it in the least efficient method by using sql+= on
each line to build the string.
 
Michael Culley said:
I don't think the efficiency is the issue in the case, generally the string
is built once, a call is made across the network and a loop is executed
retrieving records. If it was updated from the least efficient to the most
efficient method of building the string I doubt any user would notice the
speed difference even if they were looking for it. Readability is what is
important. I probably do it in the least efficient method by using sql+= on
each line to build the string.
Click to expand...

Why not just use a verbatim string literal then?

sql = @"
SELECT * FROM FOO
WHERE BAR
";

etc? Efficient, readable, very easy to edit... Wins all round as far as
I can see :)
 
Thanks Jon, I didn't know you could do that. However, I'm not a big fan
because it kills your indenting, AFAIK you need to write it like this:

[STAThread]
static void Main()
{
string sql = @"SELECT *
FROM SomeTable
WHERE ID = 1";
MessageBox.Show(sql);
}
 
Michael Culley said:
Thanks Jon, I didn't know you could do that. However, I'm not a big fan
because it kills your indenting, AFAIK you need to write it like this:

[STAThread]
static void Main()
{
string sql = @"SELECT *
FROM SomeTable
WHERE ID = 1";
MessageBox.Show(sql);
}
Click to expand...

No, you can put the indenting in yourself. Of course, the whitespace
will still be in the SQL, but that shouldn't make any real difference,
should it? I'd rather have it indented nicely in the code than in the
server :)

Here's another alternative:

string sql = "SELECT * "+
"FROM SomeTable "+
"WHERE ID = 1";
 
Hi,

What you mean with that?

If you mean that you dont like to have a very long line in the editor you
have not to worry about it

These two lines should compile to the same:

string s = "1" +
"2" +
"3" +
"4";
string s = "1234";

The compiler should be smart enough to create one string only, I haven't
test it though.

Cheers,
 
<"Ignacio Machin \( .NET/ C# MVP \)" <ignacio.machin AT
dot.state.fl.us> said:
What you mean with that?

If you mean that you dont like to have a very long line in the editor you
have not to worry about it

These two lines should compile to the same:

string s = "1" +
"2" +
"3" +
"4";
string s = "1234";

The compiler should be smart enough to create one string only, I haven't
test it though.
Click to expand...

Indeed, it will - and it's required to by the spec.
 
Jon Skeet said:
No, you can put the indenting in yourself. Of course, the whitespace
will still be in the SQL, but that shouldn't make any real difference,
should it? I'd rather have it indented nicely in the code than in the
server :)
Click to expand...

It can be a pain for debugging tho.
string sql = "SELECT * "+
"FROM SomeTable "+
"WHERE ID = 1";
Click to expand...

This is probably the best solution (in my opinion of course :-)
 
Back
Top