Fog Creek Software
Discussion Board




Code Reviews - Does Anyone Actually Do Them

I was wondering if anyone works for a company that actually does code reviews. I've been trying to get them implemented at my work, but keep meeting resistance along the lines of: "we don't have time to spend on such things".

Meanwhile we keep producing buggy code and missing schedules. So we code faster and produce even more bugs that we have to fix and then we miss the schedules anyway because we have to fix the extra bugs. Etc. I'm sure everyone is familiar with this cycle.

So I was hoping to use code reviews to help people to code more efficiently and more reliably. But of course, we have no time for such things...

RB
Sunday, December 08, 2002

I have been recently hired as a contractor, by a clinical had-held software company to do the code review of thier products. It's kind of unique case though. They don't have any in-house developers and thier product was developed by another outside software developing company. Now they want me to go through the code and make sure everything is in order!

Yaniv
Sunday, December 08, 2002

My company does.

I will be at a code review tomorrow (Monday).  I look forward to it.

Also, I send out emails to Engineering and posts things to our SharePoint Server on a fairly constant basis.

I think it's healthy to share your thoughts on the code everybody work on.

William C
Sunday, December 08, 2002

My company requires them because the company is trying to be ISO 9000 complient.  But the required formal code reviews are a waste of time for two reasons:
1. We have a good team. Long before the code review we drag passer-bys in to look at the code.
2. There is no follow up after a code review.  The bosses will say do this and do that (some of it good, some of it dumb).  There is no checking later that any of the required changes are made.  But we are glad they do not check up on the dumb part!  We don't do it.

A. Coward
Monday, December 09, 2002

I agree that formal code reviews tend to be time sinks. They also tend to be the first thing to be tossed when time gets tight. Most teams I've worked on did informal code reviews (of the drag the passerby in sort). These were a mixture of very strong teams, and some mixed teams.

You don't need any management buy-in or lots of extra time for informal code reviews, and you get most of the benefits of formal reviews. Plus they are peer-based so you avoid the resentment issues that A. Coward brings up. Want to start them? Simply find a co-worker and start reviewing one another's code.

I've also had good luck on some teams with code review checklists. Grab an experienced programmer on the team and tell them you're going to add a new feature. They'll say "ok, watch out for X, Y, and X". Write these down and use them as a quick checklist during code reviews. For a program like Microsoft Word, you might have items like "did you consider what happens in non-English languages", "did you consider macro support," and "did you consider undo".  This is a painless way to weed out common classes of bugs.

Malcolm
Monday, December 09, 2002

Having my code reviewed by a team of inferiors is a waste of time and money.

Seriously.

Guru
Monday, December 09, 2002

We've started code reviews at my company, and they end up helping the junior people the most. I generally mention several way that they can improve their code. The less experience employees can learn good programming practices when reading other people's code.

When more experienced people review each other's code, the benefits are less pronounced. They can still learn some good tips, understand what the other people are doing, and uncover areas that could use more explicit guidelines.

Code reviews definitely provide more benefit, per unit effort, than creating test cases and they're much less annoying.

Julian
Monday, December 09, 2002

Loads of Code Reviews seem to be done because the software developers / managers have read somewhere that it's good to do them or because some certification/customer requires them.

Applying best practices is good. But applying them without understanding the implications was called Cargo Cult Science by Richard Feynman. Steve McConnell has a good article about Cargo Cult Science in Software Development:

http://www.stevemcconnell.com/ieeesoftware/eic10.htm

Richard Feynman's original paper has been posted here already a couple of times but it's worth it:

http://www.lhup.edu/~dsimanek/cargocul.htm

Jan Derk
Monday, December 09, 2002

We don't do code reviews, but I think it would help
the company very much.
There are some programmers who just have read a book
about C++ and now they are coding and they are
producing nearly gargbage. They could learn very
much from code reviews.

I'd be interested if someone as a non chef has mastered it
to introduce code reviews.

AW
Monday, December 09, 2002

Every time I've doen them they've been a pain in the neck. It turns into a geek fest with the nerds looking for spelling mistakes in the comments and lines that aren't indented properly rather than checking the code does what it's supposed to.

Partly the problem is that they're called CODE REVIEWS not PROJECT REVIEWS so no-one checks the code matches the spec. The other problem is the level of immaturity in developers: they want to have some impact so half the changes will be renaming variables "because they don't like them".

Technical reviews here involve getting yelled at for missing the comparison off IF statements. Yes, here we really do have to write C++ that says:
  if (go_do_something() == TRUE)...

And every class needs to have an argument held about whether one "really" needed to create it and whether it makes the code "too complicated".

Frankly I'd be happier if they were called PEER REVIEWS and there were some peers involved. As it is, they're just make-work that gets done because it sounds good.

Katie Lucas
Monday, December 09, 2002

I'm also coming to think that informal reviews are great.  People should seek out those who have the best opinions to improve their code.

Recently, I was working on a personal project, and my friend was excited about me looking at his refactoring.  I think that's the best situation, when you write something and want an audience.  (Programmers are authors too -- no doubt Literate Programming was motivated by this feeling.)

Tj
Monday, December 09, 2002

I'd say that to be successful, code reviews should follow a similar informal but useful manner that Joel recommends writing functional specs in.

Generally what I do is when somebody checks some new functionality into source control, I'll have a look through it, test it quickly on my machine and see if I can spot any obvious mistakes. I'll then point these out to the appropriate developer, which normally only takes a few minutes.

Also, if possible, rather than just coming up with a list of mistakes, I give the developer a chance to see if they can tell me what's wrong or what can be improved. Because they have effectively spotted their own improvements, they should feel better, rather than if somebody else points them out to them. (Nobody likes a smart ass...)

Ritchie Swann
Monday, December 09, 2002

On my latest engagement, we did "formal" code reviews -- the process consisted of the developer finding 3 peers somewhat familiar with the area of development, submitting the code to those individuals, and those individuals, within a few days time, filling out -- electronically -- a list of problems within the code.  The developer then had the responsibility of addressing all the problems - not necessarily fixing, but addressing either by fixing or writing down why no code change was necessary.

This approach was "formal" in the sense that the fact that the review was performed, and the results of the review, were documented.  It was lightweight, because it didn't require any meetings, and the overall time commitment was fairly low.  And it was successful: bugs in the code were found.  Not all bugs, certainly, but significant bugs that would have been harder to find in testing.

I have no idea why code reviews aren't performed more routinely -- if people are given the simple instructions that the purpose is to find bugs in the code (not spelling errors, not style issues, not anything but bugs), I don't understand why they can't always be effective -- unless, I suppose, development teams are already seriously dysfunctional, and no one trusts one another to comment on their code.

somebody
Monday, December 09, 2002

Guru, if you state something like this, you are not a Guru but a Bob!

If you don't know what I am talking about, go to:

http://www.gotw.ca/conv/index.htm

to find out.

Have fun,

Jutta Jordans
Monday, December 09, 2002

Well, one possibility, that I've encountered in the past, is that nobody is interested in doing the code review, because they're busy trying to finish their own part of the project on time.

I've only had successful interest in reviewing code where the reviewer is going to be using the reviewed code on a regular basis (eg: to develop or test against it).

Ritchie Swann
Monday, December 09, 2002

btw, pair programming is useful when working with newbies to your project.  That's in the sprit of code review.

Of course, I envy opensource programmers who occasionally have rows on the direction the code is taking.

Tj
Monday, December 09, 2002

Thanks for your replies.

I think you've talked me out of code reviews. Unless I can think of a way of doing them informally and quickly. Peer review sounds better, but will probably only work with like-minded programmers. No-one else will want to spare the time.

Another grand scheme bites the dust.

RB
Monday, December 09, 2002


Pair programming.

Review the code BEFORE you write it.

Do it. Now.

Bruce Rennie
Monday, December 09, 2002

Unless I can think of a way of doing them informally and quickly

--> If you have visual Sorce Safe, you can always pick a junior guy and DIFF his check-ins for bugs.

For VERY junior C++ programmers, you catch a lot of errors that way.  As they get more experienced, you begin to get into style/opinion issues and that can be bad.  With more experienced developers, you need to get multiple people in a room, and that can be hard to do ...

Matt H.
Monday, December 09, 2002

I've worked on a variety of teams that did code reviews. They can be valuable if they're done the right way. Here are some things I think are absolutely essential for a good code review:

People need to read the code before the review meeting.

Reviewers need to recognize that there's a difference between "incorrect" and "not implemented exactly the way I would have done it."

There needs to be a firm time limit on the review meeting and someone needs to watch the clock and keep things moving.

The person whose code is being reviewed needs to listen carefully rather than attempting to defend their work. It's also helpful if the person whose code is being reviewed comes into the review with some ideas about where the code could use improvement, so that they can ask for suggestions in those areas.

Beth
Monday, December 09, 2002

We used code reviews at the last place I worked.  I found them helpful, and especially useful for junior and intermediate programmers.  I'd like to get them implemented where I currently work.

For the senior people, the code reviews usually boiled down to making sure the code following the companies programming style guidelines (which were not excessive!).

I would recommend them, but I'll echo some other people here.. don't make them too formal, and always assign one senior person in the group to follow up and make sure that the requested changes are complete.

Gerald Brandt
Monday, December 09, 2002

Guru wrote:

"Having my code reviewed by a team of inferiors is a waste of time and money."

I think I see the problem here....


Anyway, most companies don't do code reviews because they have tried them in the past and failed. Of course, the reason that they have failed is because they didn't know how to do them and the code review degenerated into a battle of egos and programming styles.

Unless you have a solid manager who corrals the egotistical moron who refuses to accept any advice and thinks only *his* code is quality, all other code is inferior, then they will always be a waste of time.

And of course if you have a bunch of nitwit programmers and a couple of super stars, then it will probably also fail because of the gap in experience.

But...if you have a good manager, and competent programmers who are mature and professional enough to leave their egos at the door then code reviews are great way to speed up the development process.

Mark Hoffman
Monday, December 09, 2002

Guru wrote:

"Having my code reviewed by a team of inferiors is a waste of time and money."

ALWAYS (note the capital letters),  ALWAYS invite junior or intermediate developers to a senior developers code review.  It's an excellent form of mentorship.

Gerald Brandt
Monday, December 09, 2002

I like having 'inferiors' review my code. Why? Because usually the person who catches the most mistakes in code reviews is me. The process of explaining what I've done makes me see more errors than most people can catch when reading code.

Plus the 'inferior' gets to learn what's going on, so they can apply the knowledge to their work.

Then again, I'm always seeking valid criticism. I hated turning in a paper for a class and getting back, say, a B. Why was it a B? What should I have improved? I threw the thing together at 2AM before it was due, why wasn't it a C or an F? Same with code--I know I threw it together 5 minutes ago, can you see where it is going to bite me more than I can?

mb
Monday, December 09, 2002

Although it often happens, code reviews should NEVER become personal.  I believe code reviews should be part of every company's standard coding practices.

Code reviews can serve many purposes.  One can review either general techniques or review actual source code to ensure that the code is standardized.  When reviewing techniques, diagrams (i.e. a UML activity diagram or a traditional flowchart) should probably be produced and sent to people several days before the meeting is scheduled to start.

A code review can be used for the following:

* To find ways to make the code more maintainable.
* To teach new members what the team's coding standards are.
* To help find problems in code that is not working properly.
* To help the person's code being reviewed understand/realize alternative methods that can be used to code a difficult section of code.

Team Standards
I am NOT saying that each person cannot write code in their own personal style.  I am saying that a person's individual style should work withing the team's standards.  Issues that need to be raised at a code review should be things that affect the performance, readability, and maintainability of the code.  Issues that revolve around personal preference, that have nothing to do with improving the code or working with the team's standards, should NOT be included in code reviews.

How I believe a review should be conducted
An impartial moderator (such as a PM) should oversee the review and make sure the review does not deteriorate into an attack on the person who wrote the code.

The developer or developers who wrote the code should walk through the code and explain the function of the code and how it is supposed to work.  In general, one should not try to fix problems during the review, only identify them.  If there is time to discuss solutions once all the problems have been found, then solutions can also be discussed.

All suggestions for change should be logged.  I feel it is best if the moderator takes notes and keeps a record of all suggestions made during the review.

one programmer's opinion
Monday, December 09, 2002

Code reviews can be useful if done right.

When working on a project without formal code reviews, I have found it useful to review my own code.  When I have something written and compiled (and maybe running, too), I get a hard copy listing and go through it  with the idea of seeing if it does what I think it should.

mackinac
Monday, December 09, 2002

>>>
Yes, here we really do have to write C++ that says:
  if (go_do_something() == TRUE)...
<<<

You're _required_ to do that?  There are statements like that, or worse, sprinkled throughout the project I am working on.  I take it to be a sign of someone who is quite inexperienced or is having trouble understanding boolean variables.

mackinac
Monday, December 09, 2002

We performed two types of code reviews at my last job.  The first type was used for smaller changes, while the second type was used for large changes (adding a new file, etc.).

The first type of code review was a required single-peer review that was required before checking in any changes.  We had to find another developer and have them review our changes and check off on a sheet.  It was a fantastic process; it took only a few minutes to print out the changes, and only a few more for the peer to look over the changed code.

The second type of code review was a large-scale review, involving at least two reviewers and the manager (who was a highly experienced programmer).  This was a sit-down meeting with the code, where we'd go through it page-by-page and line-by-line looking for problems.

In retrospect, the act of being reviewed were less useful than *having* *reviews*.  If you know that your code will be seen by somebody else, you tend to ensure that your code looks good, is robust, is checking for all the appropriate errors, etc.

In my opinion, code reviews are worth *trying*, and sticking with for awhile.  They're not useful for everyone.  But they're useful in an awful lot of situations.

Brent P. Newhall
Monday, December 09, 2002

I normally try to hold a 'lite' version of a formal inspection instead of a code review, and have it when the code is just starting to become functional. The inspection is limited to a single 2 hour meeting, nobody reads the code beforehand, and you don't waste any time categorizing the bugs. This makes it easier to get reviewers, and to avoid management killing it as too expensive.

Rather than trying to review all of the code what typically happens is a review of some selected areas, a quick design review of the components actual architecture, and a questioning of the actual goals of the component. Sometimes I think the latter is the most usefull because most spec's aren't truly examined (you have better things to do or you react stronger to something when its concrete) and a lot of components don't get any visibility until its too late to change course.

The real problem appears to be that many developers are  reluctant to have any sort of public code review process because they are afraid it will make them look bad. I've only seen it work when management initially demanded it, AND the most influential developers make it the cultural norm. Otherwise you just go thru the motions, and its quietly dropped for the next project/version.

Eric Moore
Monday, December 09, 2002

>>>>>>>>>>
You're _required_ to do that?  There are statements like that, or worse, sprinkled throughout the project I am working on.  I take it to be a sign of someone who is quite inexperienced or is having trouble understanding boolean variables.
>>>>>>>>>>

I see nothing wront with doing this:

if (go_do_something() == TRUE)

and I'm not inexperienced and my code tends to be very solid.

There isn't some award that you win by typing less code.  Let me guess, you do this:

for(i=0; i<j; i++){ foo(i); }

instead of this:

for ( i = 0; i < j; i++ )
{
    // all the spaces you see between parentheses
    // are on purpose!
    foo ( i );
}

because you get to save 3 whole lines -- woo-hoo!

I like to write all my boolean expressions explicitly like that.  In my opinion, it makes the code much easier to read.  Using the extra 3 lines in my for loop example, makes the code easier to read.  If the code is easier to read, simply, there will be less bugs.

Also, explicitly writing TRUE leads to less mistakes.  In the same vain as this causing less mistakes:
if ( TRUE == var )
{
    // blah
}

Just because somebody does something different from you, it doesn't mean they are "inexperienced" or having a "hard time understanding boolean expressions".

C'mon, the ego is sipping right through the web page on that one!

William C
Tuesday, December 10, 2002

Hmm...I have a bad feeling we're about to enter a discussion on how/whether to indent curly braces.

Just wanted to point out that on the compilers I've been using for the past couple of years, writing if (TRUE==f()) hasn't been necessary. You'll get an "assignment within conditional" warning or your code will simply fail to compile if you leave off an "=".

Malcolm
Tuesday, December 10, 2002


Yes. It's required. To the extent that I got bored of having people call me over and go "AH!!! Gotchya!!! You missed the compare there!!!"

So I wrote, in all seriousness, a Perl script to go wrap all the if tests with an ==TRUE on the end.

<sigh>

Sometimes I wonder where people get these ideas from. Of course the coding standards don't have any rationales behind any of the rules. And asking questions like that gets stupid answers and raises my blood pressure.


All the problems in the software world. All the not knowing how to cope with requirements that change daily, with programming environments where things don't work as advertised, with all the management crap, all that - and everyone seems to focus on things like where the brackets are and making sure they're indented with the right combinations of tabs and spaces.

Some days I just despair of this industry and everyone in it. We already lived and died and you and I were the evil in the world and this is hell.

Katie Lucas
Tuesday, December 10, 2002

One of the problems with

if (go_do_something() == TRUE)

is that in C, there is no such thing as TRUE, there is only "not FALSE". For all you know, go_do_something() might return -1 or 193 instead of the actual value TRUE. Most Windows functions are defined that way (success is non-zero).

Frederik Slijkerman
Tuesday, December 10, 2002

William C.

Here's what's, in general, requiring something like:

if (go_do_something()==TRUE) {

In well written OO code, you might have an object called, say, "commandLineInput", with a method, say, "isValid()".  You *want* to write:

if (commandLineInput.isValid()) { ...

That sort of code reads very well.  *Requiring* an explicit "== true" undermines the readability of the code:

if (commandLineInput.isValid() == true) { ...

There may be cases where it makes sens to explicitly test a boolean equality -- but in general it undermines readability.

somebody
Tuesday, December 10, 2002

The first line of the above message should be:

Here's what's, in general, *wrong with* requiring...

somebody
Tuesday, December 10, 2002

IMHO, code reviews can be very powerful. I have used them. I find them most useful if:
- performed by senior team members who also act as a coach
- performed in pairs.
- performend by the programmer (e.g. PSP: personal software process)

When I (in the role of software architect) perform them I
- try to understand; if I am not sure what something means, this likely requires some clarification
- look for dangerous constructions (e.g. Safer C - like)
- check with design documentation
- check for conflicts with other documentation and my knowledge.
For me they are a great coaching moment (definetely not a if (geek()==TRUE){//moment)}  !)

I think coding standards are great as long as they fit on 1-3 pages. Enforcing coding standards is not the primary goal of code reviews (in my humble opinion).

Adriaan van den Brand
Tuesday, December 10, 2002

>>>>>>>>>>

That sort of code reads very well.  *Requiring* an explicit "== true" undermines the readability of the code:

if (commandLineInput.isValid() == true) { ...

There may be cases where it makes sens to explicitly test a boolean equality -- but in general it undermines readability.

>>>>>>>>>>

Usually I'm in the habit of writing it like this:

if ( TRUE == commandLineInput.isValid )
{
    ....

but regardless if the true is on the left or the right, I think it depends on how you read it it.

I would read the statement (either way) as:

"if true, that the commandLineInput is Valid."

As opposed to:

"if the commandLineInput is Valid equals true".

I just think that when you read conditionals, you start with the "if true" before reading the statement.

So yes, if I was reading it like you would ... it would be bad.

Also, I know there has been times where I'm rushing through code and i'll do this:

if ( go_do_something() )
{
    ...

when I mean to do something like:

if ( FALSE == go_do_something() )
{
  ...

When you are writing a conditional, to me, explicitly writing out the TRUE or FALSE makes me really think about ... and thus be more careful about the boolean expression I'm writing.  Thus, another bug averted.  Also, when another programmer is reading my code, by being explicit, they know that I was really going for a TRUE or a FALSE in that situation ... and it wasn't just me forgetting to write FALSE.

To digress, I always write:

if ( FALSE == go_do_something() )
{
  ...

as opposed to:

if ( !go_do_something() )
{
  ...

1) I feel you should always try to avoid "Not's" in coniditionals when possible.
2) The ! is very easy to overlook when scanning through code.

--

I try to write pretty code! hahaha!

Trust me, if you were another programmer reading my code, you would be happy that I am very careful about the way I do things.

And I have found that in my thouroughness in every detail, which is really what is happening by me doing all these quirky things, my code tends to be really bug-free.  Co-workers comment about my bugless code (not saying I don't get bugs, just less than the avg. programmer).

William C
Tuesday, December 10, 2002

>>>>>>>>>>
Sometimes I wonder where people get these ideas from. Of course the coding standards don't have any rationales behind any of the rules. And asking questions like that gets stupid answers and raises my blood pressure.
>>>>>>>>>>

And so you are saying that I don't have any rationale for the reasons that I write:

if ( TRUE == go_do_something() )  ???

There really can be a reason for it.

>>>>>>>>>>>
All the problems in the software world. All the not knowing how to cope with requirements that change daily, with programming environments where things don't work as advertised, with all the management crap, all that - and everyone seems to focus on things like where the brackets are and making sure they're indented with the right combinations of tabs and spaces
>>>>>>>>>>>

So you are saying that because some people aren't good at one thing (requirements, environments, etc.) they should not pay attention to something that can be easily grasped (where the brackets are)?

One should slack off in areas such as this?

Hmmm...

I would continue to focus on "brackets, tabs, spaces" AND try to improve on the other areas that you have identified as lacking.

What do you as a sr. engineer, or what does your company do to try to improve those areas that are lacking?

Besides making fun and getting angry, of course.

William C
Tuesday, December 10, 2002

>>>>>>>>>>
is that in C, there is no such thing as TRUE, there is only "not FALSE"
>>>>>>>>>>

#define

William C
Tuesday, December 10, 2002

>>>>>>>>>>
Yes. It's required. To the extent that I got bored of having people call me over and go "AH!!! Gotchya!!! You missed the compare there!!!"
>>>>>>>>>>

And as I re-read this statement, these are the facts I get:

1) You KNOW that your company or your standard is to write the TRUE in the expression.
2) You continue to NOT do this (most likely purposefully).
3) Then you get angry when people point out that you aren't following a standard (regardless of how silly it is)?

I think the 55 speed-limit is ridiculous, but I should still get ticketed if I'm speeding.

How, in the face of these facts, can you say with a straight face that you aren't doing anything wrong?

And if you are this sloppy in this area (forgetting to put the TRUE), what other types of bugs pop up in your code.

I don't know, maybe it was my army training, but I am a big believer in "attention to detail".  When you are thourough with all the little things, it spreads out to the big things.

William C
Tuesday, December 10, 2002

====
>>>>>>>>>>
is that in C, there is no such thing as TRUE, there is only "not FALSE"
>>>>>>>>>>

#define
===

William, so you insert
#define TRUE 1
but what if a function returns -1?

Frederik Slijkerman
Tuesday, December 10, 2002

William C.

"if true, that the commandLineInput is Valid."

is how you would read

"if (true == commandLineInput.isValid())".

Fine.  But the criticism still remains: your English translation of the code scans worse than the English translation of the alternate version:

"if the command line input is valid..."

reads better than:

"if true, that the command line input is valid..."

If you were writing documentation, or a novel, the second version is less readable, less quickly understood than the first.

somebody
Tuesday, December 10, 2002

ROFL!!

After reading the last dozen or so comments, I think it's clear why most code reviews fail.

"No! This is the best way!"
"No, you inexperienced n00b, do it this way!"
"You're both crazy, everyone *knows* this is the right way!"

Hehehe

Another programmer's opinion
Tuesday, December 10, 2002

It's not that I don't pay attention to detail, it's more that this needless focus on detail is consuming a FUCK of a lot of time.

The rest of the business is already wondering why it takes us six weeks to write a peice of code it takes VB developers only a week to write. The answer is that the VB developers are spending their valuable time asking "is this code useful to the company? How can I make it more useful? How can I be more productive?"

The C++ developers are spending ages in meetings measuring the indention of their code and checking it conforms to standards, and not noticing they're in danger of getting laid off in favour of having more VB people around...

{I'd mind less if this was producing bug free code. Unfortunately we have a level of neurosis that makes the shuttle software team look like XP heroes and at the end of the day the code quality isn't any better than any random software sweatshop you'd care to name..}


See, my solution to this is not, as you phrased it, to wilfully ignore the company protocol, but rather write myself a Perl script to fix the thing after the fact... "automate all that can be automated"


Actually, I'm going to stop frequenting this site. I think I've been insulted by random testosterone-charged wankers who think they are the world once too often now. I'll pop back once in a while to see if the clue index rises.

Katie Lucas
Tuesday, December 10, 2002

>>>>>>>>>>
After reading the last dozen or so comments, I think it's clear why most code reviews fail.

"No! This is the best way!"
"No, you inexperienced n00b, do it this way!"
"You're both crazy, everyone *knows* this is the right way!"
>>>>>>>>>>

I agree with that.  This is why I don't usually get into these debates.  All debates should end with: "what is the company standard?"

If you notice, though, I NEVER, EVER said that my way was the best way.

Katie Lucas had pointed out that she thought doing this:

if ( go_do_something() == TRUE )

had no merit.

Another poster was like, "they make you do that!  Only people that aren't as great as me do that" (my paraphrase).

I chimed in with, "well, I DO THIS.  And here's my reasons why.  I don't think they are silly".

But never once did I say: "If you don't do this you are stupid" or "Can't you see my way is the best way".

Never.

I was just presenting another side of the coin that maybe people who were ciriticizing didn't realize.

The fact that people take offense to that, baffles me (and shows me that THEY are the one's that are "religious" about "their standards").

As far as I'm concerned, if there is a company standard that says:

DO THIS:

if ( TRUE == go_do_something() )

you do it!

If it doesn't say anything regarding that subject...

do this:

if ( go_do_something())

do this:

if ( TRUE == go_do_something() )

do whatever!  As long as it works.

William C
Tuesday, December 10, 2002

for the go_do_this example()
this name is ok for an example, you'd be surprised on how many 'foo_bar', 'temp' and 'i','j','k' variables people use.

That is a reason for concern and to look at in code reviews. If it is confusing, then people will make mistakes on it.

And for me, readability may be more important than safety in general. Tools like QAC will see that a statement like if (variable=TRUE) is not a great coding and possibly a mistake (==)

Adriaan van den Brand
Tuesday, December 10, 2002

>>>>>>>>>>
It's not that I don't pay attention to detail, it's more that this needless focus on detail is consuming a FUCK of a lot of time.
>>>>>>>>>>

The fact that you are angry baffles me.  Wear a helmet.  Don't get offended so damn easily.  Just because someone challenges a position that you stated on a PUBLIC message board, it doesn't mean it was done with malice.

First, you would have to define: "consuming a **** of a lot of time."

If a comment from a co-worker, that takes 5 minutes out of your day, is "a **** of a lot of time".  I'm not sure I agree.

Are you having a code review every single day?

You have to define all the variables.

Also, you said that this was the company standard.  If it is a company standard, why ignore it?  Then it wouldn't take "a **** of a lot of time".  If it's not a standard, and jr. engineers are taking up hours of your week pointing things out that aren't a company standard ... fix that problem.

>>>>>>>>>>
The rest of the business is already wondering why it takes us six weeks to write a peice of code it takes VB developers only a week to write. The answer is that the VB developers are spending their valuable time asking "is this code useful to the company? How can I make it more useful? How can I be more productive?"
>>>>>>>>>>

VB coders don't have "religious" wars such as this?  I think there's more to it than "they don't argue about brackets".

And your C++ developers take 6x longer to do the same thing a VB developer does?

I think you are leaving out a lot of information.

At my company, I switch between VB (some win32 apps like our app that integrates with ArcMap) and C++ (some win32 apps, Win CE, palm apps) depending on the project.

(VB) DLL hell ain't fun either.

>>>>>>>>>>
The C++ developers are spending ages in meetings measuring the indention of their code and checking it conforms to standards, and not noticing they're in danger of getting laid off in favour of having more VB people around...
>>>>>>>>>>

You have this many code reviews?  That you are spending ages measuring the indentation of code?  Wouldn't the same people, if they were on the VB team, quelch VB development in the same way?  Get rid of these dummies, decline the meetings, or take the preventative measure of eliminiating the number of meetings focusing on this subject.

>>>>>>>>>>
See, my solution to this is not, as you phrased it, to wilfully ignore the company protocol, but rather write myself a Perl script to fix the thing after the fact... "automate all that can be automated"
>>>>>>>>>>>

However you like to do it, Great!  But, if people still keeping bringing it to your attention...either:
a) your perl script has bugs
b) you forget to run the perl script often enough that it is an issue to others.

Why you just can't take the time to type 1,2,3,4,5,6,7 (TRUE ==) characters is beyond me.

>>>>>>>>>>
Actually, I'm going to stop frequenting this site. I think I've been insulted by random testosterone-charged wankers who think they are the world once too often now. I'll pop back once in a while to see if the clue index rises.
>>>>>>>>>>>

Show me where I insulted you.  You are insulting with your use of vulgar language.  You are insulting with the rage you display with this post.

You know, if you can't realize that someone challenging your position isn't always with angere and vitrol, especially on a message board when tone is very hard to interpret, you should stop posting.

Gosh, people get so riled up for nothing.  Does every sentence someone types on a message board have to have a:

:) or :-) or "hehe" or "haha" or "attention: everything I am about to say is supposed to be interpreted in a gumdrop and lollipop type of way".

I was just presenting an alternative point of view based on THE FACTS YOU PRESENTED.

Tone and intent aside, name one thing that I challenged you on that was completely wrong.  Please do so.

Or is it because someone didn't agree with you and stroke your ego, you lost your temper like you were some child on a playground; "He cut in line at the swings!" waaaa! :)

William C
Tuesday, December 10, 2002

Nice demonstration that it takes real skill to moderate a formal code review among people who just happen to work with each other.

Tj
Tuesday, December 10, 2002

Something that works for our team is to use an automated unit test framework akin to the JUnit/CppUnit family to validate the build, then to invest the time spent on
code reviews in reviewing component interfaces and their unit tests.

It moves the focus away from the 'microstructure' of the code - and potential for disputes - and it recoups the effort spent on reviewing each time the reviewed unit tests are used to verify the entire build after refactoring or adding new functionality.

It also spreads knowledge of the component interfaces and their semantics around the team, which helps avoids misunderstandings and integration conflicts.

As mentioned before in this thread, it's a lot easier to do reviews if they're informal: we use a style of review similar to pair programming, ie. the reviewee describes the interfaces and tests, hands over the keyboard and lets the reviewer have a look around and ask the questions, then the changes are made either there and then by either person, or are left to the reviewee to be done later,
in which case the process repeats itself until both people are happy.

I've found that when being a reviewee, in the last 12 reviews I've had, 11 resulted in an improvement
in the interfaces and unit tests. It wasn't painful.

The rest of the team seem to be comfortable with it too.

We do have a code standards document but it's as minimal as possible - where to put the braces, how to capitalise identifiers, use of namespaces and where to put source code in the source tree.

"Use the smallest coding standards document that could possibly work."

Think of it as being "A bit exciting but not quite full-on extreme programming." :-)

Maybe this will help?

Gerard
Tuesday, December 10, 2002

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Actually, I'm going to stop frequenting this site. I think I've been insulted by random testosterone-charged wankers who think they are the world once too often now. I'll pop back once in a while to see if the clue index rises.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>


Wow...Insulted? I see how people disagreed with you, but insulted? You need some thicker skin then.

This is one of the most professional and courteous boards I visit, so if you get riled here then you best stay off discussion boards or Usenet.

Another programmer's opinion
Tuesday, December 10, 2002

>>>>>>>>>>
William, so you insert
#define TRUE 1
but what if a function returns -1?
>>>>>>>>>>

Well, a function that returns -1, I would expect a return type of int or long or something of that nature.

I'm using:

if ( TRUE == go_do_something() )

when the return type is BOOL (whereas, you can expect 0 or 1, because all your assignments in that function should be ret = TRUE or ret = FALSE).

===

Although, I'm open to anything.

I might just start using this more:

if ( CommandLine.IsValid() )

that is, if the Function is set up so that it is self-documenting.

The difference between me and the religious zealots is that if someone makes a point I agree with, I'm happy to adopt it.

And if it'a a function that could return -1 (as true), and I'm assuming 0 = false and everything else true,

I might just use:

if ( go_do_something() ) when testing for true.

Or I might do:

if ( 0 != go_do_something() ) when testing for true.

although, I think right now I perfer the !=

William C
Tuesday, December 10, 2002

I used to work for a company that was SEI-CMM Level 3 certified, and part of being at that level was mandatory code reviews for all code that gets released to production.  Out of the mountain of paperwork and processes that SEI required, this is one of the few aspects that I can undoubtedly say was of significant benefit.

Code reviews do take time, but if done right there will be a net savings of time and money.

In my experience with the code reviews at my previous company and the process of establishing them at my current company, these are the main benefits, do's and don'ts that I try to work with.

Benefits:

1. Shortened testing cycle.  More programs will run right the first time, reducing the number of round trips a program takes between the testing team and development team.

2.  Verification of obscure conditions.  There are some conditions that are highly unlikely to occur in testing or may be difficult to reproduce by the testers. Having another person analyze the code to check how the conditions are handled is sometimes the only effective way to verify that the program will work properly for all valid inputs.

3.  Increased familiarity with other people's code.  If you are reviewing other people's code, with the opportunity to ask the programmer questions about dubious segments of the code, you will be in a better position to take over that person's code if they leave the company or move on to other tasks.

4.  Increased quality engendered into developers.  If you know your code is going to be reviewed, you'll take better care to make sure it is clean and correct.  By getting feedback, programmers also learn how to write good code.  And with better developers writing better code, maintenance and enhancements become cheaper and faster.


Do's:

1.  Code reviews must be supported and mandated by all levels of management. This includes having a QA team or some similar body that can veto any unit of code from going to production if it hasn't been reviewed.  Otherwise reviews won't happen. 

2.  Code reviews must be planned and estimated.  They have to be seen as a real unit of work, just like all other aspects of the development process.  Otherwise they'll be jammed in only when people can squeeze in the time.  They do take time, but that time is regained (plus more) with shorter testing cycles and easier maintenance.

3.  Code reviews must be documented.  This is so that it can be verified that all feedback has been followed up and the relevant fixes (if any) have been made.  The documentation also serves as information a future maintenance programmer can look back on to find out why certain decisions were made in the code.

4.  Separate the checking of style and conventions from code reviews.  Use a tool for indentation, and have a separate person look at it for naming conventions and the like.  Let the code reviewers focus on correctness and robustness.  Either that, or find a way to partition the code review process so that style and correctness are considered independently of each other.  Otherwise it degenerates into stylistic arguments like the one on this board, and the correctness of the code isn't given proper attention.

5.  Review the code *before* a review meeting takes place.  If necessary, a 'preview' meeting can be held in which the programmer presents what the code is doing so the reviewer(s) can better understand what they are about to look at.


Don'ts:

1.  Don't have a review meeting for everything.  If code is submitted for review, and the programmer agrees with all the feedback (or the feedback says nothing is wrong), the programmer should just go ahead and fix it without a meeting. If there is only minor disagreement the programmer should explain his position with a sentence or two in writing. The review meeting, if any, should focus on feedback that the programmer disagrees with, or questions from the reviewers for clarifying any dubious (but not necessarily erroneous) sections of code.

2.  Don't create an adversarial situation that makes programmers defensive of their code.  Management and senior developers must create an atmosphere in which the philosophy is that the reviewers are working *with* the programmers to make the code better, not to criticize and finger-point.

T. Norman
Tuesday, December 10, 2002

Why do I get the impression code reviews are popular with the less capable programmers?


Wednesday, December 11, 2002

Blank-guy said:
Why do I get the impression code reviews are popular with the less capable programmers?

What makes me think that you are one of the "less capabable" ones?


Wednesday, December 11, 2002

Somebody wrote...

"Why do I get the impression code reviews are popular with the less capable programmers?"

Your question contains its own answer - because I'm less capable, so I need help with my work.

I'd recommend "The Psychology of Computer Programming"
(G.Weinberg) for an amusing but informative account of this sort of thing.

Now, back to those unit tests...

Gerard
Wednesday, December 11, 2002

" in C, there is no such thing as TRUE, there is only "not FALSE". For all you know, go_do_something() might return -1 or 193 instead of the actual value TRUE. Most Windows functions are defined that way (success is non-zero).

Frederik Slijkerman"

MS seem to disagree with you:
#undef FALSE
#undef TRUE
#undef NULL

#define FALSE  0
#define TRUE    1
(from AFX.H shipped with VC++ 6.0) which implies to me that they are asserting that _all_ of their interfaces return 1 for TRUE and 0 for FALSE.


Wednesday, December 11, 2002

Noname,

>which implies to me that they are asserting that _all_ of >their interfaces return 1 for TRUE and 0 for FALSE.

Correct me if im wrong, but the two functions below, according to the Windows help returns either 0 or NULL on failure. This is inconsistent at best. This leads me to believe that you can not do a if (LoadLibrary...) = FALSE construction.

SetForegroundWindow()
--------------------------------
If the function succeeds, the return value is nonzero.
If the function fails, the return value is zero.

LoadLibrary()
-----------------
If the function succeeds, the return value is a handle
to the module. If the function fails, the return value is NULL.

Then again I am not proficient in C/C++

Patrik
Wednesday, December 11, 2002

>> but what if a function returns -1?

Imo, that is why you should always use ==true or !=0 etc. If you aren't certain what the function is returning when you write the if statement, then that is your bigger problem.

In C# you have to say these things explicitly if(2){} won't compile, which is a good thing to me anyway.

Robin Debreuil
Thursday, December 12, 2002

"SetForegroundWindow()
--------------------------------
If the function succeeds, the return value is nonzero."

The return type is BOOL, so you _should_ be able to test == TRUE. Note though that the above documentation conflicts with Microsoft's definition of TRUE. One wonders which is really true (pun not intended), and that seems like a good enough reason not to check == TRUE with these functions, to me.

"LoadLibrary()
-----------------
If the function succeeds, the return value is a handle
to the module. If the function fails, the return value is NULL"

I really don't think you should test == TRUE / != FALSE here; you have to test != NULL, because after all LoadLibrary isn't returning a BOOL. You should be pretty safe dropping the == TRUE / != FALSE [just writing: "if (LoadLibrary(...))" ], which assumes that NULL is always going to be equivalent to 0. According to comp.lang.c this is true (not that I'm about to start doing it, but they do say that it is so).


Thursday, December 12, 2002

I didn't want to get into this whole true.false thing, but....

My coding standards (and those at the last place I worked) is, if the method returns a bool, no check is needed in the if statement.  If the method returns other than a bool, then the code should be made easier to read by pulling the method out of the if, and checking the return value separately.

retval = method()
if(retval != NO_ERROR)
{
    code
}

Most compilers will optimize the extra 'retval' away, and it makes it easir to read.

Gerald Brandt
Thursday, December 12, 2002

I'd love to have code reviews. I've made my living programming for about two decades, and have never worked anywhere that had them. Where I am now, code reviews consist of me going back into code I wrote earlier whenever the customer pays for another round of changes.

Yes, I am the entire software department
Wednesday, December 18, 2002

I believe code reviews are useful in providing managers with insight into the complexities of the project.  It's a perfect time to wein your juniors from the hideous structural mistakes they introduce like:  global variables, loosely coupled interfaces, a complete lack of commenting, and complex, unstructured, run-on logic.

Managers need to realize that code reviews are a process of QA.  This is the time to hold the programmer accountable for the work they're producing (not after the customer complains about the first aweful bug).

Would you wire a house or building without and not have it inspected?  If you answer "Yes", then you had better make sure the fire department is close!

Thanks

AC
Tuesday, July 20, 2004

*  Recent Topics

*  Fog Creek Home