November 11, 2011 at 9:56 am
Comments posted to this topic are about the item Don't Criticize Code
Best wishes,
Phil Factor
November 11, 2011 at 12:37 pm
I'm going to have to disagree with you on this one.
There is definitely some code out there that deserves criticism.
I look at some of the database designs and stored procedures I wrote a few years ago, and I cringe. Definitely worthy of criticism, by me or anyone else.
My predecessor at my current job left behind a mess that was doing things like losing 16.7% of the orders placed for materials on our websites. (Lost 5 minutes of data twice an hour = 10 out of every 60 minutes ~= 16.7%.) This was not due to the pressure to design the thing, lack of training (degree in Comp Sci), experience (over 20 years), or anything other than not paying attention to the details of the project. Same developer also lost an unknown percentage of data, because of the way it handled inserts into sub-tables after inserting into a parent table. Instead of using Scope_Identity, or some other common method (could have used Output, it's SQL 2005, but I can easily forgive not knowing about that one), the code instead queried the table it had just inserted into for the ID of a row with the same e-mail as the input parameter value @email. One person with a blank e-mail got all the data for everyone who ever put in a blank e-mail, and the whole process would fail and rollback without raising an error if the @email value was null. E-mail was NOT a required field on the websites, nor required by business rules.
There's an ETL process that's so complex the original author would take 2-3 days to find out what was going on any time a row of data didn't transfer correctly. To move data without non-trivial transformation from one server to another. (The only transformations were renaming columns and tables.) I can't even begin to describe how this process works, because it would take a book to do so. It takes 4 SQL Agent jobs, three Replication publications, and dozens of stored procedures, all using dynamic SQL. The shortest stored procedure is over 1000 lines of code. I'm in the middle of replacing it with a single stored procedure that just uses Merge (for the 2008 targets) and 2-step upserts for 2005 targets. Troubleshooting takes seconds now, instead of days.
There were replication jobs that pushed data from one server to another, then pulled the data back and overwrote the original, completely eliminating all user input at both ends. Those I fixed a year ago.
I replaced a 2000-line stored procedure with pages of dynamic SQL in it, with a single Insert Select statement, just last week. It processes the data correctly, the business users are thrilled that it's not losing data any more and that the results from it are verifiably correct, and it runs in a few milliseconds instead of several minutes.
That's just the surface of it. There's a LOT more where that came from. And did I mention the only documentation he left behind was 2 Visio flowcharts, one of which was just plain wrong (servers that had never existed were on it), and the other consisted of two servers with an arrow between them from one to the other, and nothing else.
The only good thing is there's years left of work for me just to bring tables and processes up to at least the kind of standards I'd expect from a junior DBA with a couple of years of experience, much less up to any sort of expert level. That's only good because it pretty much guarantees employment. I'm good at what I do, but this guy is making me look like some sort of hyper-genius, to the managers and my co-workers.
I'm told the guy left because he wanted to get back into what he really enjoys, which is writing software for offshore banks. (Yeah, he writes banking software. That scares me too.)
I never walked in his shoes, so I prefer not to criticize him. There may have been valid, to him, reasons to build the Rube Goldberg machines he built, and there may have been reasons to avoid documenting incredibly complex, business-critical processes, databases, et al. I'll never know. (Note: I try not to criticize him. I'm not sure I succeed all the time, but I do try.)
But I'm definitely going to be critical of the code. And then fix it. And then be critical of my own code when I review it next year or whenever. Otherwise, it'll never improve.
- Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
Property of The Thread
"Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon
November 11, 2011 at 1:03 pm
Criticize the code, and train the developer. Constructively criticize the person who wrote the code and see if they improve. Otherwise, how else will they ever get better at it?
I think it is good to constantly try to code better.
Jason...AKA CirqueDeSQLeil
_______________________________________________
I have given a name to my pain...MCM SQL Server, MVP
SQL RNNR
Posting Performance Based Questions - Gail Shaw[/url]
Learn Extended Events
November 11, 2011 at 1:46 pm
As a consultant I have learned not to crtitise any legacy code that I see (new code from someone in my team is a different story) as quite often the person that wrote the code is sitting nearby, or even possibly the person that employed me for the contract.
November 11, 2011 at 2:23 pm
In MHO there are two types of criticism, constructive and its opposite, destructive and depending upon the circumstances one must select which to use.
In the case of poorly written code, if I have an idea as to how to improve the code, then the constructive technique is what I have used. Saying things like “have you tried and tested yyy ?”. If the persons answer is “no I have not” I then follow up with the suggestion, “Please do so and let us compare the results of each” If they have tested my suggestion, again I request a comparison of the results, and in some cases I have learned something and I will compliment them for that.
As a team lead or supervisor I have always insisted on proper documentation, starting with the objectives, and equally important IMHO is what the code SHOULD NOT DO. Failure to have the required documentation as a first time mistake, my criticism is constructive. If it is a habit of an individual not to produce any documentation the criticism is by anyone’s opinion definitely destructive, for the individual is violating a standard requirement, and testing my authority as his/her supervisor.
In the few instances where I have been an outside consultant, I treat the lack of documentation following a constructive pattern, by explaining to those who employed me how proper documentation would have reduced the time I have spent correcting deficiencies and hence their cost. Seems like everyone from the lowest denomination, to the highest supervisor understands the almighty dollar and the need to reduce unnecessary expenditures.
So like all things involving any coding/database projects the “It depends” mantra is always present.
November 12, 2011 at 3:14 am
Yes, of course we need to improve the code around us, but it can be done without criticism, in the sense of disparaging the code and the author. As any skilled teacher knows, rewarding and praising good code and giving incentives works well in any Dev team. Criticising code causes havoc, and damages relations. Even when the author has long gone, it isn't an effective way of making progress. It is much better to demonstrate how code can be improved without criticizing code. I know this can be hard to do because intuition often tells you that criticism works, but that's probably because you experienced poor teaching at school and college or Uni.
I once wrote an article about positive approaches of changing people's behavior at work, with the catch, or twist, being that I wrote it about the team members turning their manager into a better and happier leader of the team by simple positive techniques.
On Training Your IT Manager[/url]
Best wishes,
Phil Factor
November 12, 2011 at 3:19 am
But then what would they publish on TheDailyWTF.com? 😀
-- Stephen Cook
November 12, 2011 at 6:57 am
Our large database development team performs peer code reviews using a code review tool (http://smartbear.com/products/development-tools/code-review/). When we first started the process, it surprised me that some developers took defects and constructive suggestions for improvement as a personal attack on their competency. Most, however, understood the purpose of the review was to ensure adherence to standards and improve quality rather than public humiliation. It has taken some time but most developers have now learned the art of teamwork in reviewing code. Each developer is an individual who needs to be treated with respect and sensitivity with regards to their unique personality and how they take constructive criticism.
Not coincidently, the best developers tend to be the ones that embrace suggestions for improvement within project time constraints instead of trying to justify flawed code.
November 12, 2011 at 7:33 am
Stephen E. Cook (11/12/2011)
But then what would they publish on TheDailyWTF.com? 😀
As much as I hate to admit it, I've had the dubious honor of having my code showcased on the site (WTF at the time). It was VB3 code I wrote circa 1993 for an in-house tool when I was a DBA. VB was far from my core competency so I displayed an error message box to prevent an out-of-bounds array condition instead of simply expanding the length with REDIM. My method was better than a run-time exception but clearly not the best way to address the issue.
I'm rather thick-skinned so I would have welcomed a better suggestion, even if done in a disparaging way. The cure for ignorance is knowledge.
November 12, 2011 at 9:35 am
I often look back at code that I've written (sometimes very recently) and cringe, because I know it could be better. However, as I'm sure all of us do, we do the best job we can with the knowledge we have at the time and within the time/budget constraints of the project. Our knowledge is (or should be) continuously growing, so of course we know better ways to do it now versus then. Phil is correct - when you're looking at someone else's code, you have no idea what happened at the time to help it get to its current state. I have a small, simple project that had a good db design and decent code. Until the customers decided they wanted significant changes that really didn't fit well with the original design, but would not go for a redesign...
There is definitely a difference between constructive criticism and just plain criticism. Rather than discussing how horrible something is, it is a good opportunity to say "we know different and/or more effective ways to do this now."
And before you try to say that if the budget/time constraints don't allow you to do a good job, then you shouldn't do it at all, let me just remind you that some of us have families to feed and cannot turn customers away simply because they can't afford to pay for the latest & greatest that technology has to offer. Granted, the latest is typically easier/quicker than the old way of doing it, but sometimes you can help yourself by taking code from those older projects. And then you just hope that at some point you'll have time to refactor...
November 12, 2011 at 9:38 am
It's one thing to not spout off on code as a consultant, as steveb mentioned. But to argue against criticizing code at all smacks of the "we are all winners, everyone's idea is as good as any other" school of thought that is the enemy of quality.
If you want to talk about humility, I think there is a need for humility on the code writer's part. I've run into MANY developers who code in Frankenstein's Lab, keeping their code secret because they do not want external criticism of their code before it's set in deadline-driven stone.
What is needed is for developers to have the humility to put their rough sketches of a code design before their peers BEFORE it's written and say, "This is what I'm thinking, see if you can punch holes in it or think of a better way of doing it."
Because at the end of the day, the goal should be to develop the best code/design possible, not to be the sole originator of the idea.
So sorry, if someone comes out of Frankenstein's Lab with a piece of junk code, I'm going to call it out. I recently ran into this situation and was able to rewrite the code in 5 hours, leading to an 80% decrease in data access and runtime.
Rereading the article, it's clear that the author is coming from a consultant's viewpoint. And I think that's a really important detail: You have to be tactful as a consultant. But you can still criticize tactfully. If the original coder is available, they can explain valid reasons for the tortured approach which will help inform your work. If they were just a bad coder (or inexperienced at the time the code was written), the discussion can validate trying to refactor the code into shape, while at the same time educating the coder on a better design.
November 12, 2011 at 10:09 am
Reminds me of the time, back in 1993 (in a world far, far away, in a different programming language), of having to figure out a program in which the author used the variable names a, b, c, d, e,... s, t - literally! Took me a LONG time to figure out what he was doing. I did know who the programmer was, and he had left the company, and all I really cared about was trying to reproduce the results for a client. (The programmer was well-respected, and I wasn't going to dis him.)
(I did rename all those variables and added comments by the time I was done.)
November 12, 2011 at 11:02 am
i totally disagree with your observation. if the code is poorly written no matter what the circumstances were, there is no point in muddling through bad code to fix it. you will simply be wasting time fixing in one place and breaking it somewhere else. and also bad code is not testable. if you are given the task of maintaining such horrendous code, take the time to reverse engineer and re-factor and re-write the piece which needs immediate attention from inside out and at the same time make sure that the new code is testable too. if the new code is not testable a unit at a time that means you still have a "code smell".
November 13, 2011 at 7:56 am
Most idiotic editorial I've ever seen.
November 13, 2011 at 10:15 am
Ouch, it's starting to get ugly here...
Viewing 15 posts - 1 through 15 (of 53 total)
You must be logged in to reply to this topic. Login to reply