Programmatic Malpractice

Published Wed, Jan 31 2007 22:23 | William

In the category of "I couldn't make something like this up even if I tried" comes the following code.  Rather than try to dissect what it is that's wrong with it, it's a lot easier to find what's right with it.  If the person that wrote this cashes their paycheck, they should be charged with theft.  We have all been in a hurry and written code that we aren't proud of. We all are guilty of oversights.  But this was done by a self proclaimed guru and expert, and when you read the comments that were attached to it, it's clear that this was done intentionally.  So I'm opening the floor to pointing out what's wrong with it, or for my really advanced readers, what's right with it (God knows a lot can go wrong calling the AddNew method of a DataTable and wrapping such a dangerous operation in catch block catching System.Exception is absolutely critical):  Read the code first, then read the comments associated with it.  P.S. I'm warning you up front that I'm not responsible if you start barfing, become overwhelmed by the ugliness of it and gouge your eyes out as solace or if your brains succumbs to spontaneous combustion.  Yep, a 'professional consultant' wrote this work of art.  As a consultant, I guess I should find comfort in stuff like this b/c as long as there are people like this committing malpractice, I mean working, then I'll always have a job. On the other hand, this sort of stuff is so bad and so inexcusably terrible , that my heart goes out to this person's victim, urr, I mean client.

 

     public static decimal EvalNumber(string strExpr, ref bool
calcSuccessful, ref string NegativeDelim)
       {
           calcSuccessful=false;
           NegativeDelim = "-";
           DataTable dt = new DataTable();
           try
           {
               dt.Columns.Add("Expr", typeof(Decimal), strExpr);
           }
           catch
           {
               // tough decision they entered a error
               // colud be as simple as text/words
               // could be division by zero
               return (0);
           }

           try
           {
               DataRow dr; dr = dt.NewRow();
               dt.Rows.Add(dr);
           }
           catch
           {
               return (0);
           }

           decimal TempReturn=0;
           try
           {
               TempReturn=Convert.ToDecimal(dt.Rows[0]["Expr"]);
               calcSuccessful=true;
           }
           catch
           {
               calcSuccessful=false;
           }

           // expressions like (9) will resolve as positive 9 which is wrong
           // expressions like (9+2*12)*20 will obviously be a real formula
           if (calcSuccessful == true && strExpr.IndexOf("(") == 0)
           {
               string tempStr = strExpr;
               tempStr = tempStr.Replace("(", "");
               tempStr = tempStr.Replace(")", "");
               // parentheses are stripped

               decimal tempNum = 0;
               try
               {
                   tempNum = Convert.ToDecimal(tempStr);
                   // (9) == 9 so we must turn it to -9
                   if (tempNum == TempReturn)
                   {
                       TempReturn = TempReturn * -1;
                       NegativeDelim = "(";
                   }
               }
               catch
               {
                   // nothing to do, all is well
               }
           } // end if parentheses issues

           return (TempReturn);

       } // end EvalNumber



  private void ITDataGridView_CellParsing(object s,
DataGridViewCellParsingEventArgs e)
       {
           string tempMsg = "ITDataGridViewFund_CellParsing";
           Logger.DGVStatusGrid(tempMsg, s, e, this);

           if (CellTypeCalc(e.RowIndex, e.ColumnIndex, this) == false)
               return;

           // we are in a row that allows formulas
           bool evalSucceeded = false;
           string negativeDelim = "";
           decimal decMoney = 0;
           string CellValue =
this.Rows[e.RowIndex].Cells[e.ColumnIndex].EditedFormattedValue.ToString();
           decMoney = Utils.EvalNumber(CellValue, ref evalSucceeded,
ref negativeDelim);
           if (evalSucceeded)
           {

               // set format based on ( or -
               if (negativeDelim == "(")

this.Rows[e.RowIndex].Cells[e.ColumnIndex].Style.Format =
"#,##0;(#,##0)"; ;
               if (negativeDelim == "-")

this.Rows[e.RowIndex].Cells[e.ColumnIndex].Style.Format =
"#,##0;-#,##0";
           }

      } // end ITDataGridView_CellParsing

 

-----------------------------

Quote from the Guru that wrote this:

I tried every event to set .Value to the result of that formula. I
tried dozens of desperate acts including just trying to SendKeys the
Calc result to the cell (even that hack is impossible to get to work).
Despite printing dozens of articles from Google and reading and
empirically experimenting, days of frustrating no answer were my fate.
But its ok, they had not resolved this in months and were going to say
"no" to that user requirement before they handed it to me. They had
made a pretty good app, otherwise that is a 1:1 knockoff functionality
wise of another .net app written in 1 C# file with the Infragistics
grid and spaghetti code everywhere, extraneous code, ad-hoc SQL, etc.
whereas they wrote a fresh code base in 2 dozen sanely organized files
and using Sprocs instead. They just needed a few bugs and enhancements
fixed, and the code refactored in places for the new code and they
were quite burn out on taming the DataGridView with very little,
incomplete and confusing and contradictory docs whereas I am not burnt
out.

The two things that maybe could help but the docs/Google searches were
as clear as mud i.e.
e.FormattingApplied=true
e.ParsingApplied=true
seemed to not work well. They are normally false but if set to True
they tended to create recursive DGV events in an infinite loop and
overflow the stack. Ouch!

Finally the answer turns out to be to use
_CellParsing

and VERY IMPORTANT use these 2 lines:

e.Value = newValue;
e.ParsingApplied = true;

and never do:
this.Rows[e.RowIndex].Cells[e.ColumnIndex].Value = newValue;
e.ParsingApplied = true;

even though its basically the same thing because DataError will fire
and recursion will occur etc.

The users had 2 needs that sound simple but are quite tricky...

1) a cell that understands a formula
2) that cell if you enter -33 it should format as -33
   if you enter (33) it should format as (33)
   (ouch harder than it sounds)

and the Eval function I use i.e. is crude and convoluted because of
requirement 2 and the fact that when it works as designed (33)
resolves to +33 if one does not compensate since it considers
parentheses to be an algebraic operator i.e. to make (2+3)*9  resolve
without the multiplication occurring first via the order of operation
rules. I probably will redo the EvalNumber function soon to ve cleaner
and maybe with generics to support more numeric types than decimal
(int, double, etc.) but this app only needs decimal.

UPDATE: You've heard the adage 'the customer is always right' haven't you?  Well, I guess the developer that wrote this has his own version "If the customer doesn't complain, then the code doesn't suck"  Hysterically, since the customer was ok with the end result, it's not bad code.  I guess you couldn't write code that met the customer's needs that wasn't so lame?  The fact is, that a lot of code problems don't show themselves at first.  It's only after stuff doesn't scale or port well, that people realize it's crap. Trust me dude, you could have written better code that they would have been ok with and rationalize it however you like, people bragging about being gurus shouldn't start writing stuff like this. 

Filed under: ,

Comments

# Blogus Maximus said on January 31, 2007 10:00 PM:

Dear god... my eyes... my eyes... I'm blind.

Is this real? This can't be real. Tell me it's not real.

# William said on January 31, 2007 10:15 PM:

Not only is it real, but it's a decently known company. These poor SOB's actually paid money for this. It's kind of like paying money for someone to give you ebola

# Jason S. Burton said on January 31, 2007 10:58 PM:

Unreal.  Have you submitted this to the Daily WTF?  That reminds me of a 26 item Case statement I once saw that went something like this:

CASE 'A' THEN somefunction('A')

CASE 'B' THEN somefunction('B')

CASE 'C' THEN somefunction('C')

...

...

CASE 'Z' THEN somefunction('Z')

Some programmers must get paid per line...

--jason

# Ryan Olshan said on January 31, 2007 11:07 PM:

Wow. This is...I'm speechless. If this person worked for me, I'd fire them. Wow. Just wow.

Ryan

# said on January 31, 2007 11:17 PM:

If Fox created a special called When Bad Code Attacks , I think this would be in the first episode: http://msmvps.com/blogs/williamryan/archive/2007/02/01/programmatic-malpractice.asp

# Ron P. said on February 1, 2007 12:33 AM:

Bill - what the hell is a matter with you.  Posting stuff like this on a Community Oriented site is totally inappropriate <chuckle>  I just lost 10 IQ points reading that crap.  This is one post that your boy Summer's Eve Nelson SHOULD complain about b/c this sort of stuff hurts the community. It looks like you are intentionally trying to embarass Anders Heljsberg by posting examples of the most retarded way you can use his beautiful language.

So if I know you, this isn't a random post.  You are trying to ridicule someone.  So met me see.  Whos code would bill be posting ? Would bill be making fun of some newbie?  Nah not his style. Would bill be posting a coworkers code?  Nah. So whose code could this be.  I'm guessing either Massengil Nelson or Mark www.charlescarroll.com Brown? Fess up dawg. You cant just tease us like this.

# Ron P said on February 1, 2007 12:43 AM:

You evil SOB, you are talking about http://www.charlescarroll.com aren't you?  If he trains as bad as he codes I'm just glad I never took one of dude's classes.  Cuz that sh1t is like school on Sunday man.  At least homeboy uses try/catch blocks right http://msdn2.microsoft.com/en-us/library/system.data.datatable.newrow.aspx

# Jeremy Brayton said on February 1, 2007 2:28 AM:

This seems like a case of trial-and-error coding. Sadly, I can relate.

SalesLogix uses VBScript and has stock code that queries Err for any errors returned and then prints an error message. The problem with this is it isn't specific on when the error occured so it could be right before the function call or on line 1 of a 800 line routine. To trap where specific errors were I would liter code with these calls and I could pinpoint roughly where an exception would occur. Usually I'd fix what triggers the error and remove the check but it's usually not a bad idea just to keep them in. This code would almost be the C# equivalent of some of my work if I ported it straight.

I can see this person in testing trying different scenarios, running into multiple exceptions then encapsulating the error in a try/catch statement as they go. Each try/catch roughly handles one problem and moves on. I tend to think about them first and group error prone code in one big try/catch statement that roughly does the same thing. DB manipulation deserves one, straight math deserves another, etc. There are exceptions to this rule, like when one particular block throws every exception known to man but it's generally a forthought, not after-. What I find funny is the Logger.* call that apparently signals to a logger that the code is doing something but you don't see the log reporting any exceptions. Why even use a logger if you can't report meaningful stuff along the way?

# Brian H. Madsen said on February 1, 2007 7:35 AM:

without knowing the circumstances (and the experience level of the guy/girl doing the work) it's hard to relate...

ok - that was the GOOD deed of the day...i'm shocked to see such badly written code - but unfortunately it's very common these days..what i'm disgusted by is the lack of professionalism in the comment section. basically sounds like the developer just "tried" to accomodate the client with a hack and justifying his/her own lack of skill.

other than that, why no regular expressions was used and ffs - refactor some of that stuff out..Jeremy mentions the Logger.*...well - waste of bytes if you ask me.

i may not be a perfect coder myself - but at least i try and this is just hogwash.

# Summers Eve Nelson said on February 1, 2007 8:48 AM:

There's just so many things wrong with that code I don't know where to begin. If I was to point out what he did correct, all I could say is he or she knows how to use curley braces correctly. What really makes me laugh is if (CellTypeCalc(e.RowIndex, e.ColumnIndex, this) == false) as it should be if (!CellTypeCalc(e.RowIndex, e.ColumnIndex, this)). But, that's one of the many flaws of this code. The use of so many try/catches makes me want to vomit. Not to mention the horible naming standard. This is definately not the work of a guru. A beginner, yes. A guru, no. It's people like this that rip of companies that make me mad. I think I had an epileptic seizure while reading this code.

# Bill said on February 1, 2007 9:18 AM:

Brian - I don't write perfect code myself and have written a lot of stuff in a hurry that I would rather disavow. However the person that wrote this code is a consultant and a self-styled expert. This person brags incessantly about their coding prowess. The funny thing about it is that a year or two ago, I posted about this person's code and pointed out almost verbatim these terrible habits. They said I was nuts, making it up, etc. Dude, this is production code. This is their real solution. And all I'm saying is that if you code like this, you shouldn't hold yourself out as an expert, guru, boss or anything else and you damn sure ought not be training people. If this was a blog post about something, or some demo code you rolled off the top of your head, it'd be a lot more understandable but nope, this is production code.

# Bill said on February 1, 2007 9:24 AM:

Just as a reminder, I do not censor comments. The only comments I'll edit or delete are spam. With that in mind, there is a major crybaby or two that reads my blog and throws temper tantrums every time he reads something he doesn't like. Without exception the things he doesn't like happens to be comments making fun of him or his master. So, although I wholeheartedly concur about the comments here, I'd prefer that less brash names and language are used. So I'm a little torn b/c I think it is incumbent upon professionals to disavow code of this sort, as well as frauds that hold themselves out as experts and put out garbage like this. I think it gives consultants a bad rap - not to mention that MS bashers often seize upon terrible code like this whenever it gets hacked or blows up, and tries to blame Microsoft for it. So if you're reading this, please understand two things - 1) There's some harsh language here by some of my commentors. To that end, I apologize but I just don't have time to sit around and edit out comments I find unsavory. it's hard enough just managing spam. 2) Code like this is a crime. No developer with an ounce of skill, self respect or integrity would pump crap like this out and charge you for it.

# Sahil Malik said on February 1, 2007 9:33 AM:

This code is rife of beginner mistakes. Looks like the work of someone who hasn't had too much experience in actual coding - typical of a fresh out of school guy in his first job. I think with time, and enough mentorship, he may just be able to write good code, one day.

Bill - is this someone you are helping lurn .NET?

# William said on February 1, 2007 10:10 AM:

Sahil - I agree with your assessment but this person isn't fresh out of school b/c he didn't go to school. However I know a few programmers that are totally self taught that are total bad a55e5. In order to write code this bad, you need to have a certain level of arrogance and think you know what you're doing. Newbieness has more randomness to the mistakes - this garbage is intentional. In school we get certain things beat into your head that after the first semester you know never to do - like having 25 exit points in a function. This code illustrates that the person has no idea about design or implementation and even less of a clue about the framework. In many countries, they'd cut off your right hand if you were caught coding this stuff It's criminal to think that the person that wrote this not only claims to be a professional but a guru. Yes, they clearly Lurned .Net.

# Frans Bouma said on February 1, 2007 11:30 AM:

Bill, why did you post my code on your blog?

;) :P

This is indeed horrible crap, however, as I read the DailyWTF daily, I know that out there, a large group of high-payed consultants fill their pockets with cruft like this every darn day...

The sad thing is... in our lives, every day we use services which are build on drivel like this...

# Kevin Jones said on February 1, 2007 12:26 PM:

I am confused. What the hell is this code actually doing? Is it suppose to give headaches?

I feel a lot better about my job security now.

# Roland Boon said on February 1, 2007 12:27 PM:

Okay, being 14 hour late, I need to appologize, and then get on with it, and write my name on this mirror/wall.

You see, the reason is that I've been partying a bit; for the [x] time, after being blinded for far too long, I'm once again happy not to be by myself.

Please hold on; subject in question is preparing supper.

# Roland Boon said on February 1, 2007 12:41 PM:

I like it.

# Andy said on February 1, 2007 3:29 PM:

You know what this kind of code is good for? When you are on a board and some newby asks you "how do I do x" you send them a PM with that code and say "well I didn't want to show this in a public forum but this is the correct way to do it" and see if they are dumb enough to use it at their company :)

# Stan Woods said on February 1, 2007 3:31 PM:

I'm confused.  You say this is production code, but whoever wrote this and the comments you posted -- after the code, in the last paragraph -- openly admits that this is ugly, and then goes on to say how he/she is going to clean it up.

Crap code is crap code, but let's not get all ZDNet-sensational here just to make a story more exciting.  I get enough chain letters like that in my inbox every day.

# Bill said on February 2, 2007 12:51 PM:

Stan:

Well, I appreciate your comments but I'm not sure I'm reading the same comments as you.  This code was posted as a solution to the problem and of the things he mentions he's going to clean up, well, he doesn't mention the truly heinous aspects of it. Moreoever, just look at it carefully.  Some of the most serious problems are just ones that have utterly no place being there at all. An assessment was made that this code looks like that of a complete beginniner or someone with little to no understanding of my many features are supposed to be used.  

When I read the comments, I see an admission that someone spent dayS on this.  And that 'it's ok' b/c the original developers spent months and couldn't get it to work.  To be completely candid, if code is as whacked as this, and you are a billable consultant, you owe it to your client to fix it in a timely manner. I understand not rewriting an application and working with what you have, but this is  very simple functionality (at least the correct solution should be - there's nothing 'simple' about the way this code was implemented.

I honestly wasn't trying to be sensational here or make the story more exciting.  From my POV, this is code written by a self-proclaimed guru and this sort of code is completely unacceptable for a billable consutlant to implement.  The guy berates other spaghetti code but if this isn't spaghetti, I don't know what is.   If you were paying for this stuff by the hour, would you be ok with this being the result of multiple days (I wouldn't even be ok with paying multiple hours)?

I personally don't mind my code that I bill for being subject to rigorous examination. Do I make mistakes? Yes. Have I written stuff in a hurry before? Yes.  Have I written code that I thought was ok then realized it was garbage? Sure.  To that end, I really don't mind someone pointing it out. I've had people take issue with some of my demo code and in each case, my response has been "Point taken". I'm willing to accept criticism for demo code I post to a web site for free, so I'm *** sure willing to hold myself to an even higher standard on stuff I bill for. And  were I to write something like this, I would feel obligated to fix the stuff on my own time if I had billed for it in the first place. The client shouldn't suffer b/c of my lack of knowledge/understand or the fact I may have been in a hurry.

Your point about sensationalism is duly noted, but I'm going to respectfully disagree in that I think consultants have a bad rap b/c of people who do stuff like this and bill for it.  And I think drawing clears lines in this regard is entirely appropriate.

One last thing - I don't understand your comment about chain letters.  Are you talking about chain letter in general - or about Coding WTF emails?  I've never recieved such an email. If you actually get Coding WTF emails - I'd honestly be interested in seeing what such a thing looks like, would you mind forwarding me a few? (I know you probably have better stuff to do with your time then forward some guy you don't know emails but if you have a few extra seconds, I'd be appreciative of it)

Thanks again

Bill

# William said on February 2, 2007 9:36 PM:

Hi Stan:

I don't think I've seen your name here before so welcome aboard.   I understand your confusion but I think you may be a little off base.  I grabbed this code from a mailing list and didn't post all the information about it to, well, protect the innocent so to speak.  For one thing, the title of email was "Yee haa DataGridView horrible challenge finally resolved.  One of the reasons I posted it was b/c of the arrogance of the guy writing it. I believe he mentioned it in this snippet, but in the full email he also emphasizes that his coworkers were unable to solve this problem.  It's impossible to know for sure what's in someone else's mind, but the verbiage of the thing is definitely condescending to the other developers and juxtaposes it with  a "look how smart I am b/c I figured it out" tone.

So this was real production code.  This was posted as a solution to a problem.  Giving the guy the benefit of the doubt to the point of being pollyana, the question that begs to be asked is "if you're posting this as a solution and it's really not how you intended to code it, why *all* of the glaring mistakes".  I guess you can read the statements as an admission about this particular class but to be honest, it really doesn't look that way to me.  The reference to other spaghetti code for instance is kinda funny b/c if this itself isn't the epitome of spaghetti code, I don't know what is.  

The statement "I probably will redo the EvalNumber function soon to ve cleaner and maybe with generics to support more numeric types than decimal

(int, double, etc.) but this app only needs decimal.

" - surely doesn't seem to back up your assertion.  If it said "Now that I got the Parse issue sorted out, I can clean up the rest of this stuff that needs a lot of work..." - I'd expect something of that sort at a minimum.  Generics are singled out as an improvement and he states he'll 'probably' redo it.  The real problems with this code are far and deep and enhancing flexibility with Generics is probably Item 20 on any list of fixes.  All in all, I don't see anything to indicate that this code was something he deemed terribly incorrect.

You raise a point that has me intrigued though.  When you mention Chain Letters - are those code related chains or are you referring to chain emails in general that try to sensationalize that which is otherwise dull?  Either way I get your point, but if you happen to be referring to code related chains - I'd be very appreciative if you could forward me a few of these  for I've never seen one

Anyway Stan, glad to have you around and I'll take your comments to heart.  Thanks again man.

Bill

# That guy you know in Redmond from the place said on February 3, 2007 12:30 AM:

So did this code come from Charles "Still the undisputed champion of Classic ASP" or was this from "Punksaphony" Phil?

# Bill said on February 3, 2007 12:58 AM:

Punksaphony Phil - Ouch.  Be fair, the arteries and veins in Phil's head would have exploded if he saw code like this

.

There can be only one that writes code like this

# Bill said on February 3, 2007 1:05 AM:

Frans:

Sadly, you are correct (aren't you always ;-)  ).  This expert grates my nerves pretty bad, but this code is all too common.  You just don't expect it when it comes from Self Proclaimed Gurus

Are you going to jam Smoke on the Water at the Summit?

# Charles Mark Carroll Blog » Blog Archive » With even minor fame comes major infamy said on October 28, 2007 8:09 AM:

Pingback from  Charles Mark Carroll Blog  &raquo; Blog Archive   &raquo; With even minor fame comes major infamy

Search

This Blog

Tags

Community

Archives

News

My other sites

Cool Stuff

Book Stuff

Security

ORM

Data Access

Funny Stuff

Compact Framework Stuff

Web Casts

My KnowledgeBase Articles

My MVP Profile

Design Patterns

Performance

Debugging

Remoting

My Fellow Authors

My Books

LINQ

Misc

Speech

Syndication

Email Notifications