What is code review? (Java code review)

[www]

Hi,

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?

 

[Stefan Ram]

According to what I heard, "code review" is somebody reads the thousands

Ever heard of search engines or Wikipedia?

 

[David Gillen]

www said:

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

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.

D.

 

[Patricia Shanahan]

Hi,

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?

I've only participated in informal code reviews, in which the objective
is not to look at every line, but to find the lines that should be
looked at.

There is often a mix of two high level objectives:

1. General search for errors.

2. Verification of code quality.

Both aspects should focus on problems that cannot be detected by the
software development tools.

For example, I would spend far more time on synchronization and
multi-threading issues than on aspects where bugs would be found in the
first few minutes of testing. There is no point in spending human time
checking whether the indentation is correct, but no computer program can
evaluate whether identifiers are meaningful.

Remember that readability is an important aspect of code quality. Code
that can only be maintained by the author is of limited value in most
organizations. I don't think there is any automated test for readability
that works anywhere near as well as seeing what happens when a
non-author programmer tries to read the code.

This is a good time to review your documentation imagining another
programmer trying to understand your code.

Patricia

 

[Daniel Pitts]

Hi,

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

The code review is more about verifying that the design is correct,
but it does often find logic errors as well.

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.

That's not necessarily true. An author might have a good feel for the
code at the immediate moment, but what happens down the road a year or
two when you haven't thought of it for that long... You'll have to
time travel and read your own mind... But not really, if you write
well designed code, the code will convey your intentions (such as by
method name, variable name, etc...) and what you actually did (its not
difficult to step through a method mentally). A good code reviewer
would ask you questions like "You named this method getBars, when if
fact what you seem to do is get wibbles. Is it named appropriately?"

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

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

Code reviews are common practice and a good idea. You're boss is
correct, but your thoughts are not.

I wouldn't call yourself a middle level Java programmer quite yet. I
consider middle level someone who can convey meaning in there symbol
names, as well as someone who knows what a code review is.

 

[alexandre_paterson]

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

It's not just about parsing thousands of line of code but, yup, you
can do that. And it's *crazy* what an "OK" programmer will find in
code produced by some not-so-OK-yet programmer.

I've done "dumb eye ball searching" code review (literally reading
lots of lines of codes one by one) and what you find is usually
pathetic.

I've been working for a startup where a programmer had a "working"
program in which a method had 2000 lines of code.

2000 lines of nested "if / then / else" with basically the same
code copy/pasted and nested and nested several times.

(this "method" ended up being rewritten in about 5 methods /
130 lines of codes)

Crazy.

And that guy had a diploma and passed the interview...

Worst I've seen was probably in some source code
for a medical appliance where the manipulation of
dates showed a complete non-understanding of both
programming and the API available.

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?!

Well for a start the reviewer will see if the code is readable
at all. If only the author can read what some piece of code
does then the company is in big trouble.

Note that I won't call names but there's a specific
part of the programming community that do use that
age old trick of "writing unreadable code to keep their
job".

And it appears quite obviously when doing code
reviews. Companies don't need such jerks.

... and nobody knows better about the code than the
author.

But lots of programmers knows for sure a better
way to do what the code of that author does.

Been there, done that.

 

[Oliver Wong]

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.

If the review has to read minds, and this is impossible given the
level of documentation and choice of identifiers in your code, then your
code is probably poorly written.

Consider the following code snippet:

public int a(int b) {
return b;
}

Does this do what the author intended? Are there any hidden surprises?
It's impossible to tell, because the author did not write any
documentation, and chose meaningless identifier names. Contrast with this
code:

public int hashInt(int intToHash) {
/*
* Implementation is just to return the same
* int. This is a perfect hash.
*/
return intToHash;
}

Now, thanks the the name of the identifiers, and the comments, we can
guess what the author was trying to do, and check that it does in fact do
what the author intended.

Code reviews correctly would flag the first example as needing to be
rewritten.

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

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

It's common. I don't know about "very" common, but it's common enough
that I'd expect someone referring to themselves as a "middle level
programmer" (whether Java or some other language) to know what the term
"code review" refers to if I brought it up.

As an aside, to help re-assess your self-evaluation, I'd also expect
someone referring to themselves as a middle level programmer to be
familiar with the following terms. If you're not familiar with them, you
may wish to do some more reading.

* Design patterns
* Extreme Programming
* Test Driven Development
* Unit Testing
* Black Box Testing
* Model View Controller
* Singleton
* Source Version Control
* Pair Programming
* KISS
* UML
* Class Diagram
* Sequence Diagram

Note that for the terms that refer to methodologies, I don't expect a
middle level programmer to actually *practice* all of the above
methodologies, but rather that they should be familiar with what the terms
refer to.

- Oliver

 

[Oliver Wong]

[anecdote about a really bad programmer]

And that guy had a diploma and passed the interview...

IMHO, a diploma doesn't really mean anything. GPA is pretty
meaningless to me too. If I have a candidate fresh out of university, I'd
be more interested in seeing a list of classes she took (did she take a
compiler course? a course on AI? etc.), and then I'd ask her during the
interview about those classes (what projects did she work on for those
courses?)

- Oliver

 

[rossum]

Hi,

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?

Think of a code review as an opportunity for you to improve your
programming skills. Your boss is letting you take some of your
colleagues time to comment on your code. That lets you pick their
brains as to how you could code better. How much would it cost you to
hire a consultant to look at your code and suggest ways to improve it?

rossum

 

[Sherm Pendley]

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!

If ESP is required to divine the code's purpose, that's already the first
sign of a problem. There should be:

a. A written specification
b. Unit tests
c. Comments within the code itself

Furthermore, it shouldn't be delayed until thousands of lines of code have
already been written. It's an ongoing, continuous process, not a checklist
item that must be checked once before shipping. In "pair programming", this
actually involves two programmers sitting in front of one keyboard, working
together and reviewing one another's code.

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

What if the author gets hit by a meteor? Code *must* be readable and main-
tainable to be useful. If the reviewer can't read the code, it fails the
code review.

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

Your boss is correct.

sherm--

 

[Patricia Shanahan]

If ESP is required to divine the code's purpose, that's already the first
sign of a problem. There should be:

a. A written specification
b. Unit tests
c. Comments within the code itself

d. The non-comment code

Essentially, all code should be treated as communication to two
audiences with very different needs. The compiler just cares that an
identifier follows the language syntax, but "xyzzy" is just as good as
"columnNumber". The other audience is programmers, including the
author's own future self, three years later after writing a few thousand
lines more on other projects.

In addition to identifier selection, there are often multiple ways of
coding something, some of which make intent and meaning clearer than others.

Looking on the optimistic side, suppose the code is really well written,
and would be easy for another programmer to maintain. The code review is
the chance for the boss to get the good news.

Patricia

 

[szewong]

d. The non-comment code

Essentially, all code should be treated as communication to two
audiences with very different needs. The compiler just cares that an
identifier follows the language syntax, but "xyzzy" is just as good as
"columnNumber". The other audience is programmers, including the
author's own future self, three years later after writing a few thousand
lines more on other projects.

In addition to identifier selection, there are often multiple ways of
coding something, some of which make intent and meaning clearer than others.

Looking on the optimistic side, suppose the code is really well written,
and would be easy for another programmer to maintain. The code review is
the chance for the boss to get the good news.

Patricia

I've been an Architect for many years and code review is always
something that is difficult to do right. I have tried from formal code
review meetings to reviewing CVS check-ins daily, to my latest 'pair
and monitor' approach. Basically as many have pointed out, this is a
necessary process to ensure the right design, coding partice are in
place. Code reveiw is supposed to make the team more aware of what one
another is doing and improve code quality as a whole.

Anyway, I'm always for light process. And that is what right now I'm
heavily relying on 'pair and montior'. By pairing developers, you have
a higher chance of having better documented code. We then have one of
two team members who's sole responsibility is to monitor everything
that goes into CVS and flag anything that disagree with our
development handbook. So far this is a low impact approach that seem
to be working.

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

 

[Richard Reynolds]

Hi,

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?

I find that you've never heard of code reviews very strange, I've never
worked anywhere where they didn't do them in one form or another, sometimes
very useful, depends how good your process is.
If you think reading someone else's code is crazy, how do you manage to
maintain any code? Do you rewrite everything you do from scratch!?

 

[Eric Sosman]

Patricia Shanahan wrote On 05/10/07 14:37,:

d. The non-comment code

Essentially, all code should be treated as communication to two
audiences with very different needs. The compiler just cares that an
identifier follows the language syntax, but "xyzzy" is just as good as
"columnNumber". The other audience is programmers, including the
author's own future self, three years later after writing a few thousand
lines more on other projects.

In addition to identifier selection, there are often multiple ways of
coding something, some of which make intent and meaning clearer than others.

Looking on the optimistic side, suppose the code is really well written,
and would be easy for another programmer to maintain. The code review is
the chance for the boss to get the good news.

Another benefit of code review is the exchange of
ideas between author(s) and reviewer(s). "What you've
got will work, but if you used two BitSets and a HashMap
you could avoid most of the database queries." "This is
fine, but it would be easier to internationalize (should
we ever decide to) if it were refactored thusly: ..."

Or even "Hey, this is slick. Y'know, in a review
last week we saw some code where Zaphod solved a problem
a lot like this one, but this is better. If you'd make
changes X,Y,Z we could use your code in his project, too."

The main thing is to approach a review -- in either
role! -- with a peculiar mixture of pride and humility,
not with an amalgam of arrogance and apprehension.

 

[www]

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.

 

[Mark Rafn]

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

Sure, but you should do it when it's hundreds of lines of code, not thousands,
and one of the things you're looking for is enough documentation of what is
intended and how it works that you can follow it.

Think of it as "is this code well-organized, documented, and useful enough
that I'm willing to maintain it when the author is gone?"

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!

Well, if you have to read their mind, your review is "this doesn't pass my
review - you need more design and functional comments so I can tell what
you're trying to do and why."

How this could be possible?! This would be extremely time consuming

Not compared to having to figure it out or rewrite it later, after the
author has left the company.

and nobody knows better about the code than the author.

This is one of the primary failures the code review helps prevent. Writing
code for future maintainence is difficult, but for anything worked on by more
than one or two people as a hobby, it's the single most important aspect of
the work.

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

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

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.

Having another dev look at your code and suggest improvements to style,
patterns, documentation, and functionality is beneficial to you, the other
dev, and to the codebase.

 

[Sherm Pendley]

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.

If you think that habit will ensure job security, think again. If I had
an employee who was doing what you describe here, I'd fire him on the spot.

Do your job and review the code thoroughly - the "my code is perfect" prima
donnas won't be there long enough to make your life difficult anyway.

sherm--

 

[Daniel Dyer]

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.

It can be a delicate situation, and it would depend to some extent on how
you go about it. Everybody needs to understand that the reviews are for
everybody's benefit and that everybody makes mistakes, even the guru
programmers. Perhaps if everybody gets a chance to review somebody else's
code it won't be so bad. Then at least everybody experiences it from both
sides. If the junior programmers aren't ready to conduct a review by
themselves, maybe they could assist a more senior colleague. This has the
advantage of helping them to learn from other people's mistakes as well as
their own. I think it also helps if the reviewer is somebody who is
respected by the author of the code. Also, the wounded pride that comes
from somebody finding problems in your code can be good motivation to
improve things for next time.

Steve McConnell has a section on formal reviews in his book, "Code
Complete". He also devotes a chapter to "Personal Character", which
addresses the issue that you have just raised:

"The people who are best at programming are the people who realize how
small their brains are. They are humble. The people who are the worst at
programming are the people who refuse to accept the fact that their brains
aren't equal to the task. Their egos keep them from being great
programmers. The more you learn to compensate for your small brain, the
better a programmer you'll be. The more humble you are, the faster you'll
improve."

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.

In the long run that's counter-productive. Eventually you'll have to
maintain that code. It's better to catch the bad bits early before they
get out of control. If you spot problems with a colleague's code, just
let them know in private - you don't have to get up in front of the whole
team at your next meeting and declare that person to be incompetent.
Alternatively, if you don't want to single anybody out, you can raise
general problems in front of the whole team without referring to specific
examples. E.g. "I've noticed a few places where we've been over-riding
equals without over-riding hashcode - this could cause us problems
because...." Then you've raised the issue and anybody who was ignorant of
the implications will now know better without any embarrassment.

Dan.

 

[Oliver Wong]

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.

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

We seem to realize that everyone makes mistakes, so that such
corrections are not so embarassing as to cause tension in the worksplace.
I realize, though, that not everybody is like that, and you don't always
get to choose who your coworkers are.

Pair programming makes this a bit easier, because the feedback is
immediate, and we may console ourselves with "Well, even if he wasn't
around, I probably would have spotted that NullPointer bug eventually
anyway, given a few more seconds of staring at the code...", and the code
can be credited to both of you, so that you both share in its quality (and
its blame). But again, your boss may not be so keen on the idea of pair
programming.

I hope, for your sake, that you're simply underestimating your
coworkers' maturity.

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.

I don't think anyone decides to become a programmer because they think
that'll make them famous. "Microsoft Word" is a very famous application.
Who wrote it? I have no idea. Google is a very famous web application. Who
wrote that? I don't have a clue. In fact, I suspect in both cases it
wasn't one specific person, but a team of people, further diluting the
fame of any one programmer within that team.

I'll also point out that if the boss gives you the task of review a
piece of code, and later on, a bug is found in that piece of code, it will
be both the fault of the original author of that code for writing the bug,
and your fault for not having caught it as reviewer. If this happens once
or twice, it's not a big deal (bugs slip through; that's just how software
development is), but if the code you review is consistently buggy, your
boss may feel you are not very competent in terms of code reviewing, and
that may have adverse effects in the future.

- Oliver

 

[Richard F.L.R.Snashall]

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.

Don't you want to know your code will work correctly? To me, that
has always been paramount. Who cares how it got that way?