What is code review? (Java code review)

[a24900]

My boss just told me a concept I didn't know. (I am middle level Java
programmer.) -- "code review".

How did you manage to become a programmer without knowing that? A NASA
study from 1987 (yes, the idea is old and established) found that one
finds twice as much faults this way compared to testing. Other, later
sources have found an advantage of 20:1 or 33:1 in time spend on
finding and fixing faults ( which might be exaggerated).

According to what I heard, "code review" is somebody reads the thousands
lines of code written by other person

There is a difference between a code-monkey and a real programmer.

The average amount of code a real programmer writes is in the hundreds
of lines per month. That is average, and assumes thorough research of
the problem to solve, thought-out hand-written code, and code
documentation in an non-trivial field of work in a corporate
environment. I am not talking about endless copy-paste junk. It
further assumes tests are counted separately.

errors (logic errors, I guess, since the code at least can be compiled
and run).

Or, like, for example, multi threading errors? Potential performance
or scaling problems? Potential access right problems? Potential
deployment problems? Resource conflict problems?

And of course the biggies, too. Like misunderstanding or
misinterpreting of specification. Plus the human component, e,g,
weeding out slackers who didn't report their progress accurately, and
are of course behind schedule. Or fakers who can't code at all but got
the job with a faked CV.

I feel this is crazy!!!

Programmers who don't want that other programmers look at their code
usually have something to hide. Real programmers typically like to
show their code to demonstrate their competence and get
acknowledgments from their peers.

Since the reviewer has to "read" the original
code author's mind

No, the reviewer has to read the specification.

and make sure the code does what the author wants

No, makes sure the code does what the specification says.

and no hidden surprises! How this could be possible?! This would be
extremely time consuming and nobody knows better about the code than the
author.

If the author is the only one capable of understanding and handling
the code then the code has no long-term value. If the code is
unreadable or incomprehensible for others, it has a major fault, it is
unmaintainable. Code reviews are already valuable if all they could do
was to guard against unmaintainable code.

My boss says this is very common practice in software engineer development.

Your boss has a clue. A rare event in some corporate environments.
Consider listen to him, there are much worse bosses out there.

 

[Patricia Shanahan]

That's a tricky situation to be in. For what it's worth, where I work,
we feel pretty comfortable about pointing out mistakes to each other. E.g.
"You forgot a null check here", "I think it'd be clearer if you renamed
the method to 'getIntHash', rather than 'getHashInt', etc."

....

I don't know whether I've just been lucky, but for the 32 years I spent
in the computer industry, I was always in teams that had the "We are in
this together, and all want it to work" attitude.

Patricia

 

[Richard Reynolds]

Thank you all for your replies. Now, I have learned a lot about "code
review" and software testing.

Now, I hope to switch the topic to a different one, but related:

I have found that "code review" or "software testing" is really easy to
get into trouble with the team members, no matter how polite or careful
you are. Why? Because:

Nobody likes his code to be found has "error". It is completely OK for the
systems to be found behave incorrectly one or two years later, because it
is different branch people (or even different company) to debug and
maintain the software. Even they found that bug, nobody will pay attention
to who was the original author of that Java class and that person should
be blamed. But it will be very bad in the same team, when he just finished
his code and tell him that something is not right. He will say "Thank you"
to me. But I will never expect good mood between him and me.

So, no matter what textbook about program testing says, ("non-biased",
"thoroughly", ....), practically, I would just pretend I have reviewed the
code, I have tested it and let the program go. (I will point out one or
two extremely light errors to the author.) The author will get what he
wants, fame or whatever, I get what I want: no enemy and keep my job.

My god! where do you work? Nazi Germany?

 

[a24900]

I have found that "code review" or "software testing" is really easy to
get into trouble with the team members, no matter how polite or careful
you are. Why? Because:

Nobody likes his code to be found has "error".

Maybe you don't like it. The good programmers say "thank you", and
they mean it. Then they fix the code. Why, because they know it is
better a colleague finds a problem and gives one a chance to fix it,
than the boss having a word with one about the "constant lack of
quality" in the code.

It feels much worse if you are the one whos bug blew the deployment of
the software at the real big and important customer, than getting a
hint from a colleague.

But it will be very bad in the same team, when
he just finished his code and tell him that something is not right. He
will say "Thank you" to me. But I will never expect good mood between
him and me.

Well, if a clueless newbie, e.g. one who thinks he is a programmer but
doesn't even know about code reviews, comes up with alledged bugs
every half an hour, and it turns out these are usually no bugs or
irrelevant (outside of specification), or a matter of taste, that can
certainly affect the mood.

If it is a colleague who has the respect of his team, e.g. knows what
he is doing and who doesn't brag about it if he found a bug, then the
mood will actually get better.

So, no matter what textbook about program testing says, ("non-biased",
"thoroughly", ....), practically, I would just pretend I have reviewed
the code, I have tested it and let the program go.

And when the thing blows up, the boss will come back and ask you how
you could miss that obvious fault in the code review. And it is easy
to get caught. The original programmer has an excuse for not seeing an
obvious bug, he is to involved and immersed into the task. You have no
excuse for not spotting things.

Kid, if you are paid for reviewing some code, do it. And do it right.

And if your own code is reviewed and bugs are found, say "thank you",
and fix it. Don't act up.

(I will point out one
or two extremely light errors to the author.)

You are an idiot.

The author will get what he wants,

You have no clue what he wants.

I get what I want: no enemy and keep my job.

No, you are risking your job.

Actually, I should probably encourage you to risk your job. Maybe
you'll leave the programming profession for good. I hate to work with
slimy, artificial agreeableness people who have no spine.

 

[Stefan Ram]

I've heard of teams that do group code reviews, and have the
whole team in a room with a projector going over every line.
That seems insane to me.

Insane to do if someone's life depends on the software?

»[This] is one of just four outfits in the world to win
the coveted Level 5 ranking of the federal governments
Software Engineering Institute

Consider these stats : the last three versions of the
program -- each 420,000 lines long-had just one error
each. (...)

Otherwise, the hour-long meeting is sober and revealing, a
brief window on the culture. For one thing, 12 of the 22
people in the room are women, many of them senior managers
or senior technical staff. [This] group, with its
stability and professionalism, seems particularly
appealing to women programmers.

The central group breaks down into two key teams: the
coders - the people who sit and write code -- and the
verifiers -- the people who try to find flaws in the code.

The two outfits report to separate bosses and function
under opposing marching orders. The development group is
supposed to deliver completely error-free code, so perfect
that the testers find no flaws at all. The testing group
is supposed to pummel away at the code with (...)
scenarios and simulations that reveal as many flaws as
possible. (...)

The developers have even begun their own formal
inspections of the code in carefully moderated sessions, a
rigorous proof-reading they hope will confound the
testers. [The] group now finds 85% of its errors before
formal testing begins, and 99.9% before the program is
delivered (...)«

http://www.fastcompany.com/online/06/writestuff_Printer_Friendly.html

 

[Stefan Ram]

Nearly universal, in any good dev team. Failure to do code
reviews would be one sign of a sick process.

Regarding the attitude towards blaming:

»Importantly, the group avoids blaming people for errors.
The process assumes blame - and it's the process that is
analyzed to discover why and how an error got through. At
the same time, accountability is a team concept: no one
person is ever solely responsible for writing or
inspecting code. "You don't get punished for making
errors," says Marjorie Seiter, a senior member of the
technical staff. "If I make a mistake, and others reviewed
my work, then I'm not alone. I'm not being blamed for this."«

http://www.fastcompany.com/online/06/writestuff_Printer_Friendly.html

 

[Wojtek]

www wrote :

I have found that "code review" or "software testing" is really easy to get
into trouble with the team members, no matter how polite or careful you are.

Have you ever worked as a tester?

When you find a bug, you must go to the author of the code and inform
them that they made a mistake. You quickly learn to be really tactful.

This is the same situation.

Words really make a difference. Rather than "You idiot, what are doing
here", say "I think we need to look at this again".

And your team leader can make a big difference by not allowing personal
remarks to be said, from both sides.

If everything is kept at a professional level, it actually goes quite
well. Everyone must remember that they will be in the hot seat next
meeting.

 

[=?ISO-8859-1?Q?Arne_Vajh=F8j?=]

My boss just told me a concept I didn't know. (I am middle level Java
programmer.) -- "code review".

According to what I heard, "code review" is somebody reads the thousands
lines of code written by other person and try to find if there are some
errors (logic errors, I guess, since the code at least can be compiled
and run).

I feel this is crazy!!! Since the reviewer has to "read" the original
code author's mind and make sure the code does what the author wants and
no hidden surprises! How this could be possible?! This would be
extremely time consuming and nobody knows better about the code than the
author.

My boss says this is very common practice in software engineer development.

Is this true? Or my understanding from my boss is wrong?

Yes. It is very common.

What makes it work is that:
- you can see if the code follow company coding standards and
conventions without understanding what it does (this part is
so trivial that it can even automated and done by software)
- you can see if the code is well structured without understanding
what it does (this part is so trivial that it can be automated
and done by software)
- the reviewers check whether the code does what it is supposed to
do by comparing the actual code with design documentation and
comments in the code

Arne

 

[=?ISO-8859-1?Q?Arne_Vajh=F8j?=]

www said:

Why not ask your boss to clarify exactly what he means. Often management types
use terms they've picked up without having a clue to what they mean at all.

Boss: I think we should use an SQLServer.
Employee: What colour ?
Boss: Blue - they have most RAM.

(I think it is from an old Dilbert stripe)

Arne

 

[=?ISO-8859-1?Q?Arne_Vajh=F8j?=]

Thank you all for your replies. Now, I have learned a lot about "code
review" and software testing.

Now, I hope to switch the topic to a different one, but related:

I have found that "code review" or "software testing" is really easy to
get into trouble with the team members, no matter how polite or careful
you are. Why? Because:

Nobody likes his code to be found has "error". It is completely OK for
the systems to be found behave incorrectly one or two years later,
because it is different branch people (or even different company) to
debug and maintain the software. Even they found that bug, nobody will
pay attention to who was the original author of that Java class and that
person should be blamed. But it will be very bad in the same team, when
he just finished his code and tell him that something is not right. He
will say "Thank you" to me. But I will never expect good mood between
him and me.

So, no matter what textbook about program testing says, ("non-biased",
"thoroughly", ....), practically, I would just pretend I have reviewed
the code, I have tested it and let the program go. (I will point out one
or two extremely light errors to the author.) The author will get what
he wants, fame or whatever, I get what I want: no enemy and keep my job.

It is a potential problem.

But a good technical leadership will be able to solve the problem.

Key points are:
1) teach everybody that criticism is a natural way to improve and
that as professionals they should embrace the idea
2) keep the review in a good tone

re 1)

If they are true professionals then they will understand. If not then
maybe they should go in another direction careerwise.

re 2)

"You stupid idiot you forgot to test for null in method X !"

and

"Could we improve the code by testing for null in method X so we can
handle sitiation where ... ?"

will probably be perceived differently.

Note that in some environments (typical males below 30 only), then
the tone may be very harsh but then the one making the mistake gives
beer during Friday evening drinking and Monday morning there are
no hard feelings (maybe some hangovers).

Arne

 

[Eric Sosman]

...

I don't know whether I've just been lucky, but for the 32 years I spent
in the computer industry, I was always in teams that had the "We are in
this together, and all want it to work" attitude.

A decade or so ago there was a movement called "egoless
programming," which encouraged programmers to write and fix
and improve "the" code rather than "my" code. I never worked
at a shop where this notion was enshrined in formal procedure,
but the best programmers I worked with all shared the notion
that the code was "ours" and not "mine."

I think it's a healthy attitude, and a differentiator
between professionals and hobbyists.

 

[Harald Kästel-Baumgartner]

My god! where do you work? Nazi Germany?

Oh, you missed a lot the last 60 years.

A special test for you: Who is Prime Minister?
( ) Blair
( ) Churchill.

Do you know it?

 

[Ed Kirwan]

Sze Wong
Zerion Consulting
http://www.zerionconsulting.com
See my blog at: http://szewong.com

Sze, I notice you've password protected all your blog entries.

An interesting strategy ...

..ed

 

[TobiMc3]

I 2nd that. It sounds like your boss picked up the term somewhere.

I agree with Patricia's comments. Additionally, code reviews can help
you learn other easier ways of accomplishing things-because your peers
can give you suggestions on technique.

Tobi

 

[Lew]

Oh, you missed a lot the last 60 years.

A special test for you: Who is Prime Minister?
( ) Blair
( ) Churchill.

I believe that Richard was being metaphoric, referring to the apparently
extremely oppressive nature of the OP's workplace where the OP would be afraid
to speak truth even when it is their job to do so. I do not believe that
Richard literally thought the OP worked in the actual historic Nazi Germany.
Richard's comment was meant to draw a parallel through metaphor.

And it doesn't look like Tony Blair is going to be able to hold his seat much
longer. Something about the British public blaming him for involving the UK
in George Bush's Middle East fiasco of oppression and hegemony.

Hmm, who is the President of the United States:

( ) George W. "Dubya" Bush
( ) Adolf Hitler
?

To the OP -
Don't be a coward. If your job is to tell the truth, tell the truth. If you
aren't willing to do that then you are not being responsible, and in fact
would deserve the contempt some have expressed over the idea of not doing a
code review properly.

 

[Harald Kästel-Baumgartner]

....
I believe that Richard was being metaphoric, referring to the apparently
extremely oppressive nature of the OP's workplace where the OP would be
afraid to speak truth even when it is their job to do so. I do not
believe that Richard literally thought the OP worked in the actual
historic Nazi Germany. Richard's comment was meant to draw a parallel
through metaphor.

Hmm... I agree. I didn't catched the metaphor.

--harald

 

[B]

Thank you all for your replies. Now, I have learned a lot about "code
review" and software testing.

Now, I hope to switch the topic to a different one, but related:

I have found that "code review" or "software testing" is really easy to
get into trouble with the team members, no matter how polite or careful
you are. Why? Because:

Nobody likes his code to be found has "error". It is completely OK for
the systems to be found behave incorrectly one or two years later,
because it is different branch people (or even different company) to
debug and maintain the software. Even they found that bug, nobody will
pay attention to who was the original author of that Java class and that
person should be blamed. But it will be very bad in the same team, when
he just finished his code and tell him that something is not right. He
will say "Thank you" to me. But I will never expect good mood between
him and me.

So, no matter what textbook about program testing says, ("non-biased",
"thoroughly", ....), practically, I would just pretend I have reviewed
the code, I have tested it and let the program go. (I will point out one
or two extremely light errors to the author.) The author will get what
he wants, fame or whatever, I get what I want: no enemy and keep my job.

You really don't work in a very good company/team if this is how is works.
Either your place of work really, really sucks, or you have completely
misunderstood something.

 

[Oliver Wong]

To the OP -
Don't be a coward. If your job is to tell the truth, tell the truth.
If you aren't willing to do that then you are not being responsible, and
in fact would deserve the contempt some have expressed over the idea of
not doing a code review properly.

I'm not sure it's all that simple. The OP seems to believe that
telling the truth may cost them their job, and maybe their financial
situation is such that they can't afford to be jobless right now. Better
to suffer the contempt of a few people on newsgroup than to be homeless
for a few months, and get a really bad credit rating when you fail to pay
off your credit cards, right?

I think the optimal strategy for the OP really depends on a multitude
of factors, such as the psychological group dynamics of his coworkers and
bosses; too many factors, in fact, to probably ever properly summarize in
a newsgroup posting. In the end, we can offer the OP information (e.g.
that there exists some work environments where you can perform actual code
reviews without endangering your own employment), but it's up to the OP to
make the final evaluation of what to do.

- Oliver

 

[tjmadden1128]

Nobody likes his code to be found has "error". It is completely OK for
the systems to be found behave incorrectly one or two years later,
because it is different branch people (or even different company) to
debug and maintain the software. Even they found that bug, nobody will
pay attention to who was the original author of that Java class and that
person should be blamed. But it will be very bad in the same team, when
he just finished his code and tell him that something is not right. He
will say "Thank you" to me. But I will never expect good mood between
him and me.

I would be embarrassed, and try to modify my habits to not do the
error again, but I would not blame the person who found it. I've never
had anyone get mad because I found a problem. Sometimes they might
want me to demonstrate the problem, or explain what was found. The
worst case I've seen is reviewing code for offshore developers who
insisted on keeping the old code in the source and commenting it out.
This led to methods that were hundreds of lines long, most of it code
that was commented out.
I've done tons of code review; as a senior developer on a project, I
reviewed code from new hires to make sure it met the coding standards
for the project, looked for any pitfalls peculiar to the project they
may not know about, and to make sure it met the request. I've done
formal reviews where a group gets together to go over changes to a
framework. I've done pair programming, although I don't find it very
productive. I get bored as the reviewer, and my attention wanders. As
the coder, I feel a little creepy having someone watch over my
shoulder. But that's just me.

So, no matter what textbook about program testing says, ("non-biased",
"thoroughly", ....), practically, I would just pretend I have reviewed
the code, I have tested it and let the program go. (I will point out one
or two extremely light errors to the author.) The author will get what
he wants, fame or whatever, I get what I want: no enemy and keep my job.

Which in any organization I have been a part of would get you called
to task and hauled into a manager's or CEO's office. If your
organization does not track who did the review and ask why they did
not find the problem, then there is no point in doing it or even
pretending. In rare cases the problem would get your companies name
splashed across the media. In all cases, this undermines the
confidence of management in your team, makes the rest of the code
suspect, and loses business for the company.

Tim

 

[www]

Thank you all for the replies and discussions. I consider that most of
them are healthy. I really appreciate your time.

From the replies, I have learned some key words or new concepts, e.g.
"egoless programming", or related web links. From them, I have learned a
lot issues in software engineering or become aware of their existence.
These issues are hard to discuss openly in my working place and they are
not taught in Computer Science class rooms. Fortunately, through the
newsgroup, I can ask and learn here. In the end, I just want to cite one
of many things I have read from the materials provided by one replier:

<quote>
How do they write the right stuff(software)?

The answer is, it's the process. The group's most important creation is
not the perfect software they write -- it's the process they invented
that writes the perfect software.
</quote>