Oracle FAQ Your Portal to the Oracle Knowledge Grid
HOME | ASK QUESTION | ADD INFO | SEARCH | E-MAIL US
 

Home -> Community -> Usenet -> c.d.o.server -> Re: PL/SQL Code Reviews

Re: PL/SQL Code Reviews

From: Ed Prochak <ed.prochak_at_magicinterface.com>
Date: 19 Sep 2005 10:52:46 -0700
Message-ID: <1127152366.307370.32320@z14g2000cwz.googlegroups.com>

Billy wrote:
> Ed Prochak wrote:
>
> > If participants enter with the idea of beating up the coder, then who's
> > suprised at the coder getting bruised? This should not be like ORAL
> > exams for a doctorate degree. The goal is better code and some team
> > communication.
>
> And if the coders "refuse" to comply - to understand the basics, RTFM,
> achieve enlightment, etc?
>
> More than once I've had the occassion where new technology is used and
> developers insisting on using old concepts. It tends to get quite
> confrontational at times. That bit about leading a horse to water...

That's exactly the point. It is their code. If it fails to meet functionality or performance specs, then your manager can put on the pressure. You as a reviewer cannot and should not pressure the developer.

>
> I do not mind if the issues is like demonstrating why soft parses are
> better than hard parses, or no parses (statement handle re-use) is
> better than soft parses.. or what the bindless SQL do versus bind SQL.
> Or the advanatages of bulk processing.
>
> But what when the issue is a conceptual one and the counter arguments
> is based on ignorance of basic Oracle concepts? Refer them to the
> manual? Which they do not bother to read - ever? Spend hours and hours
> building test cases to show basic fundemental concepts? Thus I do tend
> at times reach for the lead pipe as I refuse to waste my time (I also
> have deadlines) with developers that refuses to understand fundamental
> concepts after I've explained in brief the concept with references to
> applicable Oracle manuals and documentation. (e.g. why ref cursors for
> doing HTMLDB reports is non-workable idea)

Reviewers should never carry lead pipes! In the review you note the problem or location for possible enhancement and move on. You do not go into the details of the alternate solutions.
>
> I think it is easier to deal with younger guys. They are still clean
> slates and can be taught. Much more difficult with older developers
> that believe that their development Kung Fu is of such a high standard
> that they know better than you.

that's a team/personnel/management issue, NOT a review issue.
>
> > A little training of participants goes a long way toward preventing the
> > negative results. Just the simeple reminder that it is a CODE review,
> > not a programmer review helps. Everyone, including the coder needs to
> > be reminded occasionally.
>
> Agree. But the Oracle University PL/SQL courses for example, seems to
> me very basic and does not teach/re-inforce fundemental programming and
> design techniques.
>

Note: I was talking about being trained in the review process, not training in programming laqnguages.

> > One rule (of many) we used at a place that did Inspections is that the
> > coder owns the code. So even if reviewers say, change this, reformat
> > that, etc., the coder has to respond but the response may be to ignore
> > the advice. This empowerment reduces the stress levels all around.
>
> I have a problem with that - as I would sit in meetings with management
> and operations and have to explain why there are poor performance. Or
> deal with issues like snapshots too old, deadlocks and so on..

Why isn't the coder in that meeting instead of you? This is where the owner of the code has to start explaining why he failed to follow suggestions from the review. You already did the CYA move in the review, pointing out the potential problems (Your team does retain review minutes, right?).
>
> Is it too much to expect a certain quality in the code that is put into
> production?

No, that's why we do code reviews. But you clearly need support, primarily from your manager. The review process isn't your problem, and using the review as a ranting session is just a BAD IDEA. Obviously your teammates are less interested in producing quality code than you are. That's a rough place to be.

>
> > And a final point. There are many ways of doing these reviews. Simple
> > desk checks, Execution reviews, unstructured reviews, structured
> > reviews, and software inspections. The unstructured reviews most easily
> > break down to beat-up-the-developer sessions. Whatever you do, the
> > process needs some clear guidelines AND practice.
>
> Agree with you on that Ed. In fact, I dislike the idea of a code review
> entirely. If it comes down to that I would rather want to deal with the
> devloper alone on a personal level - not in a peer situation where egos
> are at stake.
>
> --
> Billy

Done right, reviews should remove the ego issues, or at least isolate them outside the review meeting. A one-on-one desk check is one of the simplest reviews, but it is a coarse filter. Still it's better than no filter at all.

Please don't take offense, Billy, but you seem to take the problems personally, even when it is not your code. Keep the horse proverb in mind a little more often. It becomes almost a political process, a matter of persuassion, to bring your temmates around. Again I say your problem is not with reviews, but with the team. I hope it gets better.

  Ed Received on Mon Sep 19 2005 - 12:52:46 CDT

Original text of this message

HOME | ASK QUESTION | ADD INFO | SEARCH | E-MAIL US