See the writing on the wall...
-
ok, this makes me question some of my code. I am pretty much self taught and don't have a lot of resources available other than online and within the help features. I comment quite a bit and try to use easy to understand variables (because otherwise a few months down the road when I need to add an update, I need to look back and know what I was doing). However, I do use empty catch blocks fairly often. Typically what I will do is set a variable to a default, then use a DB query to populate the variable with in a try/catch block (if the query returns no record it throws an exception). Then after the catch block, I'll check if the variable is the default value or not and execute code accordingly. Is there a better way or a standard practice that would be better? (see my sample snippet below, I use the prefix lowercase L to indicate the scope of variables that I reuse often)NOTE: This is a Windows Form app not a web site
int HCID = -1; try { lSQL = "Select HCID from HistoricalConversion where CoNum = '" + HCCoNum.Text + "'"; lAccessCmd.CommandText = lSQL; HCID = (int)lAccessCmd.ExecuteScalar(); } catch (Exception ex) {//Ignore error if no record exists } //Only run this code if there is an active historical conversion for this company if (HCID > 0) { //My code to run a stored Procedure if the record exists }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
-
"Scotty. We need more power!" I canna give you any more Captain! The pflohneos is caught in the danapfl. I tried to cross-connect to the pflnamen, but the Konstanten is gonna blow any second if I do!"
WE ARE DYSLEXIC OF BORG. Refutance is systile. Your a$$ will be laminated. There are 10 kinds of people in the world: People who know binary and people who don't.
-
Did he move from VB and miss the
On Error Resume Nextstatement? X|Ideological Purity is no substitute for being able to stick your thumb down a pipe to stop the water
-
"Scotty. We need more power!" I canna give you any more Captain! The pflohneos is caught in the danapfl. I tried to cross-connect to the pflnamen, but the Konstanten is gonna blow any second if I do!"
WE ARE DYSLEXIC OF BORG. Refutance is systile. Your a$$ will be laminated. There are 10 kinds of people in the world: People who know binary and people who don't.
LOL, this guy's had problems with deleting a file - so he's (badly) put some tests in. pfl = program file - as my best guess I think what was intended was to locate a file, check if it was readonly, make a copy in a different location, then delete the original -- all pretty straight forward. However rather the checking for readonly he toggles it, in his local storage (not on the file) - and he may toggle it, once or twice - so you can never know what state the file was. In the end Delete file - ignore any failures (including a file that doesn't exist). A perfect implementation of try {stuff} catch {nothing} finally {pretend it worked}
-
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
Up against the wall! or is that too quick?
-
This is more like a city with every manhole opened.Care to go for a walk?
At least artificial intelligence already is superior to natural stupidity
CDP1802 wrote:
This is more like a city with every manhole opened.Care to go for a walk?
Blindfolded.
-
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
CDP1802 wrote:
My recommendation is to quickly write another application and then tar and feather the guy who wrote this one.
Both of those ideas seem like the only sane thing to do.
-
CDP1802 wrote:
My recommendation is to quickly write another application and then tar and feather the guy who wrote this one.
Both of those ideas seem like the only sane thing to do.
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
-
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.
