Code reviews? - Page 3

Do you have a question? Post it now! No Registration Necessary

Translate This Thread From English to

Threaded View
Re: Code reviews?

Quoted text here. Click to load it
That's what I mean, for the "main bulk of the review", we had 4 hours to
do it in. We had special tools to prepare the material, so each reviewer
got the same prepared package. Reviewing was done page by page,
collecting and noting comments an a page basis. DIfferent codes for
different "errors" etc. ranging from "fatal" (MUST change) to "nice
to see" (at the discretion of author). Too many fatals meant a new
review, otherwise a "check updates according to review notes" by
2 or 3 selected reviewers. Everything was archived under change control
(we were ISO-9001 certified:>) ).
Quoted text here. Click to load it
The meeting normally took about 2 hours.
Quoted text here. Click to load it

Re: Code reviews?
The one aspect I haven't seen specifically mentioned in this
thread is the human one of assigned reviewers not having or
taking the time to properly prepare for a review.  If the
review is an added task for an otherwise fully-occupied
reviewer, review preparation may become a lower priority
and may get omitted if other work is more pressing.  This
can be even more so if the reviewer's regular work is
perceived to be where success/failure is determined and
the review work will be mentioned in passing if at all.

I will mention in passing a technique I once saw used for
spec review.  A copy of the spec was fastened to the walls
of a room and everyone was supposed to make a trip to the
room and make notes on the posted copy.  This was supposed
to be a more efficient way to gather everyone's comments.
I don't know about others, but I'm not going to do nearly
as thorough a review if I have to stand on my feet for
extended periods to do it.  [Requests for personal copies
of the spec were refused.]

Re: Code reviews?


Quoted text here. Click to load it

Perhaps you missed this:

| From: Guy Macon <http://www.guymacon.com
| Newsgroups: comp.arch.embedded
| Subject: Re: Code reviews?
| Date: Mon, 22 Nov 2004 21:23:48 +0000
|
| Here are some ways a code review can go astray:
|
| The reviewer may come unprepared.  This is not the time or the
| place for him to learn what the product does or what the customer
| requires, yet he has little motivation to take time from his own
| projects to study a requirements document for someone else's project.

Quoted text here. Click to load it

http://www.csirik.net/time-management.gif

--
Guy Macon <http://www.guymacon.com




Re: Code reviews?
Quoted text here. Click to load it

1
Static analysis.  Ie Lint.

2
After that leave it for a week. Look at the spec (not the code and write
some test scripts for the ICE or SIM to unit test the code.

/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\
\/\/\/\/\ Chris Hills  Staffs  England    /\/\/\/\/\
/\/\/ snipped-for-privacy@phaedsys.org       www.phaedsys.org \/\/
\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/

Re: Code reviews?

Quoted text here. Click to load it

You would have this problem with reviews of documentation other than your
code methinks. The person that reviews your documentation and/or your code
need not be in your own company. You can always employ a reviewer to look
over the code. However, you will want to progress the code to a point where
it hangs reasonably well together.

As one who has worked as a lone consultant I maintained a small group of
contacts that I could trust who would assist me in the review stages. It
cost me a small amount to retain their services but I did not need them
full time (often doing the reviews in the evenings or at weekends and
combining it with a social meeting). Many of my reviewing colleagues I had
worked with in larger organisations so I was well aware of their
capabilities and their thinking.

--
********************************************************************
We've slightly trimmed the long signature. Click to see the full one.
Re: Code reviews?
On Fri, 19 Nov 2004 11:37:12 UTC, martin griffith

Quoted text here. Click to load it

  Martin,

  It can be done, although it is usually more helpful to have
someone else review the code, design, or behavior.  When I
have to do that I mentally put on my 'review hat' and set
aside time for just that.  It can't be done after you've
coded it because all the details are still fresh in your
head.  Give yourself a little time and approach it as trying
to find ways to improve what you did.

  I've had to work alone on several projects and it helps
to have a definite routine to follow as far as requirements,
design, coding, testing, verification, validation, and so
on.  I rely heavily on version control systems for all my
code, build procedures, and documents.  One of my absolute
rules is that all code must be reviewed before it is checked
in.  Everything must be complete, correct, development
tested, and changes documented.  If I must check in code
between complete feature implementations, I leave unique
markers thoughout the code as to what needs to be finished.
Those are checked for again later (as a rule) before I would
attempt to build the project.

  You've taken the first step and recognized the need for
improvement in your process.  Decide how to fill that role
and improve again as needed.

  If you'd like an email buddy for such reviews I can
occasionally spare some time.  If you are interested I'll
send you my real address; this one is obviously not
real.

  David

Re: Code reviews?

Another technique that I have had a lot of success with is doing
assert-based programming during development, with a way to turn
off the asserts at the end of the project.



Re: Code reviews?
It's true that reviewing the code can be good. I see two reasons this:

1. Improving the quality of the code.
I believe this is covered in many of the previous postings. If you don't
have any help to review the code static analyzers and testing, testing and
testing is the way to go.
In my experience code reviewing does not improve the quality of the code
that much.

2. Improving yourself as a programmer.
Code reviewing is a way of sharing experience with other programmers. This
cannot be done without interaction with other people. The good thing is that
it doesn´t have to be code that you produce for a living that is reviewed.
You could for example participate in an open source project just for the
posiblity of learing from others.




Quoted text here. Click to load it



Re: Code reviews?


Quoted text here. Click to load it

In my experience (and I have a *lot* of experience with code
reviews), not improving the quality of the code is a symptom
of an improperly-run code review.  

Here are some ways a code review can go astray:

It can morph into a design review.  The reviewer must avoid
redesigning the program.  There is a time and a place for that
(requirements and initial design phases) and this is not the
time or the place.  Code reviews morphing into design reviews
are especially likely when the reviewer has a much higher skill
level than the programmer.

It can morph into a teaching session.  This happens when the
reviewer has a much lower skill level than the programmer.

It can morph into a management tool with the goal of employee
evaluation rather than product improvement.  Having the boss sit
in or (worse) be the reviewer causes this, as does having a
reviewer who is the company snitch.

It can be too early.  If the project is late, there is a temptation
to schedule the code review before the code is done, and to use
that as the deadline - and as a club to beat the programmer with
to make him work faster.  That's the job of the ship date, not
the code review date.

It can be too late.  There is little point in having a code review
if you know that you will be saying "that's a real problem, but we
don't have time to fix it."

The reviewer may come unprepared.  This is not the time or the
place for him to learn what the product does or what the customer
requires, yet he has little motivation to take time from his own
projects to study a requirements document for someone else's project.

It can become a pissing contest.  The reviewer may prefer a different
methodology but he is there to help the programmer, not to get into
a fight with him.

The programmer may see it as a threat and resist the process.
The problem is that he is often right and it really is a threat.
I always tell the programmer and reviewer that "what happens in
the code review room stays in the code review room."  I also get
the buy-in of the programmer or I simply refuse to have a code
review.

It can become a committee meeting.  The best number of participants
is three: programmer, reviewer, moderator.  It is acceptable to
let a junior member of the staff sit in and write down action items,
but he should not participate.

If you avoid the above pitfalls, code reviews can do more to improve
the quality of the code than any other one-day activity can.

--
Guy Macon <http://www.guymacon.com


Re: Code reviews?

Quoted text here. Click to load it

Absolutely correct. Code reviews must be run properly to be worthwhile in
terms of quality improvement for the system and its software.
 
Quoted text here. Click to load it

The primiary purpose of a review is to identify and list the problematic
issues within the design. No reccommendations for adopting alternative
design strategies may be made as part of the review comments. Just what the
problem is and how it is exhibited or perceived to be exhibited.
 
Quoted text here. Click to load it

Selection of appropriate reviewers is necessary. However, using members of
the same team, members of other similar teams or even an outside entity
(person or company) that has been breifed on the requirements would also
suit.
 
Quoted text here. Click to load it

Technical reviews are not for the boss to attend (unless you can trust him
to wear just his technical hat).
 
Quoted text here. Click to load it

The policy I have is to review early and review often. However, the number
of lines in many of the functions I am writing are small by comparison to
many who post here (my granularity for a subroutine tends to average less
than 16 lines including the comments and statement of functional
requirements - then I use Forth quite a lot). At each review I will have a
number of units of code ready to be reviewed. Once the code has passed the
review it is subjected to the testing phases. Pass that and it enters the
trusted library.
 
Quoted text here. Click to load it

I know of companies that leave their reviews until they have completed at
least 80% of the coding. After several days of review meeting they discover
that they have really huge problems.

Quoted text here. Click to load it

If the review material has been pre-posted to the reviewers they should
have prepared. So, question one of the review is of the preparedness of the
review team. If anyone is not prepared then reschedule the meeting but
reprimand the unprepared member (he buys everyone pizza for the next
meeting out of his pocket).
 
Quoted text here. Click to load it

Agreed.
 
Quoted text here. Click to load it

The programmer whose code is being reviewed has to be present in the room
but his main job is to listen to the views of the others and to answer
specific questions directed at him for the purpose of clarification only.
 
Quoted text here. Click to load it

Minimum of three maximum of seven I think is the rule. All reviewers should
have knowledge of the domain being dealt with (not necessarily all from
programming). Meeting posts are Reader, Moderator, Recorder, Reviewer,
Developer (programmer or engineer whose work is under review). More than
one post can be filled be filled by the same person but be careful with
such job sharing.
 
Quoted text here. Click to load it

True, so true. See my book reccomendations (Freedman and Weinberg) mad a
few days ago for the proper way to organise reviews. It is all good advice.

--
********************************************************************
We've slightly trimmed the long signature. Click to see the full one.
Re: Code reviews?



Quoted text here. Click to load it
I second that....First step is to educate the participants about what a
code review is and does...etc...
Also, not sure how to interpret 'improve the quality of
code..much'...Where would you prefer to find problems? coding or
integration (or worse yet, fielding?)? Code inspections are static and
should focus on issues that can be uncovered in a static setting,
leaving those nasty concurrency issues to be caught at integration time.
  Allows the focus of integration testing to be on dynamic issues
(concurrency, latency, performance, etc...)

Quoted text here. Click to load it

"...issues with design"?  Systems already has been designed...code is
implementation (Unless your one of those folks who practice voodoo
software development from the 60s.). Code reviews evaluate the code
itself (although it may kick up issues with design that should be used
to clarify the coders understanding of it or, kick the problem to
another level).  Depending on the company/department/group, the
objectives of code reviews fall into the following categories:
conformity (to a coding standard/style), correct translation of specs to
functionality, language specific nuances (i.e. ptr boundary checking in
C, etc.)that can cause a failure of the system. A failure is defined as
the deviation of service being provided as compared to the intended
(e.g. specified) service.
As an aside, the thinking that one does not do code inspections unless
'its safety related software' is exactly the thinking that causes
billions of dollars of lost time and data in just about all software
project.  Don't know about you, but I go ballistic every time Windoz
decides to do a GPF when I am writing a doc in Word.  Then again, it
might be safety related if it keeps my from putting my fist through the
monitor.....and it costs me tons of time to reload an OS or an
application, and it costs my employer money for me to do this.
So you (collectively) think code inspections don't apply to one person
operations? Take a look at publications by Watts Humphries about the
Personal Software Process...they are techniques and guidelines that help
individuals develop a (personal) sw dev process to improve their coding.
One of the  techniques is self-reviews...hmmmm don't got time to do self
reviews?  How much time ya got to fix problems later on? Sorry...I
digress...
Quoted text here. Click to load it
I guess I am a bit stronger on that point...I'd say never, no matter
what size the organization.  To my way of thinking, a CR produces no
usable metrics for project management, other than 'coding of module xxx
is complete." If having to go through rework again and again, thats a
sign of other problems that should be handled by a process team/person
to minimize those reworks.  That involves keeping track of the types of
nonconformities that cause failures in code reviews which can be
presented to the supervisor without 'finger pointing' but in a way to
motivate improvement.
Quoted text here. Click to load it
I's say that company won't be in business very long given their current
rate.
Quoted text here. Click to load it
Problem is, managers, thinking that they are the only ones incapable of
making mistakes, look for 'measures' to evaluate a coders performance.
I've seen too many cases where code review results are used like a
'test' to grade a programmer. Can you say 'pointy haired manager'???
Then again, this is one of the signs of a truly bad organization that
probably won't be in existence very long, and thats a good thing...
Quoted text here. Click to load it
One of the functions is a 'process' observer to keep things on the
straight and narrow so it does not turn into any one of the problems
cited above (committee meeting, bash the developer, tutor the developer,
etc...)
Quoted text here. Click to load it
There are tons of literature about the good effects (and side effects)
of code reviews. Problem is most electrical and computer engineers (and
   some computer science programs) have taken 2-4 'software' courses
where they learn programming languages, structure, and environments and
then go off and write code for their embedded robotics controllers (or
whatever) to get a good grade (or maybe not) and get the degree.  That
*may* (but I have some problems with that too) be OK, but unfortunately
they aren't exposed to good sw development processes and practices in
their training (particularly true of traditional engineering students),
so when they hit the real world, they aren't adequately prepared for the
way to do things better in a more global setting.  But things are slowly
changing.
Oh well, sorry for the rant...I saw a few things that pushed my buttons
and had to respond...
John

Re: Code reviews?

Quoted text here. Click to load it

Not that it was my comment originally but I prefer to find out about
problems as early as possible. There can be enormous improvements made with
the right approach and iverview.
 
Quoted text here. Click to load it

There are sometimes elements of design going on during the coding period
concurrent with the coding effort. This may be for a number of reasons and
designs are never fully done and dusted until the implementation is
complete. I know it would be nice if the design phase could be fully
completed before coding starts but that is not always possible. This,
however, is not a problem with decent configuration and change management
practices being employed.

I do see, though, that you probably understood my drift from the comments
you made and your list of likely issues is a reasonable starting point.

Quoted text here. Click to load it

I also noted that seemed to be the general leaning of the group but I still
maintain that it is no excuse for not having a review. If you are a one man
outfit you need to hire or aquire the help you need. I expect that many one
man businesses hire in an accountant to assist with getting the books to
atax-department acceptable standard (I know I always did). So it is the
same that you obtain the assistance of someone else to do the review with
you. A simple NDA covers the issue of IP that may be in the material.

Quoted text here. Click to load it

I guess you haven't been working with outfits where the boss is also one of
the programmers. As I said, he has to promise to wear only his technical
hat in the review meeting and everyone else will remind him if he steps out
of that mode. Otherwise, if you can keep the boss out that will be to the
better.

Quoted text here. Click to load it

....and may that evolutionary process be continue to be accellerated!!!.

Quoted text here. Click to load it

My software education has never been formalised. I did my apprenticeship in
Electronic systems engineering and learnt the software side when we started
using microcontrollers for various things. My first programming job was
actually performed by placing diodes in the correct place in a matrix. That
was back in 1970. I was also let loose on General Automation based systems
(hardware maintenance (but we had to write our own test routines to check
the kit out). My first microprocessor was the 6800 which I hand crafted the
machine code for an application that only ran issue 1 for 25 years with
zero errors (4kbytes of hand entered code). To me, whether it is the
hardware or the software, it is all just good engineering practice.

Quoted text here. Click to load it

We shall try and push your buttons again sometime then ;>

--
********************************************************************
We've slightly trimmed the long signature. Click to see the full one.
Re: Code reviews?
Quoted text here. Click to load it


I prefer to find the problems myself and I do this by testing the code
before integration.

Writing test code that tests the greater part of the code you have written
will find much more problems than you can ever do by code reviews. Actually,
I see testcode as a form of code review for finding problems in the code.
The tests can be made automatic so you can test the whole product once every
day i.e. a code review every day of almost all code.
Of course testcode will not find ten levels of nested if-statements so
therefore I use code reviews to find problems with the structure of the code
(and not so much to find real errors because they have already been found in
the testing).

Just to clarify; by testing I mean unit testing, module testing or wathever
you call it. Not integration testing.



Re: Code reviews?


Quoted text here. Click to load it
Always a good idea...this is the dynamic counterpart to CRs (static)
However unit testing will likely NOT uncover integration related
problems..i.e. assumptions made about services running/available, shared
variable out-of-order (i.e. read before write-stale data) problems...etc.
Quoted text here. Click to load it
There are many published articles that put forth contrary experience.
When I find my list I'll post it..
Quoted text here. Click to load it

Re: Code reviews?
Quoted text here. Click to load it

Thanks. I would appreciate that.
I think most people (including me) that have a strong opinion about code
reviews are baseing their opinion on a gut feeling and not actual
measurements. If someone can show me that a lot of code reviews will make
the software cheaper (in long term) I´m willing to try to convince my
intestines.



Re: Code reviews?

Quoted text here. Click to load it

Why the "a lot of" qualifier?  Is your digestive tract telling you
that some code reviews are good but many are bad?  I have had good
results with a single code review after the code is 100% written but
before quality testing is complete.  Others here have had good results
with a code review of each module as it is completed.  I don't think
that anyone has suggested reviewing the same lines of code multiple
times.

--
Guy Macon <http://www.guymacon.com


Re: Code reviews?
Quoted text here. Click to load it
Many lines of code. Not so many programmers. If you want to review all the
code it has to be divided into many reviews. For example if you have 15
programmers that produces 500.000 lines of code in one and a half year. How
many reviews is that?

Quoted text here. Click to load it
Not bad but expensive (which actually is bad). The feeling I have is that I
get better pay-back from static analyzers and unit testing.




Re: Code reviews?

Quoted text here. Click to load it

So, half a million LOC in 18 moths and no code review? I am certain that
you may have found out quite a bit more about the code by just doing a
static walk-through a sub-routine at a time.

I have found that the review process I go through, in my average project,
will actually eliminate code lines. I may write more code than ends up in
the project but only that which meets the needs of the requirements will
remain, useless flourishes will be evicted and the code gets much tighter
and resilient through the review process. I doubt that tool like LINT or
other static checking tool will have that sort of effect to any great
degree.

--
********************************************************************
We've slightly trimmed the long signature. Click to see the full one.
Re: Code reviews?
Quoted text here. Click to load it
No, I didn't say anything about the number of code reviews we had. And I´m
not saying that code reviews are bad and that you shouldn´t have them.
Every programmer had about 2-3 reviews of a rather small part of the code
they had produced. I think I would like to have had twice that number of
reviews. The focus of the reviews was mainly to check that our coding
standard and rules and recommendations was followed - a way to get everybody
on the same track.
The rules and recommendations describes how the software should be developed
on a level above the coding standard. It actually handles many of the
problems that would have been discussed on a code review.



Re: Code reviews?
On Wednesday, in article

Quoted text here. Click to load it

Be sure that the tests do test all that should be tested, if nothing else
get someone else to review the test vectors/conditions and the RESULTS.
However module testing can sometimes go awry if external events screw things
up, if no locking fo resources are performed and something else tampers
with part of it (e.g. DMA buffers), the module may work on its own but
not when integrated.

Quoted text here. Click to load it

Some of the code I have written has been near impossible to create the
millions of variations possible in source data (video frames), only partial
cases and often it is easier to force conditionsto extremes of noise
and characteristics of capture method. Creating test data sets to test the
code usually creates data sets that are too 'clean' to cover all areas of
problems.

Quoted text here. Click to load it

Too true. Where possible when working on my own I often at least talk
through the algorithm flow to ensure what can be done with someone who may
understand the maths or purpose enough to highlight potential problems.
This can often be useful as a debugging aid for conditions that upset part
of an algorithm. The algorithm may well be split across MANY modules.

Quoted text here. Click to load it

Unit testing works on some parts, but falls down in my experience with
asynchronisity of various levels of integration. Especially with external
events, from the mundane of operator actions to the variabilities of video
sync delay timing (or dropout of said same for some sources).

--
Paul Carpenter          | snipped-for-privacy@pcserviceselectronics.co.uk
<http://www.pcserviceselectronics.co.uk/ PC Services
We've slightly trimmed the long signature. Click to see the full one.

Site Timeline