See the writing on the wall...
-
It was possible that this developer would be sent to us to keep working on those projects. He declined, but if that had happened I would have preferred to help him. It would not have been easy, but then we could have used the situation to neverybody's benefit. But it's very hard to teach an old dog new tricks.
At least artificial intelligence already is superior to natural stupidity
Good point. I'm tainted by my current position where some one up the food chain is the absolute worst programmer I've met in my 12 years developing software. And because she's up the food chain, I have no ability to influence any changes in her coding style, and she reports to some one non-technical so nothing is ever going to change. If I could tar, feather, and fire her...
CDP1802 wrote:
we could have used the situation to neverybody's benefit
Freudian slip?
-
Good point. I'm tainted by my current position where some one up the food chain is the absolute worst programmer I've met in my 12 years developing software. And because she's up the food chain, I have no ability to influence any changes in her coding style, and she reports to some one non-technical so nothing is ever going to change. If I could tar, feather, and fire her...
CDP1802 wrote:
we could have used the situation to neverybody's benefit
Freudian slip?
Jeremy Hutchinson wrote:
If I could tar, feather, and fire her...
... and terminate her employment afterwards?
Did you ever see history portrayed as an old man with a wise brow and pulseless heart, waging all things in the balance of reason? Is not rather the genius of history like an eternal, imploring maiden, full of fire, with a burning heart and flaming soul, humanly warm and humanly beautiful? --Zachris Topelius Training a telescope on one’s own belly button will only reveal lint. You like that? You go right on staring at it. I prefer looking at galaxies. -- Sarah Hoyt
-
Good point. I'm tainted by my current position where some one up the food chain is the absolute worst programmer I've met in my 12 years developing software. And because she's up the food chain, I have no ability to influence any changes in her coding style, and she reports to some one non-technical so nothing is ever going to change. If I could tar, feather, and fire her...
CDP1802 wrote:
we could have used the situation to neverybody's benefit
Freudian slip?
-
The sad thing is that it is beautifully indented, follows code formatting standards and probably passes all sorts of automated metrics (though empty catch blocks should trip up any of those). And yet it's completely, irredeemably awful.
-
You are right. As if DD's CCC had been turned into a convention for variable names :) Pf = Pfad (Path) l = ??? ohne = without os = operating system (Does that make any sense at all? Sounds just like 'Purple dog over the moon') fi = file info nach = after (after what, exactly)? I have marked the printout on the wall with color markers and also added some notes. At that place I also wrote 'Castro?' :) Ah, I got it! He's deleting files! fi = file (please note that he has galantly switched to English now) del = delete Here's another good one: danapfl dana = Dateiname (filename, first I thought he started to name things after his daughters) pf = Pfad (path) l = ??? What form of insanity is this and how is it treated?
At least artificial intelligence already is superior to natural stupidity
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.
-
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
It looks like German, I think you've already figured that out. I've had the pleasure in the past of maintaining code with variables and function names written in Russian, Polish and Hebrew. Good Times.
-
What you are doing there is controlling the program flow by catching the exception and doing nothing else with it. That's not very good because exception handling is very slow. A simple 'if' to check wether you have gotten any rows would suffice and your code would be faster and besides that easier to read and understand. ExecuteScalar() is a bit ugly because it returns an object if I remember right, so you must check for a null reference and after that use an adequate type cast. Besides that, just take exceptions by their name. They point out unexpected or unusual events. Don't use them for anything you expect. I usually take care of three cases: Some exceptions are avoidable. They are just cases which I did not consider in my code. This would be the NullReferenceException for example. Checking for null at the proper places is easy enough, but once in a while you simply forget it or did not expect a null value at that location. To prevent the program from terminating there should be a 'line of defense' where all exceptions are caught and at least are logged in some way, together with a stack trace and, if possible, any other relevant parameters. If you go through the trouble to read the log and then eliminating the causes of the exceptions, then you have a mechanism that's worth more than many unit tests and you will eventually get a very robust program. Then there are correctable exceptions. Obviously it's then a good idea to do whatever needs to be done to correct the problem in the catch block. Again, avoid the exception if you can. This is only for those cases you can't handle in any other way. And log it. When using your last option shows up too often in the log, it's time to consider improving the code. And then there are exceptions you cannot foresee, like calling a web service and getting a timeout because the server does not respond. Nothing much you can do about that except trying again later (or taking a look at the server if you can). Anyway, whatever you wanted to do is impossible for now and the exception then should be caught so that the following now futile steps are omitted.
At least artificial intelligence already is superior to natural stupidity
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?
-
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.