|
the original code has a "bug". If maxValue is less than minValue
That may be true if the intent is to determine whether or not the test value "is between" the other two values; which is likely, but may not be the case.
As to improving the efficiency of the routine...
I've had bosses who would say, "It's always been that way, don't change it."
And colleagues who would say, "It ain't broke, don't fix it."
In one memorable case I bloody well did fix it, and the result was that the program took ten minutes to run rather than forty... and I was out of a job. Yes, I'd do it again.
|
|
|
|
|
At one time or another, we've all coded a horror. I know I have
Deja View - the feeling that you've seen this post before.
|
|
|
|
|
Thankfully, this won't even compile using the VS2005 C# compiler.
/ravi
|
|
|
|
|
Is that VB5 code? I was forced to write in VB6 once and the code you presented would not have compiled. I understand why this is a coding horror – who ever wrote it needs to find a new line of work.
P.S. I am still having trouble believing what I am seeing.
INTP
"Program testing can be used to show the presence of bugs, but never to show their absence."Edsger Dijkstra
|
|
|
|
|
took a long time to work out what that code actually does. I kept looking at it and thinking "No surely that's not it". Now i realise that that is the true horror of this creation. On top of that i can only guess the result of having testvalue > maxvalue but i guess as a private method it can expect sanitised input.
wow,
Russ
|
|
|
|
|
OMFG ! Did his manager review ANY of his work ?
Je vous salue du Québec !
|
|
|
|
|
Being new to WPF, I've been using the excellent Lutz Reflector[^] to browse through some APIs in .NET 3.0 (in particular, some WPF APIs) and found this goofy gem. Not so much a "horror" as it is a "what the heck?" Thought it worthy of posting here:
Obviously.png[^]
I'm sure there's some deep technical reason for this, probably related to virtual methods vs. pure abstract methods. Just thought the name was kind of funny, plus it ends up calling IsEmpty.
|
|
|
|
|
Have you ever emptied out a pail of water and stood the pail back up only to come back to the pail in 5 minutes and there is still water in it. It's empty, but it's not obviously empty.
Chris Meech
I am Canadian. [heard in a local bar]
|
|
|
|
|
I like the one below it. MayHaveCurves It sounds salacious.
Deja View - the feeling that you've seen this post before.
|
|
|
|
|
MayHaveCurves and ObviouslyEmpty .
Class Blonde ?
*ducks and runs*
"Throughout human history, we have been dependent on machines to survive. Fate, it seems, is not without a sense of irony. " - Morpheus
"Real men use mspaint for writing code and notepad for designing graphics." - Anna-Jayne Metcalfe
|
|
|
|
|
hmmm yes
Regards,
Sylvester G
sylvester_g_m@yahoo.com
|
|
|
|
|
Ha, excellent!
|
|
|
|
|
Pete O'Hanlon wrote: MayHaveCurves
"That's no moon, it's a space station." - Obi-wan Kenobi
|
|
|
|
|
Maybe it means, "is empty to the untrained eye, but we know it's really not"?
|
|
|
|
|
string currentdate = "Current date and time is " + DateTime.Now.ToString();
StringBuilder sbcurrentdate = new StringBuilder();
sbcurrentdate.Append(currentdate);
return sbcurrentdate;
Regards,
Sylvester G
sylvester_g_m@yahoo.com
|
|
|
|
|
Marvelous, a real beauty. Let me guess what comes after the method call... .ToString()?!?
|
|
|
|
|
sylvesterg wrote: return sbcurrentdate.ToString();
I assume, or does it really returns StringBuilder? Either way... heh.
"Throughout human history, we have been dependent on machines to survive. Fate, it seems, is not without a sense of irony. " - Morpheus
"Real men use mspaint for writing code and notepad for designing graphics." - Anna-Jayne Metcalfe
|
|
|
|
|
I don't get it. Oh well...
|
|
|
|
|
The whole point of using StringBuilder is that it's far more efficient than using "+" to "add" two strings together - the programmer was probably told to use StringBuilder for efficiency, and that was what they came up with
|
|
|
|
|
you caught the code nicely
Regards,
Sylvester G
sylvester_g_m@yahoo.com
|
|
|
|
|
Oh...
Well, that kind of makes a bit more sense now, but I still don't see what the big deal is. Other than the uselessness of StringBuilder, everything seems fine to me. Maybe if there were a lot more "+" concatenations before the StringBuilder usage, it would be a little funnier.
Also, I never trust an object to be better than another or than simple operations just because the documentation says so. There are a lot of ways you can concatenate two strings but the simplest and fastest way should always be the obvious way...
However, if you can prove that StringBuilder is better than simple string concatenation, you get an A+.
|
|
|
|
|
It should read like so
return new StringBuilder("Current date and time is " + DateTime.Now.ToString());
Lots of useless code to return a string as a StringBuilder. Or, shouldn't have used StringBuilder at all. The object of the StringBuilder class is to save memory when appending strings together. The original code misses that concept and uses more memory than if he would have just added the strings together.
Darroll
Not one person lives in the present.
|
|
|
|
|
sk8er_boy287 wrote: However, if you can prove that StringBuilder is better than simple string concatenation, you get an A+. If you need to build a string in a loop the stringbuilder is FAR more efficient then normal string concatenation. I've run into the problem before and learned the hard way. Actually I learned about it a while back using VB6, but VB6 didn't have a nice StringBuilder so I figured out another way around it. A simple example will show the difference. Do I get an A+?
Public Class Form1
Private Sub TestStringConcatenation(ByVal loops As Integer, ByVal myText As String)
Dim s As String = ""
For i As Integer = 0 To loops
s &= myText
Next
End Sub
Private Sub TestStringBuilder(ByVal loops As Integer, ByVal myText As String)
Dim sb As New System.Text.StringBuilder
For i As Integer = 0 To loops
sb.Append(myText)
Next
End Sub
Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click
Const LOOPS As Integer = 1000
Dim myText As String = New String("A"c, 200)
Dim sw As New Stopwatch
sw.Start()
TestStringConcatenation(LOOPS, myText)
sw.Stop()
Console.WriteLine("Normal string concatenation took {0} ticks", sw.ElapsedTicks)
sw.Reset()
sw.Start()
TestStringBuilder(LOOPS, myText)
sw.Stop()
Console.WriteLine("String builder took {0} ticks", sw.ElapsedTicks)
End Sub
End Class As for the code the OP posted, what's funny about it is how much extra useless code the person used to return a string. All that was needed was a simple line
Return "Current date and time is " + DateTime.Now.ToString() As someone already mentioned the original code writer probably learned that is was more efficient to use the stringbuilder if you are going to append text to a string. Unfortunatly they didn't use it correctly and didn't really grasp when it's appropriate to use it. I ran a test to see what method performed best. In this case normal string concatenation seemed to out perform stringbuilder. Here is my code I used to see the performance of each method.
Public Class Form1
Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click
Const TRIALS As Integer = 1000
Const LOOPS As Integer = 1000
Dim totalTicks As Long = 0
For i As Integer = 0 To TRIALS - 1
Dim sw As New Stopwatch
sw.Start()
For x As Integer = 0 To LOOPS - 1
Dim s As String = StringBuilderUsage1()
Next
sw.Stop()
totalTicks += sw.ElapsedTicks
Next
Console.WriteLine("Average ticks for StringBuilderUsage1 = " & (totalTicks / TRIALS).ToString)
totalTicks = 0
For i As Integer = 0 To TRIALS - 1
Dim sw As New Stopwatch
sw.Start()
For x As Integer = 0 To LOOPS - 1
Dim s As String = StringBuilderUsage2()
Next
sw.Stop()
totalTicks += sw.ElapsedTicks
Next
Console.WriteLine("Average ticks for StringBuilderUsage2 = " & (totalTicks / TRIALS).ToString)
totalTicks = 0
For i As Integer = 0 To TRIALS - 1
Dim sw As New Stopwatch
sw.Start()
For x As Integer = 0 To LOOPS - 1
Dim s As String = NormalConcatenation()
Next
sw.Stop()
totalTicks += sw.ElapsedTicks
Next
Console.WriteLine("Average ticks for normal concatenation = " & (totalTicks / TRIALS).ToString)
End Sub
Private Function StringBuilderUsage1() As String
Dim sb As New System.Text.StringBuilder
sb.Append("Current date and time is ")
sb.Append(Now.ToLongTimeString)
Return sb.ToString
End Function
Private Function StringBuilderUsage2() As String
Return New System.Text.StringBuilder("Current date and time is " & Now.ToLongTimeString).ToString
End Function
Private Function NormalConcatenation() As String
Return "Current date and time is " & Now.ToLongTimeString
End Function
End Class It's a rainy day here, so I had the time for all this nonsense
-- modified at 15:22 Wednesday 20th June, 2007
|
|
|
|
|
|
It's all done with mirrors.;)
|
|
|
|