|
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.
|
|
|
|
|
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.
|
|
|
|
|
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.
|
|
|
|
|
The real horror is that mess should never have gone out the door and into the customer's hands. 
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. 
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
That's what I have rcommended. It will not be sufficient to refactor a little, as my bosses hope. I have inherited all his projects from the last eight years. They all are such a mess. At least I'm honored that my bosses call me every time they think that all hope is lost 
At least artificial intelligence already is superior to natural stupidity
|
|
|
|
|
Well, it is good to feel needed, but that is a really nasty mess to have to clean up.
Yes, bosses always want a quick, inexpensive fix, but I agree with you that there is no simple quick fix for this. 
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.
|
|
|
|
|
Yep, SOP is to provide little time, little money, little information, then blame the programmers when the software isn't perfect.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
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.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
