Code reviews, what do you think?
-
I'm at a small company that has a very small development department (5 programmers). Quality has recently become an issue. I've been at companies that do code reviews. My personal opinion is that formal code reviews are not worth their time. Informal code reviews on the other hand can be useful. If it's determined that the code can impact a lot of different areas and/or the code is deemed "complicated", the developer would need to call on a supervisor or peer to go over the algorithm and identify potential impacts. What is done at your company? What has worked? What hasn't? Give me one more medicated peaceful moment
We focus more on design and arch reviews but also do code reviews. Code reviews are more of an activity that is ongoing, i.e., while the code is being written. I expect team leads to be aware of the quality of code being written and coding standards being used. Rather than one day three weeks after the code being written to review it and ask developers to change stuff. That is just a waste of time. Also, I have found that code reviews if not handled well become a way for people to make other peoples work look bad. And developers can be big time arrogant SOBs. And when faced with deadlines, I hate pissing matches between developers. :-) Which is why I expect team leads to be doing the code review on an ongoing basis. This way they also have a better idea of what the hell is being built. :-)
-
We focus more on design and arch reviews but also do code reviews. Code reviews are more of an activity that is ongoing, i.e., while the code is being written. I expect team leads to be aware of the quality of code being written and coding standards being used. Rather than one day three weeks after the code being written to review it and ask developers to change stuff. That is just a waste of time. Also, I have found that code reviews if not handled well become a way for people to make other peoples work look bad. And developers can be big time arrogant SOBs. And when faced with deadlines, I hate pissing matches between developers. :-) Which is why I expect team leads to be doing the code review on an ongoing basis. This way they also have a better idea of what the hell is being built. :-)
Ranjan Banerji wrote: nd developers can be big time arrogant SOBs. And when faced with deadlines, I hate pissing matches between developers. Sooooooooo True! I rember being the "third guy" listening to two older developers saying something like "Why are you using that terse "ternarry" operator, use an if/else." and "What, have you never coded before! If/else is so pedistrian." Man I soo wanted to tell them both to just shut the hell up and stop acting like children. The word abbreviation is awfully long for what it means.
-
Imho, code reviews work well within small teams. In a previous life, I worked in a group of 4 developers, one of whom was the team lead. We were all pretty experienced and fairly solid developers. Code reviews involved the team lead (whose knowledge of the overall product was much greater than mine), the developer whose code was being reviewed, and sometimes one more developer. The "third guy" was someone who could benefit from the review by becoming aware of better ways of implementing something. The code review lasted no more than an hour and was done in an informal and friendly manner. The intent of the review was to help, not censure. The purpose of the review was to:
- catch bugs that may have been overlooked by the developer (the "2nd pair of eyes" syndrome)
- catch errors in comments
- see if a better (eg: simpler, faster) way could be found to implement something
- guard against future incompatibilities
- ensure that the code complied with the group's coding guidelines, making it easier for others to understand the code
Any required changes to the code were made by the developer after the review. There was almost never a need to have a follow-up code review. It's important to note that a code review is intended to review implementation, not design. At our company, design reviews were iterative and detailed. They were more time consuming because the implications of failure at design time are higher than that at implementation time. /ravi Let's put "civil" back in "civilization" http://www.ravib.com ravib@ravib.com
I agree with the method of code reviews that you have described. The one of the biggest problems that has occured in teams that I have worked on, it the developer may not completely understand how their changes will affect the rest of the system. They may have a solid design, but the implementation could cause problems. Another good point that you made was for an unfamiliar developer to be involved in order to learn more about the system. A very good point.
Build a man a fire, and he will be warm for a day
Light a man on fire, and he will be warm for the rest of his life! -
I'm at a small company that has a very small development department (5 programmers). Quality has recently become an issue. I've been at companies that do code reviews. My personal opinion is that formal code reviews are not worth their time. Informal code reviews on the other hand can be useful. If it's determined that the code can impact a lot of different areas and/or the code is deemed "complicated", the developer would need to call on a supervisor or peer to go over the algorithm and identify potential impacts. What is done at your company? What has worked? What hasn't? Give me one more medicated peaceful moment
Code reviews are an excellent resource that should not be overlooked. The thing is, code reviews are a management resource, so you need to have a manager that keeps control of the reviews so that they are short yet still accomplish their goal. I think the one of the most important things a code review can accomplish is prevent incompatibilities that may arise because of changes made by the reviewed code. The design may be sound, but implementations can still cause problems. Even if they are innocent changes all by themselves, they may not be well suited once they are integrated with the system. Reviews will insure that developers are using the proper methods of implementation set by the guidelines. Such as using the STL vs MFC containers or whatever other guidelines are set forth. Just these inconsistencies alone could make the code harder to read or expand in the future. Another one that I like, is for other developers to be familiar with each others work. That way people are more aware of what the team is working on and will be more aware of who to talk to if there is a problem in certain systems. I agree that they can get out of hand, but the point is to limit them to small teams, and use your time judicuously while reviewing the code.
Build a man a fire, and he will be warm for a day
Light a man on fire, and he will be warm for the rest of his life! -
I'm at a small company that has a very small development department (5 programmers). Quality has recently become an issue. I've been at companies that do code reviews. My personal opinion is that formal code reviews are not worth their time. Informal code reviews on the other hand can be useful. If it's determined that the code can impact a lot of different areas and/or the code is deemed "complicated", the developer would need to call on a supervisor or peer to go over the algorithm and identify potential impacts. What is done at your company? What has worked? What hasn't? Give me one more medicated peaceful moment
The only thing that I've ever found to work is to have the project lead review all the code him/herself. Hopefully they are project lead because they know how to run a project, are doing most of the upfront design work, and need to "merely" ensure a level of quality. Changes should be noted and made by the author of the code so he/she learns something. Group code reviews end up squabbling over the color of the moth eating the leaf on the tree in the forest. Marc Help! I'm an AI running around in someone's f*cked up universe simulator.
Sensitivity and ethnic diversity means celebrating difference, not hiding from it. - Christian Graus
Every line of code is a liability - Taka Muraoka
Microsoft deliberately adds arbitrary layers of complexity to make it difficult to deliver Windows features on non-Windows platforms--Microsoft's "Halloween files" -
I'm at a small company that has a very small development department (5 programmers). Quality has recently become an issue. I've been at companies that do code reviews. My personal opinion is that formal code reviews are not worth their time. Informal code reviews on the other hand can be useful. If it's determined that the code can impact a lot of different areas and/or the code is deemed "complicated", the developer would need to call on a supervisor or peer to go over the algorithm and identify potential impacts. What is done at your company? What has worked? What hasn't? Give me one more medicated peaceful moment
I wish our department had more than 1 programmer, so we could do code reviews. More eyes ears and knowlege will, almost, always improve the quality of a product. Trust in the code Luke. Yea right!
-
I'm at a small company that has a very small development department (5 programmers). Quality has recently become an issue. I've been at companies that do code reviews. My personal opinion is that formal code reviews are not worth their time. Informal code reviews on the other hand can be useful. If it's determined that the code can impact a lot of different areas and/or the code is deemed "complicated", the developer would need to call on a supervisor or peer to go over the algorithm and identify potential impacts. What is done at your company? What has worked? What hasn't? Give me one more medicated peaceful moment
i think a lot of the time its a good excuse for programmers to be wankers to each other AND if there is a particularly strong willed/more expericed/senior programmer then they just tend to enforce their will. I also once worked with a total bitch (lead programmer) who after i offered to write the coding style guide for the company agreed, said it was a good idea and then later said "you can write the coding style guide but i'm just going to do it my way and i'm not changing" Bryce
-
The only thing that I've ever found to work is to have the project lead review all the code him/herself. Hopefully they are project lead because they know how to run a project, are doing most of the upfront design work, and need to "merely" ensure a level of quality. Changes should be noted and made by the author of the code so he/she learns something. Group code reviews end up squabbling over the color of the moth eating the leaf on the tree in the forest. Marc Help! I'm an AI running around in someone's f*cked up universe simulator.
Sensitivity and ethnic diversity means celebrating difference, not hiding from it. - Christian Graus
Every line of code is a liability - Taka Muraoka
Microsoft deliberately adds arbitrary layers of complexity to make it difficult to deliver Windows features on non-Windows platforms--Microsoft's "Halloween files"Marc Clifton wrote: Group code reviews end up squabbling over the color of the moth eating the leaf on the tree in the forest. It was light grey anyway. You can't squabble over a fact. Seriously - I agree with you totally. I've seen so many "formal" reviews degenerate into a waste of time because individuals got tied up in naming conventions, or comment conventions. I think code reviews should start VERY early in the development process, and be used to weed out things like cut and paste code - which I see a LOT of, and functions that do more than one thing... You know - the 1700 line function that gets the data, manipulates the data, formats the data, sorts the data then exports the data to an xml format and then finally actually sends the data back to a web server. The key though is that the commitment has to be there to actually change the code. Often, the code review ends up not achieving anything because of "lack of time". Once it's in code, it's too late! And they still ran faster and faster and faster, till they all just melted away, and there was nothing left but a great big pool of melted butter "I ask candidates to create an object model of a chicken." -Bruce Eckel
-
Marc Clifton wrote: Group code reviews end up squabbling over the color of the moth eating the leaf on the tree in the forest. It was light grey anyway. You can't squabble over a fact. Seriously - I agree with you totally. I've seen so many "formal" reviews degenerate into a waste of time because individuals got tied up in naming conventions, or comment conventions. I think code reviews should start VERY early in the development process, and be used to weed out things like cut and paste code - which I see a LOT of, and functions that do more than one thing... You know - the 1700 line function that gets the data, manipulates the data, formats the data, sorts the data then exports the data to an xml format and then finally actually sends the data back to a web server. The key though is that the commitment has to be there to actually change the code. Often, the code review ends up not achieving anything because of "lack of time". Once it's in code, it's too late! And they still ran faster and faster and faster, till they all just melted away, and there was nothing left but a great big pool of melted butter "I ask candidates to create an object model of a chicken." -Bruce Eckel
I think code reviews should start VERY early in the development process, Isn't that called "design"??? ha ha ha ha Aye, there's the rub. As project lead, even given a "senior" programmer, I've had to design to such detail that I was basically psuedo-coding just to get the right level of abstraction, re-use, maintainability, instrumentation, and modularity that I demanded on the project. What was the point? I spent more time doing the design than I would have writing and testing the code myself. Well, I guess the point is that then these guys would "improve". Ha. Management never understands that in-house education is a major hidden cost. Project development is not a democratic process. It is a dictatorial process. Anyone who tries to convince you otherwise is a new-age pansy. Marc Help! I'm an AI running around in someone's f*cked up universe simulator.
Sensitivity and ethnic diversity means celebrating difference, not hiding from it. - Christian Graus
Every line of code is a liability - Taka Muraoka
Microsoft deliberately adds arbitrary layers of complexity to make it difficult to deliver Windows features on non-Windows platforms--Microsoft's "Halloween files" -
I think code reviews should start VERY early in the development process, Isn't that called "design"??? ha ha ha ha Aye, there's the rub. As project lead, even given a "senior" programmer, I've had to design to such detail that I was basically psuedo-coding just to get the right level of abstraction, re-use, maintainability, instrumentation, and modularity that I demanded on the project. What was the point? I spent more time doing the design than I would have writing and testing the code myself. Well, I guess the point is that then these guys would "improve". Ha. Management never understands that in-house education is a major hidden cost. Project development is not a democratic process. It is a dictatorial process. Anyone who tries to convince you otherwise is a new-age pansy. Marc Help! I'm an AI running around in someone's f*cked up universe simulator.
Sensitivity and ethnic diversity means celebrating difference, not hiding from it. - Christian Graus
Every line of code is a liability - Taka Muraoka
Microsoft deliberately adds arbitrary layers of complexity to make it difficult to deliver Windows features on non-Windows platforms--Microsoft's "Halloween files"Marc Clifton wrote: Project development is not a democratic process. It is a dictatorial process. Anyone who tries to convince you otherwise is a new-age pansy. Well said. And they still ran faster and faster and faster, till they all just melted away, and there was nothing left but a great big pool of melted butter "I ask candidates to create an object model of a chicken." -Bruce Eckel
