Hello all

Has anyone here ever participated or lead an group in a PL/SQL code
review? My project manager and I believe that our we need to
incorporation a code review/inspection into our process and I am trying
to assemble a list of resources that should help us in accomplishing
this task.

Most importantly, I'm looking for sites/white papers that deal with
either Code Review (the general approach) or specific documents
pertaining to the process (defect checklists specific to PL/SQL)

CW

redrobot5050@gmail.com wrote:
> Hello all
>
> Has anyone here ever participated or lead an group in a PL/SQL code
> review?

I reviewed over 2 million lines of third party PL/SQL code in a past
contract position. I was the 'one man code review' team. )


> My project manager and I believe that our we need to
> incorporation a code review/inspection into our process and I am trying
> to assemble a list of resources that should help us in accomplishing
> this task.

My problem was that the code was already running live but too many
problems had been detected, data corruptions were happening and
duplication. My job was to find out why - and looking at the code gave
me a pretty good clue !

If you are performing a code review as part of the development process
then I congratulate you and your manager on some serious foresight !


> Most importantly, I'm looking for sites/white papers that deal with
> either Code Review (the general approach) or specific documents
> pertaining to the process (defect checklists specific to PL/SQL)

I didn't have any documents to hand, other then the Oracle docs, but
common sense (badly named if you ask me !) and a good overall knowlege
of how Oracle works and what Oracle packages etc exists should be a
good help. I did my reviews based on :

... transaction control : were there rogue commits and/or rollbacks
anywhere ?

... exception handling : was there any? Did it work? Did it handle all
exceptions? Were any exceptions simply NULL'd out?

... Correct use of Oracle built in functions etc - correct calling
parameters and so forth.

... Any wheel re-inventing going on? For example, any code to validate a
string as a number but wasn't using TO_NUMBER and exception handling
etc.

... Use of SELECT COUNT(*) ... prior to an UPDATE or INSERT 'choice'
rather than doing what was required and trapping exceptions etc or use
of EXISTS where absolutely necessary.

... Bulk operations rather than looping around - ie use of FORALL and
BULK COLLECT in PL/SQL to reduce context switching and increase
performance.

... etc.

Hope this helps.

Cheers,
Norm.

redrobot5050@gmail.com wrote:
> Hello all
>
> Has anyone here ever participated or lead an group in a PL/SQL code
> review? My project manager and I believe that our we need to
> incorporation a code review/inspection into our process and I am trying
> to assemble a list of resources that should help us in accomplishing
> this task.
>
> Most importantly, I'm looking for sites/white papers that deal with
> either Code Review (the general approach) or specific documents
> pertaining to the process (defect checklists specific to PL/SQL)
>
> CW

Many including teaching classes on how to do them.

Any code that is not reviewed, quite simply, does not belong in
test ... much less production.

The biggest issues around code reviews are:
1. Management must have the background to mandate them and stand
behind the reviewers.
2. Everybodies code must be reviewed ... that includes UNIX sys
admins and their shell scripts as well as the code executed
by DBAs as part of their "maintenance" function. It can not
just be a "beat up on the developers" fest.
3. The person or persons doing the reivew must actually be
competent ... not just have an important title. I have seen
DBAs that couldn't write a decent PL/SQL package criticizing
code they quite frankly couldn't understand.
4. The persons reviewing the code should include (1) A subject
matter expert, (2) the person that wrote the code, (3) at
least a few members of the development team. This is done so
that lessons are learned and spread through the group.
5. The "subject matter expert" is not the "expert" because they
have been using Oracle since version 5, or 6, or whatever but
is actually an expert on the version being used. I have seen
Oraclasours (Oracle dinosaurs) who had never seen bulk collection
pass as good code with cursors that was horrible just out of
pure ignorance.
6. Make sure that the review is done as an educational event not
as a lets make the coder feel like they are being personally
taken to task event.

<RANT>
A major cellular phone company in this area just sent me a requirement
for a new DBA. They specifically asked for someone that DID NOT know
Oracle 6, or 7, or 8.0.x.

Why you may ask? Why not go for someone with many years of experience.

Because they are tired of dealing with dinosaurs that are trying to
manage 9i and 10g databases the way they managed older versions. They'd
rather train someone on what to do then untrain them on ingrained bad
habits.

This is not the first time I've heard this from a DBA lead about his
co-workers but it is the first time it has been reflected in a
corporate hiring policy.

If you aren't keeping your skills up ... you might as well know COBOL.
</RANT>
--
Daniel A. Morgan
http://www.psoug.org
damorgan@x.washington.edu
(replace x with u to respond)

DA Morgan wrote:

> redrobot5050@gmail.com wrote:
>
[...]
>
> The biggest issues around code reviews are:
> 1. Management must have the background to mandate them and stand
> behind the reviewers.
> 2. Everybodies code must be reviewed ... that includes UNIX sys
> admins and their shell scripts as well as the code executed
> by DBAs as part of their "maintenance" function. It can not
> just be a "beat up on the developers" fest.
> 3. The person or persons doing the reivew must actually be
> competent ... not just have an important title. I have seen
> DBAs that couldn't write a decent PL/SQL package criticizing
> code they quite frankly couldn't understand.
> 4. The persons reviewing the code should include (1) A subject
> matter expert, (2) the person that wrote the code, (3) at
> least a few members of the development team. This is done so
> that lessons are learned and spread through the group.
> 5. The "subject matter expert" is not the "expert" because they
> have been using Oracle since version 5, or 6, or whatever but
> is actually an expert on the version being used. I have seen
> Oraclasours (Oracle dinosaurs) who had never seen bulk collection
> pass as good code with cursors that was horrible just out of
> pure ignorance.
> 6. Make sure that the review is done as an educational event not
> as a lets make the coder feel like they are being personally
> taken to task event.
>
> <RANT>
>[...]>
> If you aren't keeping your skills up ... you might as well know COBOL.
> </RANT>


Well put. I first heard the term "two sets of eyes" applied to
programming quite some time ago.

"The important thing to understand is the principle behind pair
programming – two sets of eyes looking at code. Two sets of eyes catches
errors that may show up later in full testing. A second set of eyes can
tell how readable is your code."

How two-years-ago! ;-)

http://www.artima.com/forums/flat.js...6&thread=20073
http://www.cs.cmu.edu/~pattis/15-1XX...ogramming.html

-Mark Bole

redrobot5050@gmail.com wrote:
>
> Has anyone here ever participated or lead an group in a PL/SQL code
> review? My project manager and I believe that our we need to
> incorporation a code review/inspection into our process and I am trying
> to assemble a list of resources that should help us in accomplishing
> this task.

Code reviews tend to be a negative experience for developers. Not that
I mind grabbing the old lead pipe and beating up developers over bad
code in production.. but that seldom achieves the desired result.

Why are code reviews needed? I believe that question should first be
asked. And then solutions must be looked at. In my experience the prime
one is education. Get the developers' skills to a common and acceptable
level. Get them to understand why it makes sense to follow the
development standards set, show them the goalposts so they know what to
aim for.

Of course, not that easy. What did work and worked well is when we did
an outsource job many years ago using external sub-contractors for
development. The first week for a new group of contractors was
dedicated to training. I gave them the specs for a minor subsystem
(already developed) that was about 3 man days worth of development. The
week (5md's) was spend by the new contractors developing that subsystem
in a dev environment. This was the ideal environment for them to get
their feet wet, make mistakes, and learn from it.

After that week, they were fully productive and turned out quality
code. I reviewed their code as part of my Q&A duties, but it was minor
and nothing like these full blown (and often confrontational) reviews
one tends to get. And 90+% of the code was really quality stuff. Sure,
the 1st 5md's was training at our expense. But we got a lot more back
on that investment in the end.

We had the occassion where a contractor kind of failed this 1st week's
training. We terminated their contracts immediately.

Obviously this situation we had was unique in that it did not have to
deal with existing "entrenched" developers that are use to doing things
in a specific "non-standard" (hackish?) way. Still.. I believe that
this type of approach to training and evolving skill levels is far more
beneficial than code reviews.

--
Billy

Billy wrote:

> Of course, not that easy. What did work and worked well is when we did
> an outsource job many years ago using external sub-contractors for
> development. The first week for a new group of contractors was
> dedicated to training. I gave them the specs for a minor subsystem
> (already developed) that was about 3 man days worth of development. The
> week (5md's) was spend by the new contractors developing that subsystem
> in a dev environment. This was the ideal environment for them to get
> their feet wet, make mistakes, and learn from it.
>
> After that week, they were fully productive and turned out quality
> code. I reviewed their code as part of my Q&A duties, but it was minor
> and nothing like these full blown (and often confrontational) reviews
> one tends to get. And 90+% of the code was really quality stuff. Sure,
> the 1st 5md's was training at our expense. But we got a lot more back
> on that investment in the end.
>
> We had the occassion where a contractor kind of failed this 1st week's
> training. We terminated their contracts immediately.

An excellent methodology. I plan to flatter you by copying it the next
time I get the opportunity: Brilliant.
--
Daniel A. Morgan
http://www.psoug.org
damorgan@x.washington.edu
(replace x with u to respond)

DA Morgan wrote:
> redrobot5050@gmail.com wrote:
> > Hello all
> >
> > Has anyone here ever participated or lead an group in a PL/SQL code
> > review? My project manager and I believe that our we need to
> > incorporation a code review/inspection into our process and I am trying
> > to assemble a list of resources that should help us in accomplishing
> > this task.

I've done reviews in PL/SQL, C code, FORTRAN, PASCAL, et al.
The processes are really independent of the language. (Note the plural.
There really more than one way to do this, ranging from simple desk
check to Inspections. I'll try to make further comments about this in
another post.)

> >
> > Most importantly, I'm looking for sites/white papers that deal with
> > either Code Review (the general approach) or specific documents
> > pertaining to the process (defect checklists specific to PL/SQL)
> >
> > CW

(Checklists hints that you are leaning towards Inspections.)
>
> Many including teaching classes on how to do them.
>
> Any code that is not reviewed, quite simply, does not belong in
> test ... much less production.

Wish that were true every where. 8^(

>
> The biggest issues around code reviews are:
[good points but I want to emphasize this one:]
> 6. Make sure that the review is done as an educational event not
> as a lets make the coder feel like they are being personally
> taken to task event.

Given your situation, new to team, poorly defined requirements, code in
place that has mutated over time, Then the opportunity is to present
this as training, refining the current code, and knowledge transfer. If
a requirement is in developer A's head, in the review he points it out
and thru the review now the other three know this (even if it ends up
not being written down).

So reviews are educational for all participants. Small teams need this
kind of knowledge transfer.

>
> <RANT>
[deleted 8^) ]
>
> If you aren't keeping your skills up ... you might as well know COBOL.
> </RANT>

Programming is sometimes an Alice-In-Wonderland experience. You must
run as fast as you can to stay in the same place.

Ed
Magic Interface, Ltd.

Billy wrote:
> redrobot5050@gmail.com wrote:
> >
> > Has anyone here ever participated or lead an group in a PL/SQL code
> > review? My project manager and I believe that our we need to
> > incorporation a code review/inspection into our process and I am trying
> > to assemble a list of resources that should help us in accomplishing
> > this task.
>
> Code reviews tend to be a negative experience for developers. Not that
> I mind grabbing the old lead pipe and beating up developers over bad
> code in production.. but that seldom achieves the desired result.

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.

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.

>
> Why are code reviews needed? I believe that question should first be
> asked. And then solutions must be looked at. In my experience the prime
> one is education. Get the developers' skills to a common and acceptable
> level. Get them to understand why it makes sense to follow the
> development standards set, show them the goalposts so they know what to
> aim for.

And like a marriage, there will have to be some compromises to be
successful.
>
> Of course, not that easy. What did work and worked well is when we did
> an outsource job many years ago using external sub-contractors for
> development. The first week for a new group of contractors was
> dedicated to training. I gave them the specs for a minor subsystem
> (already developed) that was about 3 man days worth of development. The
> week (5md's) was spend by the new contractors developing that subsystem
> in a dev environment. This was the ideal environment for them to get
> their feet wet, make mistakes, and learn from it.
>
> After that week, they were fully productive and turned out quality
> code. I reviewed their code as part of my Q&A duties, but it was minor
> and nothing like these full blown (and often confrontational) reviews
> one tends to get. And 90+% of the code was really quality stuff. Sure,
> the 1st 5md's was training at our expense. But we got a lot more back
> on that investment in the end.

Exellent example.
>
> We had the occassion where a contractor kind of failed this 1st week's
> training. We terminated their contracts immediately.
>
> Obviously this situation we had was unique in that it did not have to
> deal with existing "entrenched" developers that are use to doing things
> in a specific "non-standard" (hackish?) way. Still.. I believe that
> this type of approach to training and evolving skill levels is far more
> beneficial than code reviews.
>
> --
> Billy

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.

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. (try getting some
code not done by anyone on your team and seen how you all deal with it,
so there's no team member feelings to hurt).

HTH
Ed
(I'm no guru, but I have successfully used just about every review
process at one time or other. Being a contract engineer can provide a
wide range of experience.)

DA Morgan wrote:
> An excellent methodology. I plan to flatter you by copying it the next
> time I get the opportunity: Brilliant.

I'm more flattered by free beer coupons.. <hint> <hint>

;-)
--
Billy

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...

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)

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.

> 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.

> 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..

Is it too much to expect a certain quality in the code that is put into
production?

> 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