Reviewing code...
-
I often find myself having to review code within all of our systems as our company grows/changes, as im sure most of us have to during the course of work. Sometimes I find myself looking at old code, and feeling embaressed for the coder (who is sometimes myself) as code that was applicable due to knowledge at the time looks frightful in the light of newer knowledge and skills, but occasionally I come across absolute gems. The coder who wrote this (who shall remain unamed) is no longer with the company, but I do not regret the loss one bit in the face of massive amounts of code being uncovered of this stunning calibre; Classic ASP:
If Request.ServerVariables("REQUEST_METHOD") = "POST" Then
If Request.Form("btnAddTask") <> "" Then
For Each obj In Request.FormiSectionID_New = ""
If Len(iSectionID_New) = 0 Then
iSectionID_New = Trim(Request.Form("section_id_filter"))
End IfiTaskID_New = ""
If Len(iTaskID_New) = 0 Then
iTaskID_New = Trim(Request.Form("task_new"))
End IfiDisplayOrder_New = ""
If Len(iDisplayOrder_New) = 0 Then
If Len(Trim(Request.Form("display_order_new"))) > 0 And IsNumeric(Trim(Request.Form("display_order_new"))) Then
iDisplayOrder_New = Trim(Request.Form("display_order_new"))
End If
End IfiActionOrder_New = ""
If Len(iActionOrder_New) = 0 Then
If Len(Trim(Request.Form("action_order_new"))) > 0 And IsNumeric(Trim(Request.Form("action_order_new"))) Then
iActionOrder_New = Trim(Request.Form("action_order_new"))
End If
End If
Next
End if
End ifthe bold sections are the ones that immediately drew the attention of the team, but we then looked at the logic itself, and were stunned by the loop itself
For Each obj In Request.Formwhich not only is never used, but just increases the number of iterations of the same code for absolutely no reason whatsoever... Anyone else have the joy of such discoveries?? Any clangers found that left you slack-jawed and slightly disoriented?? :) :)Often such strange things are the result of tweaking the code until it finally worked and not taking the time to clean up the debris from all this trial and error.
A while ago he asked me what he should have printed on my business cards. I said 'Wizard'. I read books which nobody else understand. Then I do something which nobody understands. After that the computer does something which nobody understands. When asked, I say things about the results which nobody understand. But everybody expects miracles from me on a regular basis. Looks to me like the classical definition of a wizard.
-
I often find myself having to review code within all of our systems as our company grows/changes, as im sure most of us have to during the course of work. Sometimes I find myself looking at old code, and feeling embaressed for the coder (who is sometimes myself) as code that was applicable due to knowledge at the time looks frightful in the light of newer knowledge and skills, but occasionally I come across absolute gems. The coder who wrote this (who shall remain unamed) is no longer with the company, but I do not regret the loss one bit in the face of massive amounts of code being uncovered of this stunning calibre; Classic ASP:
If Request.ServerVariables("REQUEST_METHOD") = "POST" Then
If Request.Form("btnAddTask") <> "" Then
For Each obj In Request.FormiSectionID_New = ""
If Len(iSectionID_New) = 0 Then
iSectionID_New = Trim(Request.Form("section_id_filter"))
End IfiTaskID_New = ""
If Len(iTaskID_New) = 0 Then
iTaskID_New = Trim(Request.Form("task_new"))
End IfiDisplayOrder_New = ""
If Len(iDisplayOrder_New) = 0 Then
If Len(Trim(Request.Form("display_order_new"))) > 0 And IsNumeric(Trim(Request.Form("display_order_new"))) Then
iDisplayOrder_New = Trim(Request.Form("display_order_new"))
End If
End IfiActionOrder_New = ""
If Len(iActionOrder_New) = 0 Then
If Len(Trim(Request.Form("action_order_new"))) > 0 And IsNumeric(Trim(Request.Form("action_order_new"))) Then
iActionOrder_New = Trim(Request.Form("action_order_new"))
End If
End If
Next
End if
End ifthe bold sections are the ones that immediately drew the attention of the team, but we then looked at the logic itself, and were stunned by the loop itself
For Each obj In Request.Formwhich not only is never used, but just increases the number of iterations of the same code for absolutely no reason whatsoever... Anyone else have the joy of such discoveries?? Any clangers found that left you slack-jawed and slightly disoriented?? :) :)I'm astounded that people will criticize code, but make spelling mistakes during their public diatribe ("embaressed").
GC
-
Often such strange things are the result of tweaking the code until it finally worked and not taking the time to clean up the debris from all this trial and error.
A while ago he asked me what he should have printed on my business cards. I said 'Wizard'. I read books which nobody else understand. Then I do something which nobody understands. After that the computer does something which nobody understands. When asked, I say things about the results which nobody understand. But everybody expects miracles from me on a regular basis. Looks to me like the classical definition of a wizard.
-
I often find myself having to review code within all of our systems as our company grows/changes, as im sure most of us have to during the course of work. Sometimes I find myself looking at old code, and feeling embaressed for the coder (who is sometimes myself) as code that was applicable due to knowledge at the time looks frightful in the light of newer knowledge and skills, but occasionally I come across absolute gems. The coder who wrote this (who shall remain unamed) is no longer with the company, but I do not regret the loss one bit in the face of massive amounts of code being uncovered of this stunning calibre; Classic ASP:
If Request.ServerVariables("REQUEST_METHOD") = "POST" Then
If Request.Form("btnAddTask") <> "" Then
For Each obj In Request.FormiSectionID_New = ""
If Len(iSectionID_New) = 0 Then
iSectionID_New = Trim(Request.Form("section_id_filter"))
End IfiTaskID_New = ""
If Len(iTaskID_New) = 0 Then
iTaskID_New = Trim(Request.Form("task_new"))
End IfiDisplayOrder_New = ""
If Len(iDisplayOrder_New) = 0 Then
If Len(Trim(Request.Form("display_order_new"))) > 0 And IsNumeric(Trim(Request.Form("display_order_new"))) Then
iDisplayOrder_New = Trim(Request.Form("display_order_new"))
End If
End IfiActionOrder_New = ""
If Len(iActionOrder_New) = 0 Then
If Len(Trim(Request.Form("action_order_new"))) > 0 And IsNumeric(Trim(Request.Form("action_order_new"))) Then
iActionOrder_New = Trim(Request.Form("action_order_new"))
End If
End If
Next
End if
End ifthe bold sections are the ones that immediately drew the attention of the team, but we then looked at the logic itself, and were stunned by the loop itself
For Each obj In Request.Formwhich not only is never used, but just increases the number of iterations of the same code for absolutely no reason whatsoever... Anyone else have the joy of such discoveries?? Any clangers found that left you slack-jawed and slightly disoriented?? :) :)I think my favorite was an Access program (yes, there are things worse than VB), where most of the logic was in nested IIfs within queries. After copying them into a text editor so I could separate them out enough to see what was going on, I discovered that the last 3 or 4 of the 10-15 nested IIfs all =0 for the true result. There were 4-5 query columns like this. So the last 3 or 4 conditions didn't need to be checked because they would all result in 0. If a, then 0, else if b, then 0, else if c then 0. Lovely code.
-
I once came upon a huge line that contained about 15 nested if then else statements. Not only was it ugly, it was very inefficient. Horrid little bit of code, but it worked, although owing to the nature of what it was doing, a simple Case would have been infinitely preferable. There was no reason to do it that way, and I told myself off!
------------------------------------ I will never again mention that I was the poster of the One Millionth Lounge Post, nor that it was complete drivel. Dalek Dave
For the last 18 months I have been stuck with maintaining a 2500 line if statement. (I have never managed to actually count how deep the layers get.) And, to make matters worse there is no means of unit testing available. Thank goodness the replacement system is in place.
-
I often find myself having to review code within all of our systems as our company grows/changes, as im sure most of us have to during the course of work. Sometimes I find myself looking at old code, and feeling embaressed for the coder (who is sometimes myself) as code that was applicable due to knowledge at the time looks frightful in the light of newer knowledge and skills, but occasionally I come across absolute gems. The coder who wrote this (who shall remain unamed) is no longer with the company, but I do not regret the loss one bit in the face of massive amounts of code being uncovered of this stunning calibre; Classic ASP:
If Request.ServerVariables("REQUEST_METHOD") = "POST" Then
If Request.Form("btnAddTask") <> "" Then
For Each obj In Request.FormiSectionID_New = ""
If Len(iSectionID_New) = 0 Then
iSectionID_New = Trim(Request.Form("section_id_filter"))
End IfiTaskID_New = ""
If Len(iTaskID_New) = 0 Then
iTaskID_New = Trim(Request.Form("task_new"))
End IfiDisplayOrder_New = ""
If Len(iDisplayOrder_New) = 0 Then
If Len(Trim(Request.Form("display_order_new"))) > 0 And IsNumeric(Trim(Request.Form("display_order_new"))) Then
iDisplayOrder_New = Trim(Request.Form("display_order_new"))
End If
End IfiActionOrder_New = ""
If Len(iActionOrder_New) = 0 Then
If Len(Trim(Request.Form("action_order_new"))) > 0 And IsNumeric(Trim(Request.Form("action_order_new"))) Then
iActionOrder_New = Trim(Request.Form("action_order_new"))
End If
End If
Next
End if
End ifthe bold sections are the ones that immediately drew the attention of the team, but we then looked at the logic itself, and were stunned by the loop itself
For Each obj In Request.Formwhich not only is never used, but just increases the number of iterations of the same code for absolutely no reason whatsoever... Anyone else have the joy of such discoveries?? Any clangers found that left you slack-jawed and slightly disoriented?? :) :)What about the next piece of code, which was endorsed by the product manager :
while ( true)
{
if( formName == Evita.GuiCommon.Constants.GuiScreens.STARTING_UP)
{
this.mExeStartingUpHandler.Display();
break;
}
if( formName == Evita.GuiCommon.Constants.GuiScreens.Exe.ADD_CLEANING_STATION)
{
this.mExeAddCleaningStationHandler.Display(this.mTcpIpMessages);
break;
}
else if( formName == Evita.GuiCommon.Constants.GuiScreens.Exe.ADD_DRIVER)
{
this.mExeAddDriverHandler.Display(this.mTcpIpMessages);
break;
}
else if (...)
...
}The reason for doing the while loop with the breaks : it runs faster than if-else-if without it. Also note there was no final break, so if the loop was entered with an unknown 'formName', it would loop forever. Countless arguments against doing things like this came to my mind, but they were all disregarded and this was the way to do it. Then they wondered why some people started to rebel.
-
True. In many cases, if a page shows up without a significant delay, its code might not be looked at again.
TOMZ_KV
It has happened to everyone, I guess. Be it that, you had only little time or the next emergency had to be taken care of. Whatever happened, you simply forgot to clean up.
A while ago he asked me what he should have printed on my business cards. I said 'Wizard'. I read books which nobody else understand. Then I do something which nobody understands. After that the computer does something which nobody understands. When asked, I say things about the results which nobody understand. But everybody expects miracles from me on a regular basis. Looks to me like the classical definition of a wizard.
-
It has happened to everyone, I guess. Be it that, you had only little time or the next emergency had to be taken care of. Whatever happened, you simply forgot to clean up.
A while ago he asked me what he should have printed on my business cards. I said 'Wizard'. I read books which nobody else understand. Then I do something which nobody understands. After that the computer does something which nobody understands. When asked, I say things about the results which nobody understand. But everybody expects miracles from me on a regular basis. Looks to me like the classical definition of a wizard.
-
I'm astounded that people will criticize code, but make spelling mistakes during their public diatribe ("embaressed").
GC
-
I think my favorite was an Access program (yes, there are things worse than VB), where most of the logic was in nested IIfs within queries. After copying them into a text editor so I could separate them out enough to see what was going on, I discovered that the last 3 or 4 of the 10-15 nested IIfs all =0 for the true result. There were 4-5 query columns like this. So the last 3 or 4 conditions didn't need to be checked because they would all result in 0. If a, then 0, else if b, then 0, else if c then 0. Lovely code.