Bad Programming??...
-
So, I've done mainly VBc and C/C++ for embedded device, and a tiny bit of VC++. When .Net came along, after a while, it was often repeated that .Net allowed people to write bad code, but will still work. I was a stickler for following good programming form. On one occasion, someone had ask a question about what the .Net compiler will do with [whatever code it was], and someone else chimed in saying something else like 'the compiler is smart and will optimize...'. But, you know, I'm still a stickler for proper form, allocating vars and objects, properly destroying them, stopping any timers before program exit, etc. I guess, proper "cleanup" you could say. So I'm working with some code the company hired a supposedly experienced programmer to write a program for the company that monitors devices it makes. Some of this is so appalling, that it looks like an absolute beginner would write. So the below chunk of code is one function he had written. He has this running in it's own thread, and it's a UPD listening socket. This is used to prevent multiple instances of it running, and to return from the system tray after it's been minimized to the tray. So this function is always running in it's own thread...
Private Sub udpThreadsub()
While True
Try
udpcl = New UdpClient(UDPPORT)
Dim RemoteIpEndPoint As New IPEndPoint(IPAddress.Any, 0)
While True
Try
Dim rec = udpcl.Receive(RemoteIpEndPoint)
If rec.Length = 11 Then
Dim match = True
For i = 0 To 10
If rec(i) <> UDP_MSG(i) Then
match = False
Exit For
End If
Next
If match Then
Outoftray()
End If
End If
Catch e As Exception
End Try
End While
Catch e As Exception
End Try
End While
End SubTo me, this is some of the ugliest code I've ever seen. I mean, the compiler isn't THAT smart is it? To me, this looks like the dim'd object will be created and sent to GC every iteration through the loop! But then again, I don't have an intimate knowledge of what the compiler will do
-
So, I've done mainly VBc and C/C++ for embedded device, and a tiny bit of VC++. When .Net came along, after a while, it was often repeated that .Net allowed people to write bad code, but will still work. I was a stickler for following good programming form. On one occasion, someone had ask a question about what the .Net compiler will do with [whatever code it was], and someone else chimed in saying something else like 'the compiler is smart and will optimize...'. But, you know, I'm still a stickler for proper form, allocating vars and objects, properly destroying them, stopping any timers before program exit, etc. I guess, proper "cleanup" you could say. So I'm working with some code the company hired a supposedly experienced programmer to write a program for the company that monitors devices it makes. Some of this is so appalling, that it looks like an absolute beginner would write. So the below chunk of code is one function he had written. He has this running in it's own thread, and it's a UPD listening socket. This is used to prevent multiple instances of it running, and to return from the system tray after it's been minimized to the tray. So this function is always running in it's own thread...
Private Sub udpThreadsub()
While True
Try
udpcl = New UdpClient(UDPPORT)
Dim RemoteIpEndPoint As New IPEndPoint(IPAddress.Any, 0)
While True
Try
Dim rec = udpcl.Receive(RemoteIpEndPoint)
If rec.Length = 11 Then
Dim match = True
For i = 0 To 10
If rec(i) <> UDP_MSG(i) Then
match = False
Exit For
End If
Next
If match Then
Outoftray()
End If
End If
Catch e As Exception
End Try
End While
Catch e As Exception
End Try
End While
End SubTo me, this is some of the ugliest code I've ever seen. I mean, the compiler isn't THAT smart is it? To me, this looks like the dim'd object will be created and sent to GC every iteration through the loop! But then again, I don't have an intimate knowledge of what the compiler will do
Ignoring the fact that this is VB.NET ... X| The outer loop only runs if an exception is thrown (and swallowed) outside of the inner loop's exception-swallowing
Try..Catchblock. Realistically, the only things that could trigger that would be the creation of theUdpClient, or the creation of theIPEndPoint. But that would mean that either the port was inaccessible, or the application had run out of memory. In which case, the next iteration would almost certainly throw the same exception again, leading to a thread constantly executing a tight try-throw-catch-try-again loop. The receive call will allocate a new byte array. But there's no option to avoid that, and no way to make it reuse the same array. The only way that could be optimized would be if the BCL code did it for you. But that's unlikely, since it has no way of knowing what the caller will do with the returned array. So you'll have some GC churn on the inner loop. If this is intended to implement a single instance application, there are far better ways to do that. For a start, in a VB.NET WinForms project, there's a specific option on the application properties to do this.
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
-
So, I've done mainly VBc and C/C++ for embedded device, and a tiny bit of VC++. When .Net came along, after a while, it was often repeated that .Net allowed people to write bad code, but will still work. I was a stickler for following good programming form. On one occasion, someone had ask a question about what the .Net compiler will do with [whatever code it was], and someone else chimed in saying something else like 'the compiler is smart and will optimize...'. But, you know, I'm still a stickler for proper form, allocating vars and objects, properly destroying them, stopping any timers before program exit, etc. I guess, proper "cleanup" you could say. So I'm working with some code the company hired a supposedly experienced programmer to write a program for the company that monitors devices it makes. Some of this is so appalling, that it looks like an absolute beginner would write. So the below chunk of code is one function he had written. He has this running in it's own thread, and it's a UPD listening socket. This is used to prevent multiple instances of it running, and to return from the system tray after it's been minimized to the tray. So this function is always running in it's own thread...
Private Sub udpThreadsub()
While True
Try
udpcl = New UdpClient(UDPPORT)
Dim RemoteIpEndPoint As New IPEndPoint(IPAddress.Any, 0)
While True
Try
Dim rec = udpcl.Receive(RemoteIpEndPoint)
If rec.Length = 11 Then
Dim match = True
For i = 0 To 10
If rec(i) <> UDP_MSG(i) Then
match = False
Exit For
End If
Next
If match Then
Outoftray()
End If
End If
Catch e As Exception
End Try
End While
Catch e As Exception
End Try
End While
End SubTo me, this is some of the ugliest code I've ever seen. I mean, the compiler isn't THAT smart is it? To me, this looks like the dim'd object will be created and sent to GC every iteration through the loop! But then again, I don't have an intimate knowledge of what the compiler will do
Without a (stopwatch) timing, the "GC collecting" theory is meaningless. I'm amazed how fast C# can run "before" I've bothered with my optimizations; where I routinely create objects in called methods that run at many frames per second. I had the same concerns and found I had cycles to spare. The GC "graph" just chugs along.
"Before entering on an understanding, I have meditated for a long time, and have foreseen what might happen. It is not genius which reveals to me suddenly, secretly, what I have to say or to do in a circumstance unexpected by other people; it is reflection, it is meditation." - Napoleon I
-
So, I've done mainly VBc and C/C++ for embedded device, and a tiny bit of VC++. When .Net came along, after a while, it was often repeated that .Net allowed people to write bad code, but will still work. I was a stickler for following good programming form. On one occasion, someone had ask a question about what the .Net compiler will do with [whatever code it was], and someone else chimed in saying something else like 'the compiler is smart and will optimize...'. But, you know, I'm still a stickler for proper form, allocating vars and objects, properly destroying them, stopping any timers before program exit, etc. I guess, proper "cleanup" you could say. So I'm working with some code the company hired a supposedly experienced programmer to write a program for the company that monitors devices it makes. Some of this is so appalling, that it looks like an absolute beginner would write. So the below chunk of code is one function he had written. He has this running in it's own thread, and it's a UPD listening socket. This is used to prevent multiple instances of it running, and to return from the system tray after it's been minimized to the tray. So this function is always running in it's own thread...
Private Sub udpThreadsub()
While True
Try
udpcl = New UdpClient(UDPPORT)
Dim RemoteIpEndPoint As New IPEndPoint(IPAddress.Any, 0)
While True
Try
Dim rec = udpcl.Receive(RemoteIpEndPoint)
If rec.Length = 11 Then
Dim match = True
For i = 0 To 10
If rec(i) <> UDP_MSG(i) Then
match = False
Exit For
End If
Next
If match Then
Outoftray()
End If
End If
Catch e As Exception
End Try
End While
Catch e As Exception
End Try
End While
End SubTo me, this is some of the ugliest code I've ever seen. I mean, the compiler isn't THAT smart is it? To me, this looks like the dim'd object will be created and sent to GC every iteration through the loop! But then again, I don't have an intimate knowledge of what the compiler will do
I've seen uglier code than that, but there is still only so much the compiler can do with what it sees. Detection of "code intent" is what gives a compiler the ability to optimize, but it can only go so far. The compiler can optimize inside methods, but over large sections of an app or even a class? Not so much. The compiler cannot optimize you out of bad design.
Asking questions is a skill CodeProject Forum Guidelines Google: C# How to debug code Seriously, go read these articles.
Dave Kreskowiak -
So, I've done mainly VBc and C/C++ for embedded device, and a tiny bit of VC++. When .Net came along, after a while, it was often repeated that .Net allowed people to write bad code, but will still work. I was a stickler for following good programming form. On one occasion, someone had ask a question about what the .Net compiler will do with [whatever code it was], and someone else chimed in saying something else like 'the compiler is smart and will optimize...'. But, you know, I'm still a stickler for proper form, allocating vars and objects, properly destroying them, stopping any timers before program exit, etc. I guess, proper "cleanup" you could say. So I'm working with some code the company hired a supposedly experienced programmer to write a program for the company that monitors devices it makes. Some of this is so appalling, that it looks like an absolute beginner would write. So the below chunk of code is one function he had written. He has this running in it's own thread, and it's a UPD listening socket. This is used to prevent multiple instances of it running, and to return from the system tray after it's been minimized to the tray. So this function is always running in it's own thread...
Private Sub udpThreadsub()
While True
Try
udpcl = New UdpClient(UDPPORT)
Dim RemoteIpEndPoint As New IPEndPoint(IPAddress.Any, 0)
While True
Try
Dim rec = udpcl.Receive(RemoteIpEndPoint)
If rec.Length = 11 Then
Dim match = True
For i = 0 To 10
If rec(i) <> UDP_MSG(i) Then
match = False
Exit For
End If
Next
If match Then
Outoftray()
End If
End If
Catch e As Exception
End Try
End While
Catch e As Exception
End Try
End While
End SubTo me, this is some of the ugliest code I've ever seen. I mean, the compiler isn't THAT smart is it? To me, this looks like the dim'd object will be created and sent to GC every iteration through the loop! But then again, I don't have an intimate knowledge of what the compiler will do
Member 10097972 wrote:
VBc and C/C++ for embedded device, and a tiny bit of VC++.
Perhaps the rest of the comments are based on that background. Certainly for me (decades ago) working on computers with much smaller memories one had to be much, much more careful not only with how memory was used but how and when it was allocated. Computers have substantially more memory than almost all embedded devices. That was even true long ago. Both have gotten bigger since then. Additionally for PC computers the OS and compilers/interpreters have been designed for decades to optimize the usage of that memory. One need only read up on how memory heaps used to work (perhaps even how they are still taught) versus the complexity that goes into moderns heaps to see the changes. And even reading up on how the OS and even the CPUs are optimized to assist in that. Even physical memory and other hardware in computers has changed to facilitate that.
Member 10097972 wrote:
So this function is always running in it's own thread...
Excluding the VB part of that... Yep that is how you write it - in its own thread. Unclear to me why you seem to think that is a problem?
Member 10097972 wrote:
This is used to prevent multiple instances of it running,
Not sure what that is supposed to mean. But any IP port should never have more than one reader. Not in any circumstance. So mentioning seems odd since that is exactly what must happen.
Member 10097972 wrote:
the compiler isn't THAT smart is it?...I would have done the DIM's above the loops
I think this is your major concern. And yes the compiler (and interpreter in this case) is that smart. The code I see is getting a message, in a buffer, but still a message. So the API creates the buffer and not the code that you posted. You definitely could not have moved that outside the loop. (As mentioned compiler/interpreter will easily deal with this.) So I don't see any other allocations in the loop?
-
So, I've done mainly VBc and C/C++ for embedded device, and a tiny bit of VC++. When .Net came along, after a while, it was often repeated that .Net allowed people to write bad code, but will still work. I was a stickler for following good programming form. On one occasion, someone had ask a question about what the .Net compiler will do with [whatever code it was], and someone else chimed in saying something else like 'the compiler is smart and will optimize...'. But, you know, I'm still a stickler for proper form, allocating vars and objects, properly destroying them, stopping any timers before program exit, etc. I guess, proper "cleanup" you could say. So I'm working with some code the company hired a supposedly experienced programmer to write a program for the company that monitors devices it makes. Some of this is so appalling, that it looks like an absolute beginner would write. So the below chunk of code is one function he had written. He has this running in it's own thread, and it's a UPD listening socket. This is used to prevent multiple instances of it running, and to return from the system tray after it's been minimized to the tray. So this function is always running in it's own thread...
Private Sub udpThreadsub()
While True
Try
udpcl = New UdpClient(UDPPORT)
Dim RemoteIpEndPoint As New IPEndPoint(IPAddress.Any, 0)
While True
Try
Dim rec = udpcl.Receive(RemoteIpEndPoint)
If rec.Length = 11 Then
Dim match = True
For i = 0 To 10
If rec(i) <> UDP_MSG(i) Then
match = False
Exit For
End If
Next
If match Then
Outoftray()
End If
End If
Catch e As Exception
End Try
End While
Catch e As Exception
End Try
End While
End SubTo me, this is some of the ugliest code I've ever seen. I mean, the compiler isn't THAT smart is it? To me, this looks like the dim'd object will be created and sent to GC every iteration through the loop! But then again, I don't have an intimate knowledge of what the compiler will do
I know this is a few months old, but I got a chuckle out of this! =) Back in the day I moved from C++ to VB6, then to .NET and I've been there since. Honestly I wasn't the most excited about the changes, and I probably went through all the states of acceptance getting to know VB. It's funny how things turned out, because even though I don't use it much at all any more, I actually have fond memories of it. This code though, it's funny to look at. But I'm not sure I would call it horrible for the ways that most might in knee-jerk reaction. In plain English, it's intended to do the same thing over and over again. Listen for a a message, check if it meets a criteria "good enough to try getting what it wants", if it met the first challenge, but ultimately didn't work, try again. What I dislike about it is more the fact that it's not as explicit about exactly what it's looking for, and the fact that it's not even logging errors. The fact that it's destroying and re-creating the client in this case might be wasteful at first glance, but it's clear that whatever it's looking for is just meant to wake something else up, and it probably works as intended... For all we know if it received a malformed packet and they ignored it and tried again with the same instance of the client, the old "UNIX server in the back that nobody knows how to use any more" would catch fire and have to be hit with a hammer. =)
