Code reviews?
martin griffith
Fine
Q:
How do you review your own code, ie as a 1 man self employed dude?
Its so easy to miss things when reviewing your own code.
martin
Serious error.
All shortcuts have disappeared.
Screen. Mind. Both are blank.
Gary Kato
>How do you review your own code, ie as a 1 man self employed dude?
>
>Its so easy to miss things when reviewing your own code.
So true. Have someone else you know (a programmer) look at the code. Another
way is to put the code away for a few days, then come back to it.
Find good coding standards and use them. Don't try to write one routine that
does a bunch of stuff. That makes it easier for you to see whether it is doing
its task correctly depending on inputs.
In general, watch the complexity. It's hard to strike a balance sometimes. If
you have too many routines, that can be hard to keep track of too. Having
decent documentation helps there.
Also having an overall document on the goals of the project is good. Sometimes
when coding, you can stray off the path or forget about a goal or three,
especially if there are a lot of goals (as I found when trying to make a
canonical Palm OS app).
Hans-Bernhard Broeker
> How do you review your own code, ie as a 1 man self employed dude?
In a nutshell: you don't. You simply don't have the necessary
resources to do it.
Which leaves you with three options:
1) Become a "Real Programmer"(no TM). I.e., you could such a god-like
perfect code that reviews are futile.
2) Get external consultants (including your customers), to review your
code.
3) Become a >1 person shop. Marry another programmer, hire one, team
up with one, anything, just make sure you're no longer all alone.
Of course, we all know that 1) is a non-option, realistically speaking
--- insuring against it is what reviews are about, after all.
--
Hans-Bernhard Broeker (bro...@physik.rwth-aachen.de)
Even if all the snow were burnt, ashes would remain.
Hans-Bernhard Broeker
> How do you review your own code, ie as a 1 man self employed dude?
In a nutshell: you don't. You simply don't have the necessary
resources to do it.
Which leaves you with three options:
1) Become a "Real Programmer"(no TM). I.e., you could teach yourself
to become such a god-like perfect coder that reviews are futile.
Robert Kaiser
martin griffith <marting...@yahoo.co.uk> writes:
> A bit of googling shows that code reviews are a "good idea"
> Fine
>
> Q:
> How do you review your own code, ie as a 1 man self employed dude?
Hmm, make it open source? (That's how Linus Torvalds gets his
code reviewed..)
If thats impractical: Try leaving it alone for at least a week while
working on something completely different, then look at it again.
Rob
--
Robert Kaiser email: rkaiser AT sysgo DOT com
SYSGO AG http://www.elinos.com
Klein-Winternheim / Germany http://www.sysgo.com
Frank Bemelman
news:8gmrp0l2t1li8ep7b...@4ax.com...
> A bit of googling shows that code reviews are a "good idea"
> Fine
>
> Q:
> How do you review your own code, ie as a 1 man self employed dude?
>
> Its so easy to miss things when reviewing your own code.
Do you need it? Assuming you're not in the rocket lauch
business like everybody else here, you can rely on the
experience with your previous projects. If you didn't
have too much problems with that, why bother.
I don't really review my code. I test. I improve if performance
demands it. Sometimes, when I borrow a piece of my older code,
(if I can find it quick enough) I may say to myself 'You can
do better than that' and still use it as is ;)
--
Thanks, Frank.
(remove 'x' and 'invalid' when replying by email)
Casey
> 3) Become a >1 person shop. Marry another programmer, hire one, team
> up with one, anything, just make sure you're no longer all alone.
Just a thought - it's a lot easier to "unhire" a helper than it is to
divorce one.
Casey
Casey
> Q:
> How do you review your own code, ie as a 1 man self employed dude?
>
> Its so easy to miss things when reviewing your own code.
Yes, it is. Been there, done that, still doing it.
Regardless of all the good reasons why code reviews are considered
necessary, the ugly truth is that sometimes you just can't do one.
I've done a lot of one man jobs and sometimes there really is no one
else available or the weeks of time to allow someone to dive into your
creation and understand it.
Out of necessity, I end up relying on others to torture test the
product instead. Those people are usually far more readily available.
It's amazing what someone will try that you'd never dream of.
I've been doing embedded work in the telecom industry for the last 16
years and never seen a code review yet. Granted, I've worked with
smaller companies and products, but between tight schedules, looming
deadlines, and overstretched budgets, it's just never happened.
I can just hear the screams now ....
Casey
Dave Hansen
wrote:
[...]
>I've been doing embedded work in the telecom industry for the last 16
>years and never seen a code review yet. Granted, I've worked with
>smaller companies and products, but between tight schedules, looming
>deadlines, and overstretched budgets, it's just never happened.
If you program in C, I would say you _need_ lint.
Regards,
-=Dave
--
Change is inevitable, progress is not.
Chris Hills
>Fine
>
>Q:
>How do you review your own code, ie as a 1 man self employed dude?
>
>Its so easy to miss things when reviewing your own code.
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 /\/\/\/\/\
/\/\/ ch...@phaedsys.org www.phaedsys.org \/\/
\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/
R Adsett
says...
> On Fri, 19 Nov 2004 10:09:16 -0600, Casey <cclremov...@cox.net>
> wrote:
>
> [...]
> >I've been doing embedded work in the telecom industry for the last 16
> >years and never seen a code review yet. Granted, I've worked with
> >smaller companies and products, but between tight schedules, looming
> >deadlines, and overstretched budgets, it's just never happened.
>
> If you program in C, I would say you _need_ lint.
Robert
WaldemarIII
"Dave Hansen" <id...@hotmail.com> schreef in bericht
news:419e2231....@News.individual.net...
> On Fri, 19 Nov 2004 10:09:16 -0600, Casey <cclremov...@cox.net>
> wrote:
> If you program in C, I would say you _need_ lint.
;-)
(Oops, shouldn't have said that.)
Former collegue of mine (in one of his more exuberant moods) is known to
have said:
"...if you need lint or similar crap, you just can't program and you never
will..."
He was reprimanded by his boss, who prided himself with some 10 years of
programming
experience (a decade, ha ha ha ha ha! impressive). Boss dictated that every
piece of
code head to be submitted to lint every day and the logs be forwarded to
him. Boss left
the company a couple of months later. :-p
Seriously now... Lint flags ambiguous constructs and stuff like that, (don't
need to tell you
that, do I?), which is good (no question about that) but it can not and
never will explain
why you ordered the statements in that particular way, nor will it explain
the functionality
and the creative process behind those statements. It's a tool, not a means.
Waldemar
WaldemarIII
"Frank Bemelman" <f.bem...@xs4all.invalid.nl> schreef in bericht
news:419df889$0$568$e4fe...@news.xs4all.nl...
> Do you need it? Assuming you're not in the rocket lauch
> business like everybody else here, you can rely on the
> experience with your previous projects. If you didn't
> have too much problems with that, why bother.
honest professional software development here. Companies
can't afford to ship products that may be flawed because the
software wasn't submitted to at least one review. Oh, I know,
it happens all the time, I'm not that naive... But mentioning
medical applications, traffic control, automotive control,
you name it... All those critical applications that may cause
injury or worse if malfunctioning... I bet you your head that
software like that is reviewed till it croakes.
Waldemar
Paul E. Bennett
> A bit of googling shows that code reviews are a "good idea"
> Fine
>
> Q:
> How do you review your own code, ie as a 1 man self employed dude?
>
> Its so easy to miss things when reviewing your own code.
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.
--
********************************************************************
Paul E. Bennett ....................<email://peb@a...>
Forth based HIDECS Consultancy .....<http://www.amleth.demon.co.uk/>
Mob: +44 (0)7811-639972 .........NOW AVAILABLE:- HIDECS COURSE......
Tel: +44 (0)1235-811095 .... see http://www.feabhas.com for details.
Going Forth Safely ..... EBA. www.electric-boat-association.org.uk..
********************************************************************
Frank Bemelman
"WaldemarIII" <blee...@hotmail.com> schreef in bericht
news:419e51f2$0$46979$cd19...@news.wanadoo.nl...
When I said 'assuming you're not in the rocket lauch business' I
actually meant to say *all* applications where failure does not
result in truly big trouble. Extreme critical applications aren't
typically the area where one-man-businesses find their bread and
butter. Many one-man-businesses develop solutions for other (small)
companies, often to improve production and often very unique to the
situation at hand. Not sold in the thousands, low risk etc.
In other words, I was very serious. If you are in that kind of
business, and the stuff you deliver works reasonably well, it
makes little sense to change your methods. Although I have to
admit that it sometimes would be nice if someone else would
review my code, just for the sake of getting some feedback.
Paul E. Bennett
> Regardless of all the good reasons why code reviews are considered
> necessary, the ugly truth is that sometimes you just can't do one.
I thought the truth was that sometimes you feel it may not be worthwhile
doing a review. In actuallity it is always worthwhile and you may get to
the point of wishing you had when an un-reviewed bit of code bites you back
hard. Of course, up to now you may have just got away with it but one
day...... ;>
> I've done a lot of one man jobs and sometimes there really is no one
> else available or the weeks of time to allow someone to dive into your
> creation and understand it.
If you keep to small simple modules of documentation and code, you can
merely drip feed it through the reviewer. Other respondents have already
mentioned the benefits of keeping it simple.
> Out of necessity, I end up relying on others to torture test the
> product instead. Those people are usually far more readily available.
> It's amazing what someone will try that you'd never dream of.
OK if it is a product that has no safety implications.
> I've been doing embedded work in the telecom industry for the last 16
> years and never seen a code review yet. Granted, I've worked with
> smaller companies and products, but between tight schedules, looming
> deadlines, and overstretched budgets, it's just never happened.
Is that why we get glitches on the phone line from time to time??? ;>
> I can just hear the screams now ....
No screams. Just the gentle sounds of "tut, tut, tut".
Casey
> Casey wrote:
>
> > Regardless of all the good reasons why code reviews are considered
> > necessary, the ugly truth is that sometimes you just can't do one.
>
> I thought the truth was that sometimes you feel it may not be worthwhile
> doing a review.
Really? I certainly didn't say or imply that.
> > I've done a lot of one man jobs and sometimes there really is no one
> > else available or the weeks of time to allow someone to dive into your
> > creation and understand it.
>
> If you keep to small simple modules of documentation and code, you can
> merely drip feed it through the reviewer. Other respondents have already
> mentioned the benefits of keeping it simple.
You can't if there is "the reviewer" doesn't exist. Keeping things
simple is great, but there are real-world cases where you just don't
have the time, money, or luxury for a review.
> > Out of necessity, I end up relying on others to torture test the
> > product instead. Those people are usually far more readily available.
> > It's amazing what someone will try that you'd never dream of.
>
> OK if it is a product that has no safety implications.
Products with safey implications are of course in a different category.
> > I can just hear the screams now ....
>
> No screams. Just the gentle sounds of "tut, tut, tut".
Client asks you to do the software for a new product - the budget is
tight, the schedule almost impossible. You're presented with a choice
- take the job and do your best or refuse the job. "Tut, tut," doesn't
enter into the equation. The fact that you have a teenaged son that
eats a lot might weigh heavily on your mind.
Btw, I just finished one of those jobs.
Casey
Frank Bemelman
news:cnlo3m$cao$1$8300...@news.demon.co.uk...
>
> OK if it is a product that has no safety implications.
>
What about products that cause a lot of damage. If a CNC machine
drills holes in the wrong spot, it can ruin quite a bit, truly
upsetting people.
This is all kindergarten talk. We all know that some parts of
our software are more critical than others. And not all bugs
have desastrous effects. Nobody ships code without at least
some confidence, and most 'bugs' are only a nuisance, not more.
Hell, I even ship code with known bugs. Work in progress.
A few months ago, somewhere in the UK, an ATM machine was
spitting out twice the amount of money. In no-time the
entire village was queing up in front of the machine.
Software bug? Nah. A bank employee had swapped the bins
by mistake, and 20 pound notes were ejected instead of 10
pounds.
I am a bit amused that the majority here exclusively works
on applications that launch rockets, keep satelites in
orbit, control heart-lung machines, or are used in airplanes
(and I am not referring to the sensor that detects if the
toilet seat is up or down) and god knows what have you.
As a result, malfunctioning software always let the rockets
explode, satelites get lost, patients die by the dozen,
and airplanes burry themselves in the ground. For some
reason we never hear of that button that pops up the
wrong menu and confuses the operator, the vending machine
that thought it was empty while not, the weird message in
the logfile, the panel light that didn't go off, the
christmas lights that sometimes changes it's blinking
pattern, etc....
David
<marting...@yahoo.co.uk> wrote:
> A bit of googling shows that code reviews are a "good idea"
> Fine
>
> Q:
> How do you review your own code, ie as a 1 man self employed dude?
>
> Its so easy to miss things when reviewing your own code.
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
Paul E. Bennett
> "Paul E. Bennett" <p...@amleth.demon.co.uk> schreef in bericht
> news:cnlo3m$cao$1$8300...@news.demon.co.uk...
>>
>> OK if it is a product that has no safety implications.
>>
>
> What about products that cause a lot of damage. If a CNC machine
> drills holes in the wrong spot, it can ruin quite a bit, truly
> upsetting people.
Safety is an issue in all CNC machines so I would hope is not the type of
product that does not see a code review, not only for the basic executive
code but also for the machining code to produce the product. In the latter
I know there is some tool assistance. Not only is it expensive to damage
the tools but it can also launch shrapnel if it shatters (hende you are
then hoping that the guards are actually closed and adequate to the task of
catching the bits).
> This is all kindergarten talk. We all know that some parts of
> our software are more critical than others.
True.
> And not all bugs
> have desastrous effects. Nobody ships code without at least
> some confidence, and most 'bugs' are only a nuisance, not more.
I would hope that it is more than merely some confidence. I tend to require
a greater element of certainty about what I ship. Just for my own peace of
mind if nothing else.
> Hell, I even ship code with known bugs. Work in progress.
If I know I have a bug in the code it is not shipped until the bug has been
removed. Mind you, most of them are eliminated very early in the coding
segment of development.
> I am a bit amused that the majority here exclusively works
> on applications that launch rockets, keep satelites in
> orbit, control heart-lung machines, or are used in airplanes
> (and I am not referring to the sensor that detects if the
> toilet seat is up or down) and god knows what have you.
Some examples of my involvements:-
Hydrolastic Mixing Facility (25 years at issue 1 - 6800 Assembly)
Gas Analysis (for automotive industry - GA Assemmbly)
Peripheral Test Programs (Pr1me Assembly, BASIC and Fortran IV)
Gas & Oil Production Well Head Controls (WesDAC SCADA)
Railway Signalling Equipment (Ada)
Automated Battery Charging Systems (6309 Forth)
Paraplegic Electric Wheelchair (RTX2001 Forth)
Nuclear Fuel Re-processing equipment
Train Safety Monitoring System
Coal Stacker Reclaimer (Mitsubishi PLC)
Warship Compass Data Distribution System (D3 and 1553B)
Army Tank Equipment Test Rigs (Gun and Suspension) (Forth)
Nuclear Power Plant Fuel Handling and Recycling Equipment (S80)
Plutonium Pick and Place Crane (6309 Assembly, Literal and Forth)
Medical Anesthesia Ventilator Software Certification (Forth)
Multihead Weighing System (Forth)
Medical Sterilisation Fluid Manufacturing Facility (Forth)
Fusion Reactor Fuelling Systems (Allen Bradley PLC and Forth)
Naturally I wasn't on my own for all of those. Some of the projects are
only done from within a larger company. However, some of the more critical
ones were handled by a team of between 5 and 9 people filling all the
engineering and project management roles.
> As a result, malfunctioning software always let the rockets
> explode, satelites get lost, patients die by the dozen,
> and airplanes burry themselves in the ground. For some
> reason we never hear of that button that pops up the
> wrong menu and confuses the operator, the vending machine
> that thought it was empty while not, the weird message in
> the logfile, the panel light that didn't go off, the
> christmas lights that sometimes changes it's blinking
> pattern, etc....
No matter how good we all become at our jobs, no matter how perfectly
produced and debugged our software becomes, there is always the chaotic
element that will unravel our best efforts. However, the chaotic element
does not need to be given a helping hand to disrupting our lives just
because we failed to review our work and let the bug we should have spotted
through the net.
WaldemarIII
news:dZWdnRAxmrq...@rogers.com...
> Everyone who programs in C NEEDS lint
Beg pardon for my ignorance. Why would I need lint?
How can I benefit from it?
Waldemar
Peter
"WaldemarIII" <blee...@hotmail.com> wrote in message
news:419f15c0$0$69648$1b2c...@news.wanadoo.nl...
also pick out your bad habits. Once you have cleaned up your code with lint
you can get on with the real debugging.
Peter
Hans Bus
> martin griffith said
<snip>
> I've been doing embedded work in the telecom industry for the last 16
> years and never seen a code review yet. Granted, I've worked with
> smaller companies and products, but between tight schedules, looming
> deadlines, and overstretched budgets, it's just never happened.
>
> I can just hear the screams now ....
>
>
> Casey
I have different experience in the (last 12 years) in the embedded
Telecom business. We ALWAYS did reviews. Unfortunately, if the
planning allows you 4 hours preparation and 2 hours review meetings
for a 3 month programming job, why bother, you get a lot of comments
on comments, nobody had time to really analyse your code properly.
Morale : Serious reviewing takes a lot of time, plan for that.
Regards,
Hans Bus
Ed Beroset
> makes little sense to change your methods. Although I have to
> admit that it sometimes would be nice if someone else would
> review my code, just for the sake of getting some feedback.
It's not quite the same thing, but one way you can get another "opinion"
on your code (if it's C) is to run it through lint. Gimpel's PC-Lint is
very good, and for open source, there is Splint (
http://lclint.cs.virginia.edu/ ).
Another way to approximate a real code review that is to use a checklist
(there are many on the web). For each checklist item, if you go through
the code just scanning for that one thing, you can do a pretty fair even
on your own code.
I've done both of these on code I've written as a one-man-job. It's
also a useful habit to get into when you get into the situation that
you're actually getting real code reviews with other people, because
your code will be that much better going into the review.
Ed
Paul E. Bennett
>
> Morale : Serious reviewing takes a lot of time, plan for that.
Except that really serious reviewing doesn't take that long at all. For
guidance on the process of conducting all kinds of technical reviews and
walkthroughs see the "Handbook of Walkthroughs, Inspections, and Technical
Reviews: Evaluating Programs, Projects, and Products." by Daniel P.
Freedman and Gerald M. Weinberg. Third Edition is ISBN 0-932633-19-6.
Frank Bemelman
news:_hHnd.4417$Qh3....@newsread3.news.atl.earthlink.net...
> Frank Bemelman wrote:
>
> > makes little sense to change your methods. Although I have to
> > admit that it sometimes would be nice if someone else would
> > review my code, just for the sake of getting some feedback.
>
> It's not quite the same thing, but one way you can get another "opinion"
> on your code (if it's C) is to run it through lint. Gimpel's PC-Lint is
> very good, and for open source, there is Splint (
> http://lclint.cs.virginia.edu/ ).
Yes, (sp)lint is nice to catch some pitfalls. But often I found it
quite a hassle to configure it for a particular compiler.
> Another way to approximate a real code review that is to use a checklist
> (there are many on the web). For each checklist item, if you go through
> the code just scanning for that one thing, you can do a pretty fair even
> on your own code.
I will look into that, and test myself ;)
> I've done both of these on code I've written as a one-man-job. It's
> also a useful habit to get into when you get into the situation that
> you're actually getting real code reviews with other people, because
> your code will be that much better going into the review.
Frank Bemelman
> I will look into that, and test myself ;)
And I feel I will waste a lot of time on silly red tape:
g. Structuredness:
1) Is each function of the program recognizable as a block of code?
2) Do loops only have one entrance?
1) - of course, if it compiles it has an end.
2) - I never have done this, and if I had, I have a bloody good reason
to do so.
R Adsett
mooc...@newprovidence.demon.co.uk says...
Yep, lint serves multiple purposes.
- It's a good syntax checker, for many small embedded systems I've
worked with it's orders of magnitude better than the compiler. I'm in
the habbit of running through lint before the compiler. Usually the only
compiler complaints I see are either chip specific or the compiler is in
error.
- It catches many simple errors that are syntactically correct. Usually
if lint is complaining about it it is an error and if it is not there is
usually a clearer way of expressing the intent that doen't cause lint to
complain so..
- It leads to clearer code. One of the benefits of lint is over time it
beats certain bad habbits out of you.
- Since lint can check a lot of the detailed line by line picky points
it reduces the load on reviewers to catch such things as order
dependances, mismatched types etc, leaving them free to concentrate on
the logic. When I last ran reviews it was a precondition of the review
that the code lint clean (any exceptions noted and justified).
- With Gimpels lint (I don't know about lc-lint) the use of strong type
checking will catch type mismatches. Unresolvable type mismatches (ie
w/o extensive casting) usually indicate an underlying flaw in the model
used to represent the values used in the program. It's also capable of
catching cross module errors which compilers don't (at least none I've
worked with) catch (although proper header files make a difference here).
Consider lint an idiot savant reviewer. It doesn't tire and it doesn't
grasp the logic of the specification but what it does check for it checks
throughly and quickly. I've found that if lint is complaining about a
lot of things that's usually a sign there are other problems as well.
These comments don't apply to the original Unix lint. It's been
outpaced.
Robert
Guy Macon
Paul E. Bennett wrote:
>Except that really serious reviewing doesn't take that long at all. For
>guidance on the process of conducting all kinds of technical reviews and
>walkthroughs see the "Handbook of Walkthroughs, Inspections, and Technical
>Reviews: Evaluating Programs, Projects, and Products." by Daniel P.
>Freedman and Gerald M. Weinberg. Third Edition is ISBN 0-932633-19-6.
Excellent book.
It has been my experience that *not* doing reviewing and other kinds
of software testing/quality assurance takes far more time than doing
those things takes.
Guy Macon
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.
Ed Beroset
> "Ed Beroset" <ber...@mindspring.com> schreef in bericht
> news:_hHnd.4417$Qh3....@newsread3.news.atl.earthlink.net...
>
>>Frank Bemelman wrote:
>>
>>
>>>makes little sense to change your methods. Although I have to
>>>admit that it sometimes would be nice if someone else would
>>>review my code, just for the sake of getting some feedback.
>>
>>It's not quite the same thing, but one way you can get another "opinion"
>>on your code (if it's C) is to run it through lint. Gimpel's PC-Lint is
>>very good, and for open source, there is Splint (
>>http://lclint.cs.virginia.edu/ ).
>
>
> Yes, (sp)lint is nice to catch some pitfalls. But often I found it
> quite a hassle to configure it for a particular compiler.
It's easiest to start out with "-weak" and just work on that first, but
you're right that there is work involved in configuring it to work with
a particular compiler. Unfortunately, there is still no such notion as
"interrupt" in standard C.
>>Another way to approximate a real code review that is to use a checklist
>>(there are many on the web). For each checklist item, if you go through
>>the code just scanning for that one thing, you can do a pretty fair even
>>on your own code.
>
> I will look into that, and test myself ;)
It works for me. I have also found that if I find an error (of any
kind) the best thing to do is to look for more just like it. In some
cases, I have written quick Perl scripts to automatically go through my
code and search for certain kinds of things (e.g. free() of space
allocated by alloca()). If I were more organized, I'd have saved each
of those and made a nice collection, but I'm sad to report that I didn't
have the forethought to do it. Maybe you will. :-)
Ed
Ed Beroset
> "Frank Bemelman" <f.bem...@xs4all.invalid.nl> schreef in bericht
> news:419f71b6$0$65124$e4fe...@news.xs4all.nl...
>
>
>>I will look into that, and test myself ;)
>
>
> And I feel I will waste a lot of time on silly red tape:
>
> g. Structuredness:
> 1) Is each function of the program recognizable as a block of code?
> 2) Do loops only have one entrance?
>
> 1) - of course, if it compiles it has an end.
Not the same thing. The question here is whether each *function*
corresponds to a block of code. If you have an embedded system which
has an LCD display, is the function responsible for driving that LCD
recognizeable as a block of code? Or is the code which talks to the LCD
littered throughout the source code? That's what that first one means.
> 2) - I never have done this, and if I had, I have a bloody good reason
> to do so.
In assembly language, it's much easier to make this kind of error, and
in fact, there usually is no good reason to do so, as evidenced by the
fact that you've never done it!
In any case, you need to create your *own* checklist which retains only
relevant items. For example, "conforms to coding guidelines" might be
irrelevant if you don't have any. However, I'd argue that the better
fix to that particular one would be to invent and follow consistent
coding guidelines. You probably don't write randomly formatted code
now, but you might not necessarily have written down the "rules" for how
you write code. That's all the coding guidelines are, and they're only
important because they eliminate the distraction of poorly or
inconsistently written code.
My rule of thumb: if you have a good reason to break a rule, do so.
Ed
R Adsett
ber...@mindspring.com says...
>
> My rule of thumb: if you have a good reason to break a rule, do so.
>
Robert
Chris Hills
<f.bem...@xs4all.invalid.nl> writes
>"Ed Beroset" <ber...@mindspring.com> schreef in bericht
>news:_hHnd.4417$Qh3....@newsread3.news.atl.earthlink.net...
>> Frank Bemelman wrote:
>>
>> > makes little sense to change your methods. Although I have to
>> > admit that it sometimes would be nice if someone else would
>> > review my code, just for the sake of getting some feedback.
>>
>> It's not quite the same thing, but one way you can get another "opinion"
>> on your code (if it's C) is to run it through lint. Gimpel's PC-Lint is
>> very good, and for open source, there is Splint (
>> http://lclint.cs.virginia.edu/ ).
>
>Yes, (sp)lint is nice to catch some pitfalls. But often I found it
>quite a hassle to configure it for a particular compiler.
>
"free" tool can cost more than a commercial tool by the time you have
set it up.
Chris Hills
<blee...@hotmail.com> writes
>
>"Dave Hansen" <id...@hotmail.com> schreef in bericht
>news:419e2231....@News.individual.net...
>> On Fri, 19 Nov 2004 10:09:16 -0600, Casey <cclremov...@cox.net>
>> wrote:
>> If you program in C, I would say you _need_ lint.
>Lint doesn't understand functionality, to me it's just another C beautifier
>;-)
>(Oops, shouldn't have said that.)
No you shouldn't. It is a static analyser. It is not a beautifier. If
you don't understand the difference you shouldn't be programming.
BTW what do you use for static analysis?
You do, of course know, that static analysis will pick up 80% of the
bugs and dynamic the other 20%?
>Former collegue of mine (in one of his more exuberant moods) is known to
>have said:
>"...if you need lint or similar crap, you just can't program and you never
>will..."
Idiots usually have delusions.
I note that many who design the language swear by lint.
>He was reprimanded by his boss, who prided himself with some 10 years of
>programming
>experience (a decade, ha ha ha ha ha! impressive). Boss dictated that every
>piece of
>code head to be submitted to lint every day and the logs be forwarded to
>him.
>Boss left
>the company a couple of months later. :-p
I am not surprised. If the comment above from your colleague was
symptomatic of the company the boss probably left to find somewhere more
professional.
>Seriously now... Lint flags ambiguous constructs and stuff like that, (don't
>need to tell you
>that, do I?), which is good (no question about that)
That is I hope not in debate by anyone.
> but it can not and
>never will explain
>why you ordered the statements in that particular way, nor will it explain
>the functionality
>and the creative process behind those statements.
That is in the design notes and comments....
>It's a tool, not a means.
Yes.
Chris Hills
eolusdevelopment.cm> writes
Interestingly I have found those who really know what they are doing
swear by lint (or something similar) and would not program without some
form of static analysis.
It is the cowboys and hackers who think they are so clever who don't use
it. They *always* have their own special method that is better.
Chris Hills
eolusdevelopment.cm> writes
>
>Yep, lint serves multiple purposes.
> - It's a good syntax checker, for many small embedded systems I've
>worked with it's orders of magnitude better than the compiler.
The compiler is a translator.
Lint is a static analyser.
They do different jobs.
> I'm in
>the habbit of running through lint before the compiler.
That was I believe the original intention. It was intended to be used in
makefiles every time you compiled.
>Usually the only
>compiler complaints I see are either chip specific or the compiler is in
>error.
Likewise.
Chris Hills
<http@?.guymacon.com> writes
VERY TRUE I have seen it in action. Teams that use coding guidelines,
static analysis and reviews had fewer bugs. Lint, specifically, cut out
a lot of the noise which meant the few remaining bugs were easier to
find.
This meant the project came in early! Saved the company a lot of money
and we got on with the next one.
Chris Hills
<cclremov...@cox.net> writes
>
>Client asks you to do the software for a new product - the budget is
>tight, the schedule almost impossible. You're presented with a choice
>- take the job and do your best or refuse the job. "Tut, tut," doesn't
>enter into the equation. The fact that you have a teenaged son that
>eats a lot might weigh heavily on your mind.
Then this is the perfect time to use lint... it will rip out many bugs
and potential bugs almost as you write the code. Cuts down on a hell of
a lot of debugging.
I too have done comms (SDH) systems. I also have a son (or two) who eats
more than his own body weight per day, demands pocket money and borrows
the car.... :-)
Guy Macon
Chris Hills wrote:
>BTW what do you use for static analysis?
>You do, of course know, that static analysis will pick up 80% of the
>bugs and dynamic the other 20%?
You have found a way to identify 100% of all bugs? Unbelievable!
It has been my experience that Static analysis finds the first 80%
of the bugs, then dynamic analysis finds the second 80% of the bugs,
leaving an additional 80% of the bugs undiscovered. :)
Frank Bemelman
> Frank Bemelman wrote:
> > "Frank Bemelman" <f.bem...@xs4all.invalid.nl> schreef in bericht
> > news:419f71b6$0$65124$e4fe...@news.xs4all.nl...
> >
> >
> >>I will look into that, and test myself ;)
> >
> >
> > And I feel I will waste a lot of time on silly red tape:
> >
> > g. Structuredness:
> > 1) Is each function of the program recognizable as a block of code?
> > 2) Do loops only have one entrance?
> >
> > 1) - of course, if it compiles it has an end.
>
> Not the same thing. The question here is whether each *function*
> corresponds to a block of code. If you have an embedded system which
> has an LCD display, is the function responsible for driving that LCD
> recognizeable as a block of code? Or is the code which talks to the LCD
> littered throughout the source code? That's what that first one means.
>
> > 2) - I never have done this, and if I had, I have a bloody good reason
> > to do so.
>
> In assembly language, it's much easier to make this kind of error, and
> in fact, there usually is no good reason to do so, as evidenced by the
> fact that you've never done it!
Yes, that's true, there's another reason why I avoid assembler ;)
> In any case, you need to create your *own* checklist which retains only
> relevant items. For example, "conforms to coding guidelines" might be
> irrelevant if you don't have any. However, I'd argue that the better
> fix to that particular one would be to invent and follow consistent
> coding guidelines. You probably don't write randomly formatted code
> now, but you might not necessarily have written down the "rules" for how
> you write code. That's all the coding guidelines are, and they're only
> important because they eliminate the distraction of poorly or
> inconsistently written code.
Yes, over the years I have developed my own guidelines or adopted
ones from others. While I haven't written them down, it does help
a lot. But my latest bug would not have been caught by merely code
review, not very likely anyway. Only by testing, I'd think. And I
had tested it, but obviously not thoroughly enough ;)
> My rule of thumb: if you have a good reason to break a rule, do so.
Sounds like a good rule. Let's stick to that, before we break more ;)
Rich Walker
> It is the cowboys and hackers who think they are so clever who don't use
> it. They *always* have their own special method that is better.
Amen to that.
Also, "I can't use someone else's library. It doesn't use my stylistic
conventions for naming things, I don't know where it's been, and it'll
take longer to find the bugs in it than to write my own from scratch."
cheers, Rich.
--
rich walker | Shadow Robot Company | r...@shadow.org.uk
technical director 251 Liverpool Road |
need a Hand? London N1 1LX | +UK 20 7700 2487
www.shadow.org.uk/products/newhand.shtml
Paul E. Bennett
>>> If you program in C, I would say you _need_ lint.
>>Everyone who programs in C NEEDS lint
That I would agree with.
[%X]
> It is the cowboys and hackers who think they are so clever who don't use
> it. They *always* have their own special method that is better.
Alternatively they may be using a different language for which LINT or any
other automated static analysis tool does not exist. By using such a
totally different language they gain in other ways and still turn out
exceptionally good software. They will, however, do reviews, unit testing,
system testing and regression checking all the way through development.
R Adsett
says...
> In article <09qdnR9_0On...@rogers.com>, R Adsett <radsett@junk.a
> eolusdevelopment.cm> writes
> >
> >Yep, lint serves multiple purposes.
> > - It's a good syntax checker, for many small embedded systems I've
> >worked with it's orders of magnitude better than the compiler.
>
> The compiler is a translator.
> Lint is a static analyser.
>
> They do different jobs.
I really was referring to syntax checking in this case though. That
portion of analysis that each does (if for different purposes) to confirm
the code is valid and catches the simple typos (extra closing brackets,
dropped ; etc..). Some compilers do a truly lousy job of identifying the
original error. Lint (PC Lint at least) is as good as the best compilers
I've worked with at that identification.
Lint has many more important strengths (especially given good compilers)
but it's a very practical advantage with compilers that have had little
thought given to their error detection.
Robert
Patrik Servin
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.
"martin griffith" <marting...@yahoo.co.uk> skrev i meddelandet
news:8gmrp0l2t1li8ep7b...@4ax.com...
> A bit of googling shows that code reviews are a "good idea"
> Fine
>
> Q:
> How do you review your own code, ie as a 1 man self employed dude?
>
> Its so easy to miss things when reviewing your own code.
>
>
>
> martin
>
> Serious error.
> All shortcuts have disappeared.
> Screen. Mind. Both are blank.
Guy Macon
Patrik Servin wrote:
>In my experience code reviewing does not improve the quality
>of the code that much.
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>
Hans Bus
> Hans Bus wrote:
>
>
>>Morale : Serious reviewing takes a lot of time, plan for that.
>
>
> Except that really serious reviewing doesn't take that long at all. For
> guidance on the process of conducting all kinds of technical reviews and
> walkthroughs see the "Handbook of Walkthroughs, Inspections, and Technical
> Reviews: Evaluating Programs, Projects, and Products." by Daniel P.
> Freedman and Gerald M. Weinberg. Third Edition is ISBN 0-932633-19-6.
>
Still, I can't imagine a technique to review 20.000 - 25.000 line of
(commented) C code in 4 hours. We found out the hard way that the review
process wasn't good enough. During integration testing we found lots of
errors that should have been detected in the reviews.
But my main point was, that reviewing has to be taken into account
during the planning stage, and not just a token to please management.
Regards,
Hans Bus
Paul E. Bennett
>
> Patrik Servin wrote:
>
>>In my experience code reviewing does not improve the quality
>>of the code that much.
>
> 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.
Absolutely correct. Code reviews must be run properly to be worthwhile in
terms of quality improvement for the system and its software.
> 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.
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.
> It can morph into a teaching session. This happens when the
> reviewer has a much lower skill level than the programmer.
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.
> 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.
Technical reviews are not for the boss to attend (unless you can trust him
to wear just his technical hat).
> 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.
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.
> 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."
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.
> 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.
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).
> 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.
Agreed.
> 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.
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.
> 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.
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.
> 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>
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.
Paul E. Bennett
> Paul E. Bennett wrote:
>> Hans Bus wrote:
>>
>>
>>>Morale : Serious reviewing takes a lot of time, plan for that.
>>
>>
>> Except that really serious reviewing doesn't take that long at all. For
>> guidance on the process of conducting all kinds of technical reviews and
>> walkthroughs see the "Handbook of Walkthroughs, Inspections, and
>> Technical Reviews: Evaluating Programs, Projects, and Products." by
>> Daniel P. Freedman and Gerald M. Weinberg. Third Edition is ISBN
>> 0-932633-19-6.
>>
> Can't say I know the book :>)
>
> Still, I can't imagine a technique to review 20.000 - 25.000 line of
> (commented) C code in 4 hours. We found out the hard way that the review
> process wasn't good enough. During integration testing we found lots of
> errors that should have been detected in the reviews.
That is just too much of a big chunk all at once. Wasn't this code in a
series of smaller sub-routines? The main bulk of the review work is
actually conducted outside of the meeting room. Pre-circulate the material
to be reviewed (the moderators) and the meeting collects the list of
problems areas to be revisited by the developer.
> But my main point was, that reviewing has to be taken into account
> during the planning stage, and not just a token to please management.
Agreed. It should be part of the scheduled time and significantly so. No
review should meeting should be longer than 2 hours. Many of mine take less
than an hour but I do have a lot of them.
Patrik Servin
"Guy Macon" <http://www.guymacon.com> skrev i meddelandet
news:10q4m7m...@corp.supernews.com...
>
> Patrik Servin wrote:
>
> >In my experience code reviewing does not improve the quality
> >of the code that much.
>
> 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.
>
failing.
What I question is the value of the code reviews (given that you know how to
do it properly). Code reviews will improve the quality of the code, no
question about that, and most projects should have them. But how much will
they improve the code? Not that much I say.
In many of the projects I have worked in it would have been better to spend
less time on code reviews and more time on design and testing. These
projects did have a good design and good test cases but still I think too
much time was spent on code reviews.
John Hudak
Paul E. Bennett wrote:
> Guy Macon <http://www.guymacon.com> wrote:
>
>
>>Patrik Servin wrote:
>>
>>
>>>In my experience code reviewing does not improve the quality
>>>of the code that much.
>>
>>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.
>
>
> Absolutely correct. Code reviews must be run properly to be worthwhile in
> terms of quality improvement for the system and its software.
>
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...)
>
>>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.
>
>
> 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.
"...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...
>
>
>>It can morph into a teaching session. This happens when the
>>reviewer has a much lower skill level than the programmer.
>
>
> 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.
>
>
>>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.
>
>
> Technical reviews are not for the boss to attend (unless you can trust him
> to wear just his technical hat).
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.
>
>
>>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.
>
>
> 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.
>
>
>>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."
>
>
> 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.
I's say that company won't be in business very long given their current
rate.
>
>
>>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.
>
>
> 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).
>
>
>>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.
>
>
> Agreed.
>
>
>>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.
>
>
> 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.
>
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...
>
>>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.
>
>
> 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.
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...)
>
>
>>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>
>
>
> 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.
>
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
Paul E. Bennett
> 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...)
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.
>> 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.
>
> "...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.
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.
> 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...
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.
>> Technical reviews are not for the boss to attend (unless you can trust
>> him to wear just his technical hat).
> 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.
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.
>> 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.
>>
> 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...
....and may that evolutionary process be continue to be accellerated!!!.
>> 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.
>>
> 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.
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.
> Oh well, sorry for the rant...I saw a few things that pushed my buttons
> and had to respond...
We shall try and push your buttons again sometime then ;>
Guy Macon
>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.
Point well taken. What I was getting at with my comment about
3 or 4 being ideal was the undesirable "one developer, five
reviewers" situation. 3-7 would work well with the division of
labor you list.
Needless to say, if there are seven people in the room, any
splitting off into side conversations must be suppressed
with an iron fist.
Paul E. Bennett
Guy Macon
Paul E. Bennett wrote:
> Date: Tue, 23 Nov 2004 22:47:35 +0000
> NNTP-Posting-Date: Tue, 23 Nov 2004 23:46:09 +0000
Post you replied to: Tue, 23 Nov 2004 23:18:24 +0000
:)
Patrik Servin
> 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...)
>
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.
Jouko Holopainen
> In many of the projects I have worked in it would have been better to spend
> less time on code reviews and more time on design and testing.
My experience is completely opposite. The reason, I think, is because
we have reused a lot of code, and same code again and again for several
years.
> These
> projects did have a good design and good test cases but still I think too
> much time was spent on code reviews.
Obviously I have no clue how much time you did spent in code reviews.
But still I have never ever seen a single properly commented code.
And I do not think I will ever see.
--
@jhol
Jotkut pitävät armeijaa parhaana järjestelmänä joissakin tapauksissa.
He ovat oikeassa, armeija on paras järjestelmä Helvetin jälkeen.
Hans Bus
> Hans Bus wrote:
>
>
>>Paul E. Bennett wrote:
>>
>>>Hans Bus wrote:
>>>
>>>
>>>
>>>>Morale : Serious reviewing takes a lot of time, plan for that.
>>>
>>>
>>>Except that really serious reviewing doesn't take that long at all. For
>>>guidance on the process of conducting all kinds of technical reviews and
>>>walkthroughs see the "Handbook of Walkthroughs, Inspections, and
>>>Technical Reviews: Evaluating Programs, Projects, and Products." by
>>>Daniel P. Freedman and Gerald M. Weinberg. Third Edition is ISBN
>>>0-932633-19-6.
>>>
>>
>>Can't say I know the book :>)
>>
>>Still, I can't imagine a technique to review 20.000 - 25.000 line of
>>(commented) C code in 4 hours. We found out the hard way that the review
>>process wasn't good enough. During integration testing we found lots of
>>errors that should have been detected in the reviews.
>
>
> That is just too much of a big chunk all at once. Wasn't this code in a
> series of smaller sub-routines? The main bulk of the review work is
> actually conducted outside of the meeting room. Pre-circulate the material
> to be reviewed (the moderators) and the meeting collects the list of
> problems areas to be revisited by the developer.
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:>) ).
>
>
>>But my main point was, that reviewing has to be taken into account
>>during the planning stage, and not just a token to please management.
>
>
> Agreed. It should be part of the scheduled time and significantly so. No
> review should meeting should be longer than 2 hours. Many of mine take less
> than an hour but I do have a lot of them.
The meeting normally took about 2 hours.
>
>
Everett M. Greene
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.]
Guy Macon
Everett M. Greene wrote:
>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.
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
| Message-ID: <10q4m7m...@corp.supernews.com>
|
| 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.
>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.
http://www.csirik.net/time-management.gif
John Hudak
Patrik Servin wrote:
>>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...)
>>
>
>
>
> I prefer to find the problems myself and I do this by testing the code
> before integration.
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.
>
> 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,
There are many published articles that put forth contrary experience.
When I find my list I'll post it..
Patrik Servin
> When I find my list I'll post 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.
mike
> A bit of googling shows that code reviews are a "good idea"
> Fine
>
> Q:
> How do you review your own code, ie as a 1 man self employed dude?
>
> Its so easy to miss things when reviewing your own code.
>
>
>
> martin
>
> Serious error.
> All shortcuts have disappeared.
> Screen. Mind. Both are blank.
Rule Number one. BE SURE what you're reviewing!!!
Horror Story:
Did a project to convert an FPGA to an ASIC.
Did design reviews out the wazoo. ASIC came out EXACTLY like the spec.
Only problem was that somebody forgot to roll the version at the last
FPGA update. So the timing mod to compensate for the bad display
drivers didn't get included. Classic example of GIGO. $17K down the drain.
mike
--
Return address is VALID.
500MHz Tek DSOscilloscope TDS540 $2200
http://nm7u.tripod.com/homepage/te.html
Wanted, 12.1" LCD for Gateway Solo 5300. Samsung LT121SU-121
Bunch of stuff For Sale and Wanted at the link below.
http://www.geocities.com/SiliconValley/Monitor/4710/
Guy Macon
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.
Frank Bemelman
news:41A4EF3...@netscape.net...
> martin griffith wrote:
> > A bit of googling shows that code reviews are a "good idea"
> > Fine
> >
> > Q:
> > How do you review your own code, ie as a 1 man self employed dude?
> >
> > Its so easy to miss things when reviewing your own code.
> >
> >
> >
> > martin
> >
> > Serious error.
> > All shortcuts have disappeared.
> > Screen. Mind. Both are blank.
>
> Rule Number one. BE SURE what you're reviewing!!!
>
> Horror Story:
> Did a project to convert an FPGA to an ASIC.
> Did design reviews out the wazoo. ASIC came out EXACTLY like the spec.
> Only problem was that somebody forgot to roll the version at the last
> FPGA update. So the timing mod to compensate for the bad display
> drivers didn't get included. Classic example of GIGO. $17K down the
drain.
> mike
Nice aneckdote ;) I wonder if extensive code reviews leads more
often to the false assumption that everything is covered for.
Paul Carpenter
<LFVod.122559$dP1.4...@newsc.telia.net>
patrik...@hotmail.com "Patrik Servin" wrote:
>> 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...)
>
>I prefer to find the problems myself and I do this by testing the code
>before integration.
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.
>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.
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.
>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).
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.
>Just to clarify; by testing I mean unit testing, module testing or wathever
>you call it. Not integration testing.
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 | pa...@pcserviceselectronics.co.uk
<http://www.pcserviceselectronics.co.uk/> PC Services
<http://www.gnuh8.org.uk/> GNU H8 & mailing list info
<http://www.badweb.org.uk/> For those web sites you hate
Paul E. Bennett
>
> Rule Number one. BE SURE what you're reviewing!!!
>
> Horror Story:
> Did a project to convert an FPGA to an ASIC.
> Did design reviews out the wazoo. ASIC came out EXACTLY like the spec.
> Only problem was that somebody forgot to roll the version at the last
> FPGA update. So the timing mod to compensate for the bad display
> drivers didn't get included. Classic example of GIGO. $17K down the
> drain. mike
Nice rule and nice anecdote to go with it.
Each review has to start with a number of cross-checking actions.
Specification issue level and date; Code issue and date (perhaps even time);
What the Configuration Management Record believes to be the current status.
Problem Reports Received and the records. Latest Change Proposals and Work
Instruction Record.
When all (the flowchart on my HIDECS page gives you an idea of my own
process) those items check out with each other correctly then you can begin
to look at the item to be reviewed.
As for the number of reviews:-
I prefer to resolve the specification first (usually up to 3 iterations)
before I progress to starting the coding. When I do start the coding the
first run through is normally just writing the glossary comment - a sort of
mini-requirement statement - so that I know what I have planned for each
module I am going to build. These glossary comments are then reviewed,
before the code is written, to ensure that the intent for each module is in
conformity to the spec. This might take two iterations at most. When I
start with real code it is tested for intended function by the programmer
almost immediately it is written (remember I use Forth for the most part
and Forth does encourage small functional elements at whatever abstraction
level you are dealing with). There follows a further review which
incorporates a full Fagan style inspection. If this is passed the code is
issued for testing where a formal functional test and a limits test are
performed. The results come back to the review panel who then review the
test results and, if all is OK, approve the code for release to the trusted
library of functions ready to build the rest of the system.
So, from requirements to system integration there would have been 9 or 10
reviews of each element. Once integrated there would be further reviews of
the unit testing, system testing and final integrations testing. As a wrap
up there may also be a formal client witnessed factory acceptance test of
the whole system which would involve yet another review.
While it may seem that there are a lot of reviews the time taken to
accomplish them is of the order of 8% to 10% of the project. The benefit is
that it is quite possible to eliminate at least 95% of all the bugs you are
ever likely to see in the project by taking the issue of proper review and
testing seriously.
It is interesting to note that the longest review meetings actually happen
very early in the project when the requirements and specification documents
are being resolved. These review meetings have occassionally broken the two
hour rule but at that stage it was always worthwhile geting the issues
sorted.
Patrik Servin
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?
> Is your digestive tract telling you
> that some code reviews are good but many are bad?
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.
Paul E. Bennett
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.
Patrik Servin
> 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.
not saying that code reviews are bad and that you shouldn愒 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.
Marc Warden
review and revisit the code to refresh your memory as to why you did
what you did.
Sincerely,
MarcW.