[KenJackson]

If your organization does formal code reviews, what is the purpose of the review?

I expect it will be both of these major goals:

• Correct errors in execution, including race conditions and bugs, and
  
• Make the code syntax conform to the organization's standard

But which one is actually predominant?

That is, in your code reviews, does the discussion revolve around how the code looks? Or how it works?

[Curtis Rutland]

The first, by a huge margin. If your code doesn't work, it doesn't matter if it looks pretty or conforms to a standard. First and foremost, make sure your code functions correctly. If you can do this and match the style guide at the same time, then great. If not, then make it work then make it match the style. But broken code is broken code, regardless of it's conformation to standards.

[KenJackson]

I like the way you think, Curtis.

But I notice this thread was viewed something like 160 times before I got the first response, so I wonder if everyone thinks that way. And even more than thinking one way or the other, I wonder how it works out in practice.

[Curtis Rutland]

It was viewed that many times because it's featured on the front page. Lots of people who aren't even members can see it easily. Most won't respond because they're just "driving by."

Anyway, I won't downplay the importance of conforming to style guidelines. It's very important, and if your code is functional, then by all means, review the style. But no point in reviewing broken code.

[Programmist]

Definitely the first is most important of those two, but code review is also a great opportunity to educate and mentor.

[sbromley]

Where I work we use a tool called crucible, it lets us review code online, so we don't have to meet every time a code review is needed. Most of the time when you upload code and create a review there should not be bugs in it. All of our test code gets uploaded with it, and people will check to make sure that you've hit all possible test cases. Then most developers and at least one of the architects will run through the flow and point out anything that might look bad or needs adjusting.

All code is reviewed and no one has time to comb through everything finding all your bugs. This is why you can't blame a code review if a bug shows up in a production environment, and your code is producing it.

At the same time if you do submit poor code, that is full obvious bugs, you'll probably just get told to rewrite it.

[cannymonkey]

Code reviews where I work are much more to do with programming style and aesthetics, and looking for cleanly-written code with good encapsulation, loose-coupling, etc. Although we don't have any discernible "house style" to adhere to, we are encouraged to take on and use the ideas laid out in Bob Martin's book, Clean Code: A handbook of Agile software craftsmanship.

By the time our code is reviewed it has usually already been through the hundreds/thousands of little ad hoc tests that we have done while developing the application, and also through the rigorous testing of our App. Support team....i.e. functionally, all bugs and erroneous logic issues have been ironed out before it is reviewed.

This works well for us as we are relatively small development team (15-20 developers) and there is quite a bit of back-and-forth during development anyway, so a more experienced developer can point out issues and alternative ideas on the fly, not just in one massive critique at the end.

[Curtis Rutland]

It's difficult to take anything written in Comic Sans seriously.

[cannymonkey]

Curtis Rutland, on 10 September 2011 - 07:59 AM, said:

It's difficult to take anything written in Comic Sans seriously.

Comic Sans rocks dude...although perhaps that's why i'm a web developer rather than a web designer. It's the substance that counts in my book, not how pretty the UI is.

This post has been edited by cannymonkey: 10 September 2011 - 09:47 AM

[KenJackson]

cannymonkey, on 10 September 2011 - 08:41 AM, said:

Code reviews where I work are much more to do with programming style and aesthetics, ...

By the time our code is reviewed it has usually already been through ..., all bugs and erroneous logic issues have been ironed out before it is reviewed.

Well that's kind of a twist that I didn't expect. Though I guess it makes sense. Rely on testing to find bugs and have code reviews to placate the style gestapos.

But neither style reviews nor testing will find the bad coding decisions that pass all tests, yet crash and burn badly after somebody implements a fix for something unrelated. Often a later change alters assumptions made at the start. That's the kind of thing I was hoping someone would say they look for.

Interesting though.

[BobRodes]

Curtis Rutland, on 30 August 2011 - 12:06 AM, said:

The first, by a huge margin. If your code doesn't work, it doesn't matter if it looks pretty or conforms to a standard. First and foremost, make sure your code functions correctly. If you can do this and match the style guide at the same time, then great. If not, then make it work then make it match the style. But broken code is broken code, regardless of it's conformation to standards.

The less people in the organization, the huger the margin of the first IMO. As you begin to add resources, maintenance overhead becomes more and more of an issue and therefore more of a subject in code reviews. The quicker that another person can get up to speed on whatever you did, the less money it costs. Conformance to corporate standards lowers that time.

[BobRodes]

[

Quote

But neither style reviews nor testing will find the bad coding decisions that pass all tests, yet crash and burn badly after somebody implements a fix for something unrelated. Often a later change alters assumptions made at the start. That's the kind of thing I was hoping someone would say they look for.

Perhaps those conducting regression testing would beg to differ with your position that testing will not find these bad coding decisions, for finding them is regression testing's precise goal. For more, see here: http://en.wikipedia....ression_testing .

This isn't really the place of code or peer review, which is more about finding known weaknesses in coding style and coding errors. For more, see here: http://en.wikipedia....iki/Code_review .