See the writing on the wall...
-
Today I have my code horror hanging on the wall, in form of a printout from the ceiling to the floor. It's from a Win CE application which had been sent to a customer totally untested. Of course it failed miserably. It's almost ironic, but reporting the error in a message box also fails, due to a missing resource assembly. There also is no logging or any other mechanism to record errors. In short: Nobody really knows what's going on, not even the criminal who wrote that program. The good news: The form that produces the error only has three methods, PageLoad() and two others. PageLoad() is only a few lines long. Initialisation, nothing really problematic. The first of the two other methods also was quite short. No problems here. And now method #3: - 350 lines of best spaghetti code rolled in a loop. Whenever this is executed, it will then hang up the entire application until it is done. - A total of 17 try/catch blocks. Most catch blocks are totally empty. There is no error handling. The errors are simply swept under the rug and the code goes on as if nothing happened. Only three try to at least show the error in a message box, but our friend also failed at doing that. A brief scan of the rest of the application's code revealed that this is one of his favorite practices. - There are eight code sequences which appear redundantly in this spaghetti monster. They could easily have been refactored into methods, but that of course would have harmed this work of art. - The least of the problems, but does anybody know what a pflohneos, a finach or a fidel is? Those are just some of the variable names and they don't make any sense in any language I know. Edit: Here a tiny sample cut out of the spaghetti monster. It's easy to see what he tries to do, but I can only guess what the intentions behind all this are. There are no comments and the strange variable names don't help very much:
try
{
string pflohneos = pfl.Name.ToUpper().Replace(pflnamen, "");
string danapfl = Path.Combine(Konstanten.Flashdir, pflohneos);
MessageLabel.Text = String.Format(tt.Readwert("t1"), danapfl);
Refresh();
try
{
FileInfo finach = new FileInfo(danapfl);
if (finach != null)
{
finach.Attributes &= ~FileAttributes.ReadOnly;
}
}
catch
{
}
pfl.Attributes &= ~FileAttributes.ReadOnly;
pfl.CopyTo(danapfl, true);
pfl.Delete();
}
catch
{
}This is really an insult to the customer. Good that they will
Ummm. 350 lines? I once worked for a customer that had a rolled their antenna control state machine into a single 12,000 line method. It was coded as a single loop with a switch statement, and used various means of state caching to achieve nesting. The part of the code I had to modify was down around line 17,000 of the file, which caused the Visual Studio editor of the era to get confused about where to insert characters. I did most of the modifications in WordPad.
-
Yes that is exactly what I am doing. I'm using the try/catch to handle the null reference exception that is thrown if there is no value. Is there a better way to do it?
spotsknight wrote:
Yes that is exactly what I am doing
No it's not, you're waiting for an error to happen and then you choose to ignore it.
spotsknight wrote:
Is there a better way to do it?
Like CDP said, you check the condition. Instead of:
HCID = (int)lAccessCmd.ExecuteScalar();
You can use something like:
object o = lAccessCmd.ExecuteScalar();
if (o is int)
{
HCID = (int)o
}Light moves faster than sound. That is why some people appear bright, until you hear them speak. List of common misconceptions
-
spotsknight wrote:
Yes that is exactly what I am doing
No it's not, you're waiting for an error to happen and then you choose to ignore it.
spotsknight wrote:
Is there a better way to do it?
Like CDP said, you check the condition. Instead of:
HCID = (int)lAccessCmd.ExecuteScalar();
You can use something like:
object o = lAccessCmd.ExecuteScalar();
if (o is int)
{
HCID = (int)o
}Light moves faster than sound. That is why some people appear bright, until you hear them speak. List of common misconceptions
Thank you! I didn't realize you could check the type of an object like that. Like I said I'm pretty much self taught and appreciate all suggestions to improve my code. One more question. Another thing I used the try/catch blocks for was to stop running code and display a custom error. For example if a company already existed in the database I'd set a custom message and throw an exception. Is that still a valid use of that or would it be better to build the code within the if statements. i.e. after removing the extra try/catch blocks so now I only have one within the function, would I use;
if (tmpobj is int)
{
msg = "Company already has a project assigned";
throw new Exception();
}Then in the catch I display the msg. or instead would I use;
if (tmpobj is int)
msg = "Company already has a project assigned";
else
{
//the rest of my code
}
MessageBox.Show(msg); -
Today I have my code horror hanging on the wall, in form of a printout from the ceiling to the floor. It's from a Win CE application which had been sent to a customer totally untested. Of course it failed miserably. It's almost ironic, but reporting the error in a message box also fails, due to a missing resource assembly. There also is no logging or any other mechanism to record errors. In short: Nobody really knows what's going on, not even the criminal who wrote that program. The good news: The form that produces the error only has three methods, PageLoad() and two others. PageLoad() is only a few lines long. Initialisation, nothing really problematic. The first of the two other methods also was quite short. No problems here. And now method #3: - 350 lines of best spaghetti code rolled in a loop. Whenever this is executed, it will then hang up the entire application until it is done. - A total of 17 try/catch blocks. Most catch blocks are totally empty. There is no error handling. The errors are simply swept under the rug and the code goes on as if nothing happened. Only three try to at least show the error in a message box, but our friend also failed at doing that. A brief scan of the rest of the application's code revealed that this is one of his favorite practices. - There are eight code sequences which appear redundantly in this spaghetti monster. They could easily have been refactored into methods, but that of course would have harmed this work of art. - The least of the problems, but does anybody know what a pflohneos, a finach or a fidel is? Those are just some of the variable names and they don't make any sense in any language I know. Edit: Here a tiny sample cut out of the spaghetti monster. It's easy to see what he tries to do, but I can only guess what the intentions behind all this are. There are no comments and the strange variable names don't help very much:
try
{
string pflohneos = pfl.Name.ToUpper().Replace(pflnamen, "");
string danapfl = Path.Combine(Konstanten.Flashdir, pflohneos);
MessageLabel.Text = String.Format(tt.Readwert("t1"), danapfl);
Refresh();
try
{
FileInfo finach = new FileInfo(danapfl);
if (finach != null)
{
finach.Attributes &= ~FileAttributes.ReadOnly;
}
}
catch
{
}
pfl.Attributes &= ~FileAttributes.ReadOnly;
pfl.CopyTo(danapfl, true);
pfl.Delete();
}
catch
{
}This is really an insult to the customer. Good that they will
A bunch of try/catch, poorly written serial code, isn't spaggetti code (irritating and difficult to understand/debug, yes) A real "S" epic has about 4-5k lines in one routine, conditionals on most of the branches, a bit of dead code thrown in, and logic similar to: goto 123 3000 continue goto 3214 10 continue goto 143 5130 continue exit 1634 continue goto 3214 123 continue goto 10 3214 continue goto 5130 143 continue goto 3000
-
Maybe he's using a technique of a friend I used to program with. His style was that all variables names must be 8 characters long. He would take 8 divided by the number of words you would normally use to describe the function/variable name to arrive at the number of letters to use from each word. So "Print Spooler" would become "PRINSPOO" (not so bad, but note all caps), but "Shipping Station Control Loop" would become "SHSTCOLO". Maybe as you suspect "dana" means "date in name", so "finach" may be "File Name Change"?
Psychosis at 10 Film at 11 Those who do not remember the past, are doomed to repeat it. Those who do not remember the past, cannot build upon it.
Dateiname = Datei Name = file name. Datei is German word for file.
-
Thank you! I didn't realize you could check the type of an object like that. Like I said I'm pretty much self taught and appreciate all suggestions to improve my code. One more question. Another thing I used the try/catch blocks for was to stop running code and display a custom error. For example if a company already existed in the database I'd set a custom message and throw an exception. Is that still a valid use of that or would it be better to build the code within the if statements. i.e. after removing the extra try/catch blocks so now I only have one within the function, would I use;
if (tmpobj is int)
{
msg = "Company already has a project assigned";
throw new Exception();
}Then in the catch I display the msg. or instead would I use;
if (tmpobj is int)
msg = "Company already has a project assigned";
else
{
//the rest of my code
}
MessageBox.Show(msg);As CDP said - exceptions are named for the circumstances where you need to deal with the unexpected. In your example "the company already having a project assigned" could perhaps be considered an error; most programmers would not, however, consider it to be an exception. So here, too, the use of exceptions to deal with the situation is inappropriate. Given your specific example, I would venture to say that "the company already having a project assigned" is not even an error, but just a specific condition that you need to manage - and your example of showing a message to the user (second example) is consistent with that interpretation. None - after assimilating what conditions could reasonably considered exceptions (out of memory; out of hard drive space; file not found when it really should be there; web service not responding, etc.) it becomes an interesting question as to what an 'error' actually is, and whether it's a term that's safe to use. I'd say that there is no right answer to that question - but there is definitely value in considering the question and the implications of any answer you come up with.
-
Is the nutcase an old git that learned programming when bytes were expensive? I'm thinking about classic unix folder names like 'USR' here. It could be fun decrypting this, but I think I would need to see more of the spaghetti monster to have a fair chance.
Light moves faster than sound. That is why some people appear bright, until you hear them speak. List of common misconceptions
I'm one of those old gits. Honestly, I don't see what the problem is with the code except for the variable names. The programmer set all the files to read-only as a precaution. Probably not necessary now with the fast drives and IO buses, but in the 90's, it would have been a good precaution. As for the term spaghetti monster, the term doesn't fit the example. At no time does the code jump out of scope, unless the definition has changed to include bad variable naming.
-
I'm one of those old gits. Honestly, I don't see what the problem is with the code except for the variable names. The programmer set all the files to read-only as a precaution. Probably not necessary now with the fast drives and IO buses, but in the 90's, it would have been a good precaution. As for the term spaghetti monster, the term doesn't fit the example. At no time does the code jump out of scope, unless the definition has changed to include bad variable naming.
Can't answer the part about spaghetti code or the other errors as I haven't seen more of the code than anyone else on the forum. But empty catch blocks is a coding horror in my book. And those abbreviated variable names are just plainly unnecessary today.
Light moves faster than sound. That is why some people appear bright, until you hear them speak. List of common misconceptions
-
Can't answer the part about spaghetti code or the other errors as I haven't seen more of the code than anyone else on the forum. But empty catch blocks is a coding horror in my book. And those abbreviated variable names are just plainly unnecessary today.
Light moves faster than sound. That is why some people appear bright, until you hear them speak. List of common misconceptions
Cryptic variable names would have been a prrogramming crime 20 years back too. It was the binaries where bytes were expensive, not the source code.
-
The real horror is that mess should never have gone out the door and into the customer's hands. X| The customer needs to be given proper replacement ASAP and, of course the original programmer should not touch the replacement program. And tar and feathers is to good for the individual who wrote that mess. :wtf:
Just because the code works, it doesn't mean that it is good code.
My guess is that there's a good chance that this mess got to be delivered because the customer is a cheapskate. That's what you get when you contract to script kiddies instead of serious, qualified and more expensive programmers. And when you insist on things to be delivered yesterday, but never have time to discuss the actual requirements with your service provider.
-
Cryptic variable names would have been a prrogramming crime 20 years back too. It was the binaries where bytes were expensive, not the source code.
Florin Jurcovici wrote:
Cryptic variable names would have been a prrogramming crime 20 years back too
Indeed, I was thinking more like 40-50 years back in time when harddrives were a novelty.
Light moves faster than sound. That is why some people appear bright, until you hear them speak. List of common misconceptions
-
My guess is that there's a good chance that this mess got to be delivered because the customer is a cheapskate. That's what you get when you contract to script kiddies instead of serious, qualified and more expensive programmers. And when you insist on things to be delivered yesterday, but never have time to discuss the actual requirements with your service provider.