|
I inherited a Borland Win32 application that has over 60 global variables interspersed throughout it. Many of the globals are multilevel struct instantiations.
Nearly every class definition has a line similar to this after it: "extern PACKAGE TTree *Tree;" The actual declarations of these variables are either in the code for the "about page", above the program's "main" function, or in "globals.cpp"
There is a file called "structs.h" which contains the definitions for every struct used in the application. structs that have to do with the database, visual controls, file contents, etc are all in the same file. The file is 6,000 lines long.
There is a file called "globals.h" that is 3,000 lines long that is nothing but #defines. The #defines are related to, you guessed it: database, individual visual controls, different file formats, and other processing specific code. The file is included in every .cpp file. Very few of the #defines have meaning in more two .cpp files.
8 of the 28 .cpp files in the project are over 10,000 lines long. The longest is 14,433 lines. Most of the code in these mammoths has nothing to do with what the file name would have you think they are doing.
Instead of using one of the hundred different XML processing libraries out there, this guy decides to write his own in order to make a way of saving data. The produced files aren't even valid XML so I can't replace it with a more efficient XML processor. I have to maintain his version of XML syntax for backward compatibility.
There isn't a single pass by reference in the entire application. It is all pointers.
There is no function polymorphism being performed. There are a ton of functions with parameters that specify the kind of data being handled with switch-cases that handle different kinds of data.
Every struct declaration looks like this: "struct CALIB_STRUCT Calib;" Always all caps, always has the "struct" keyword, and always ends with "_STRUCT".
There is no real naming scheme. Sometimes constants are all caps, sometimes they aren't, sometimes they are structs.
He declares every variable that could possibly be used by the function at the top of the function. This includes those huge structs that never get used. Get this guy a copy of the C++ standard please! Many functions are over 500 lines long. Thank goodness for split-screened editors.
The only container that is used are statically defined arrays (usually of structs) even when there is no way to know how many elements will be in it. He just makes it big and forgets about it.
There is no class inheritance, encapsulation, polymorphism, or specialization for different kinds of data. Just dozens of functions with huge multilevel switch-case statements to handle the "special" cases of each type of data.
Every function, I mean every single function, has something similar to this as its first line:
AnsiString errorHeader = "Showing Probe Calibration Error: ";
errorHeader += "Function Where Error Occurred: ";
errorHeader += "TfrmConfig::SaveCalData(); Type of error: ";
Every function, I mean every single function, has this exact code at the end (misc is a global of type TMisc):
} // end catch
catch(Exception &e)
{
errorMsg = errorHeader;
errorMsg += e.ClassName();
errorMsg += " Exception; Error message: " + e.Message;
if (Misc)
Misc->LogSystemError(errorMsg);
error = EI_KNOWN_EXCEPTION;
}
catch(...)
{
errorMsg = errorHeader + "Unknown Error";
if (Misc)
{
Misc->LogSystemError(errorMsg);
}
error = EI_UNKNOWN_EXCEPTION;
}
return error;
There is no other exception handling being performed.
The list goes on. These are just the things I can easily describe here. Overall, this guy just loves to copy and paste code. Apparently making a robust function or class specialization is just too much work.
In debugging and running the code, I find that along a single execution path functions may be needlessly called multiple times to fill visual controls with statically defined data. I am seriously terrified to run a code profiler on it. This guy wrote another similar size app here that has also landed in my lap.
Did I mention that this guy teaches at ITT Tech? He no longer works here (I replaced him). Oddly enough, he wasn't let go for incompetence.
|
|
|
|
|
Ugh, my wife was the director of placement at an ITT Tech a bunch of years ago (late 90s).
One day she came home and said I "had to" look at this great website one of the graduates had created for his employer.
It was all jpg images! That's what they were being taught. I couldn't read a word of it.
Dreadful dreadful dreadful.
Then again, the C code I wrote when just out of college has warts all its own. New programmers really have to work under the supervision of experienced developers.
|
|
|
|
|
PIEBALDconsult wrote: Then again, the C code I wrote when just out of college has warts all its own. New programmers really have to work under the supervision of experienced developers.
I have to agree. No matter how smart a person is they need to work with someone who really knows the business. That is why in the old days trades had apprentices and journeymen that were taught by masters of their craft. Nowdays, anyone with a couple programming classes thinks they are a programmer.
Bill W
|
|
|
|
|
Geeze.
"The clue train passed his station without stopping." - John Simmons / outlaw programmer
"Real programmers just throw a bunch of 1s and 0s at the computer to see what sticks" - Pete O'Hanlon
|
|
|
|
|
Great, so now he's teaching his brand of programming to the next generation of Indian programmers.
More job security for the west then. Brillant.
|
|
|
|
|
No, that'd be IIT. ITT is a US tech school (2 year degrees) chain.
Today's lesson is brought to you by the word "niggardly". Remember kids, don't attribute to racism what can be explained by Scandinavian language roots.
-- Robert Royall
|
|
|
|
|
Ah my bad. easy mistakea-to-makea.
|
|
|
|
|
maizhiming wrote: aren't even valid XML
You mean not well-formed?
|
|
|
|
|
Here is an example:
<Stagenumber="0" minaw="0.1" maxaw="0.8" desorpFr="300" adsorpFr="300" used="true"/>
I didn't know elements could have values. Well they can't. XML viewers won't even read it.
You should see the code that parses this. It is pretty much one gigantic function that reads in the file character by character.
|
|
|
|
|
Maybe he was really writing an SGML parser instead, and just got confused on the extension.
Today's lesson is brought to you by the word "niggardly". Remember kids, don't attribute to racism what can be explained by Scandinavian language roots.
-- Robert Royall
|
|
|
|
|
Thank goodness I didn't get my programming degree at ITT Tech!
I went there for CADD and Electronics Engineering instead.
Imagine that you are hired to build a bridge over a river which gets slightly wider every day; sometimes it shrinks but nobody can predict when. Your client provides no concrete or steel, only timber and cut stone (but they won't tell you what kind). The coefficient of gravity changes randomly from hour to hour, as does the viscosity of air. Your only tools are a hacksaw, a chainsaw, a rubber mallet, and a length of rope.
Welcome to my world.
-Me explaining my job to an engineer
|
|
|
|
|
maizhiming wrote: There isn't a single pass by reference in the entire application. It is all pointers.
This one I don't get. The only thing references give you, is a nicer syntax for objects of classes with overloaded operators. A reference is just a shorthand notation for *ptr . And by the sound of it, it sounds like the code is based off of some legacy C code.
maizhiming wrote: catch(...)
Now that one's just plain horrible. That one will catch anything. Stuff that ought to be handled by Dr Watson, or at the very least terminate the program in some graceful way.
maizhiming wrote: Oddly enough, he wasn't let go for incompetence.
Have you worked long where you work now? Maybe you should take a long hard look at the time ahead of you before the probation time is over (assuming you have a probation period of your employment). Maybe they (your boss(es)) couldn't see his incompetence because of their own?
--
Kein Mitleid Für Die Mehrheit
|
|
|
|
|
Alright, so I'm young and new to programming in general and that kind of made me puke in my mouth a little bit. I guess I get to thank my lucky stars that I have 1 of probably 5 really great programmers (of close to 40) in my shop as my immediate supervisor. People here call him the .NET daddy.
But yeah, I guess I'm not going to ITT Tech for my degree...
maizhiming wrote: Oddly enough, he wasn't let go for incompetence.
It really seems to be the trend (at least that I've seen). The incompetent programmers work at the same place for more than a few years while the good ones move around and inherit the atrocities that these "career" guys build. I cringe every time one of the guys from management comes over to see what we're doing and starts making suggestions. It's like "please oh please don't make me do that..."
"The shortest distance between two points is under construction"
-Noelie ALtito
|
|
|
|
|
AeonBlue wrote: It really seems to be the trend (at least that I've seen). The incompetent programmers work at the same place for more than a few years while the good ones move around and inherit the atrocities that these "career" guys build. I cringe every time one of the guys from management comes over to see what we're doing and starts making suggestions. It's like "please oh please don't make me do that..."
But, just once in a blue moon, a great team comes together and a bunch of great people churn productively for years. It's a rare mythical beasty to be sure. But it does happen now and then.
I'm largely language agnostic
After a while they all bug me
|
|
|
|
|
I'd insult the guy, but i made some terrible code when i was just starting.. (actually i'm still new, but my code has improved significantly.. still has a ways to go though, will never be perfect) so I wouldn't completely throw away the idea that he's a bad teacher now. Hell i'd bet he learned a lot from writing that crappy app looks like he had the opportunity to make plenty of mistakes.
Einstein argued that there must be simplified explanations of nature, because God is not capricious or arbitrary. No such faith comforts the software engineer.
-Fred Brooks
|
|
|
|
|
I failed to mention that this application was developed over the past 4 years by this guy. He is 50+ years old and is a part-time ITT Tech teacher; a job that he was doing while he worked here and I think still is doing. I don't know what he teaches but I think he has a CS degree.
I don't know how many years experience he has in the industry. It could be that this was his first C++ app though. He obviously is or was a C programmer at some point in time.
This week I removed 20,000+ lines of redundant and unnecessary code from the program. CODE, not comments. And I am just getting started. I have to add functionality to it now.
I spend half my time writing new C# code and the other half maintaining the legacy stuff. It makes me cry every time I close Visual Studio 2008 and open Borland Studio 2006 .
|
|
|
|
|
Any chance of being able to pull the Borland libraries into VS2k8, or does the app take excess advantage of Borlands proprietary language extensions?
Today's lesson is brought to you by the word "niggardly". Remember kids, don't attribute to racism what can be explained by Scandinavian language roots.
-- Robert Royall
|
|
|
|
|
"He wasn't let go for incompetance."
Try explaining why all the stuff you laid out is incompetent to labour courts. You also find that most of these lecturers really don't have a clue of real-world stuff (most, not all). One of my lectures recently wrote a book on design patterns in C#: she made an entire chapter on the observer pattern. Yep, freaken observer pattern in C# - I'll stick to events thank you very much and save myself a few classes every time I want to handle a click event. And she was the head of department. And the book was about C# 3.0 (you would think they would catch a wiff of the multicast delegate by that stage!).
By the way, a lot of the more influential folks in CS are giving universities a wrap lately (don't have any specific references.) for pushing out incompetant graduates: I left my first university after getting 70% plus for all my CS subjects without ever attending classes or studying (guess where that lecturer lectured?).
He who asks a question is a fool for five minutes. He who does not ask a question remains a fool forever. [Chineese Proverb]
Jonathan C Dickinson (C# Software Engineer)
|
|
|
|
|
Wow! Now that is a real coding horror! Five.
To me it sounds like an in-house app. I would really consider migrating the existing database to standard XML format, to be able to use a standard parser.
maizhiming wrote: There isn't a single pass by reference in the entire application. It is all pointers.
Also, like the guy above, I do not get the big problem od refrences vs. pointers. Of course const references are the most proper way in many cases, but IMO using pointers is not a horror, compared to the rest of the stuff you mention.
_____________________________________
Action without thought is not action
Action without emotion is not life
|
|
|
|
|
LOL Well, I graduated from ITT Tech in Seattle, WA in 1997. Hopefully, the code you're talking about above did not come from anyone tought by my professor. He was very strict on proper code practice, and placing things where they truly belong. However, I seemed to be the only guy there, besides my professor, who seemed to get it, so your post doesn't surprise me in the slightest.
Kyosa Jamie Nordmeyer - Taekwondo Yi (2nd) Dan
Portland, Oregon, USA
|
|
|
|
|
At my last job I had to work a such a mess except it was in VB6. The original programmer was clueless. It had Global variables all over the place. There were no arguments passed to any methods ever! Everything was a variable in a larger scope. And this was the core software for the entire business. 100 people used this every day.
I remember one section of code that had to import a bunch of records. It read a file line by line and inserted the records into the table with SQL (of course it didn't reference the columns so you can never add any columns to the table). On top of that it execute a select count(*) from tbl after each insert to display the count on the screen. It would take forever and the users would be complaining about the slow speed of the program.
I started looking for another job on my first day of work because I got 40 emails about problems with the program. Fortunately I was only there for about 6 weeks. What's funny is that I purposely did not put VB6 anywhere on my resume, not did I mention it in the interview because I HATE programming VB6. Yet they expected me to work on a VB6 application.
I didn't get any requirements for the signature
|
|
|
|
|
I used to work at a place where many of those things were done and no one saw anything wrong with it.
I switched to a better job.
Bill W
|
|
|
|
|
|
Any database table with a 1,000 columns should be flagged for restructuring. That is indeed a horror.
"The clue train passed his station without stopping." - John Simmons / outlaw programmer
"Real programmers just throw a bunch of 1s and 0s at the computer to see what sticks" - Pete O'Hanlon
|
|
|
|
|
Any database table with a 1,000 columns should be flagged for restructuring deletion.
Simon
|
|
|
|