|
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.
|
|
|
|
|
True, pretty code does not guarantee good code. See my sig. 
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
CDP1802 wrote: pflohneos, a finach or a fidel is?
That's quite elementary my dear Watson!
pfl = pathfile
ohne = without
os = OS
finach and fidel are variable containing FileInfo objects so the first part (fi) shoult be easy.
nach = after (so be on the lookout for a variable like fivor)
del = delete
To f*** things really up for my cow-workers I'd have preferred variables named after whiskeys. Or better yet random text strings. 
Cheers!
"With sufficient thrust, pigs fly just fine."
Ross Callon, The Twelve Networking Truths, RFC1925
|
|
|
|
|
How about hexadecimal?
F87AB986DEADC0DE and E87AB986DEADC0DE
At least artificial intelligence already is superior to natural stupidity
modified 4-May-12 4:13am.
|
|
|
|
|
Did he move from VB and miss the On Error Resume Next statement? 
Ideological Purity is no substitute for being able to stick your thumb down a pipe to stop the water
|
|
|
|
|
I think he was already around when the abacus was invented. But now that you mention it: VB is the only horror I have never seen him commit.
At least artificial intelligence already is superior to natural stupidity
|
|
|
|
|
'Nuff said.
public class SysAdmin : Employee
{
public override void DoWork(IWorkItem workItem)
{
if (workItem.User.Type == UserType.NoLearn){
throw new NoIWillNotFixYourComputerException(new Luser(workItem.User));
}else{
base.DoWork(workItem);
}
}
}
|
|
|
|
|
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)
{
}
if (HCID > 0)
{
}
|
|
|
|
|
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?
|
|
|
|
|
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
}
|
|
|
|
|
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
{
}
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.
|
|
|
|
|
"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.
|
|
|
|
|
Tom Delany wrote: but the Konstanten is gonna blow any second if I do!"
If only you knew how constant those 'constants' really are...
At least artificial intelligence already is superior to natural stupidity
modified 4-May-12 2:25am.
|
|
|
|
|
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}
|
|
|
|
|
Up against the wall! or is that too quick?
|
|
|
|
|
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
modified 4-May-12 8:44am.
|
|
|
|
|
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
|
|
|
|
|
Jeremy Hutchinson wrote: Freudian slip?
Must be. It could not even be explained with fat fingers 
At least artificial intelligence already is superior to natural stupidity
|
|
|
|
|
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.
|
|
|
|
|
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.
|
|
|
|
|
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
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
