Click here to monitor SSC
SQLServerCentral is supported by Red Gate Software Ltd.
 
Log in  ::  Register  ::  Not logged in
 
 
 
        
Home       Members    Calendar    Who's On


Add to briefcase 12345»»»

Don't Criticize Code Expand / Collapse
Author
Message
Posted Friday, November 11, 2011 9:56 AM


Mr or Mrs. 500

Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500

Group: General Forum Members
Last Login: 2 days ago @ 3:19 AM
Points: 577, Visits: 2,503
Comments posted to this topic are about the item Don't Criticize Code


Best wishes,

Phil Factor
Simple Talk
Post #1204334
Posted Friday, November 11, 2011 12:37 PM


SSCoach

SSCoachSSCoachSSCoachSSCoachSSCoachSSCoachSSCoachSSCoachSSCoachSSCoachSSCoach

Group: General Forum Members
Last Login: Friday, June 27, 2014 12:43 PM
Points: 15,444, Visits: 9,596
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
Post #1204458
Posted Friday, November 11, 2011 1:03 PM


SSC-Insane

SSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-Insane

Group: General Forum Members
Last Login: Today @ 7:37 PM
Points: 21,215, Visits: 14,917
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


SQL RNNR

Posting Performance Based Questions - Gail Shaw
Posting Data Etiquette - Jeff Moden
Hidden RBAR - Jeff Moden
VLFs and the Tran Log - Kimberly Tripp
Post #1204471
Posted Friday, November 11, 2011 1:46 PM
SSCrazy

SSCrazySSCrazySSCrazySSCrazySSCrazySSCrazySSCrazySSCrazy

Group: General Forum Members
Last Login: Friday, May 30, 2014 6:27 PM
Points: 2,808, Visits: 7,175
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.
Post #1204485
Posted Friday, November 11, 2011 2:23 PM


SSCertifiable

SSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiableSSCertifiable

Group: General Forum Members
Last Login: Today @ 8:11 PM
Points: 5,571, Visits: 24,767
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.


If everything seems to be going well, you have obviously overlooked something.

Ron

Please help us, help you -before posting a question please read

Before posting a performance problem please read
Post #1204502
Posted Saturday, November 12, 2011 3:14 AM


Mr or Mrs. 500

Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500

Group: General Forum Members
Last Login: 2 days ago @ 3:19 AM
Points: 577, Visits: 2,503
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



Best wishes,

Phil Factor
Simple Talk
Post #1204575
Posted Saturday, November 12, 2011 3:19 AM
SSC Eights!

SSC Eights!SSC Eights!SSC Eights!SSC Eights!SSC Eights!SSC Eights!SSC Eights!SSC Eights!

Group: General Forum Members
Last Login: Friday, June 27, 2014 12:08 AM
Points: 860, Visits: 807
But then what would they publish on TheDailyWTF.com?




-- Stephen Cook
Post #1204576
Posted Saturday, November 12, 2011 6:57 AM
Valued Member

Valued MemberValued MemberValued MemberValued MemberValued MemberValued MemberValued MemberValued Member

Group: General Forum Members
Last Login: 2 days ago @ 8:19 PM
Points: 55, Visits: 291
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.
Post #1204620
Posted Saturday, November 12, 2011 7:33 AM
Valued Member

Valued MemberValued MemberValued MemberValued MemberValued MemberValued MemberValued MemberValued Member

Group: General Forum Members
Last Login: 2 days ago @ 8:19 PM
Points: 55, Visits: 291
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.
Post #1204623
Posted Saturday, November 12, 2011 9:35 AM
Valued Member

Valued MemberValued MemberValued MemberValued MemberValued MemberValued MemberValued MemberValued Member

Group: General Forum Members
Last Login: Friday, April 25, 2014 10:27 AM
Points: 71, Visits: 671
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...
Post #1204635
« Prev Topic | Next Topic »

Add to briefcase 12345»»»

Permissions Expand / Collapse