Code reviews
-
Code reviews aren't a magic bullet for solving issues of code quality. If your software has a high defect rate there are deeper issues you need to address. You need to take a look at the root causes of your defects, and the most likely cause is a lack of rigorous testing. A good start would be to introduce automated unit testing into your regime, and possibly look at things like Test Driven Development. Unit Testing is a discipline that needs to be worked at but, done properly, it is worth doing and does help.
Deja View - the feeling that you've seen this post before.
Pete O'Hanlon wrote:
Code reviews aren't a magic bullet for solving issues of code quality. If your software has a high defect rate there are deeper issues you need to address.
According to Steve McConnell,author of Code Complete, code reviews are far more effective than testing at eliminating software defects (I can't check this personally as I've never been in an environment that does them.) However, this doesn't mean we shouldn't test. We should do both. Nothing on its own is a magic bullet.
Kevin
-
Several places I've worked have tried formalized code reviews. It's never worked. Informal code reviews have worked well, but entirely depend on quality engineers.
Anyone who thinks he has a better idea of what's good for people than people do is a swine. - P.J. O'Rourke
Joe Woodbury wrote:
Informal code reviews have worked well
What does an informal code review look like? I'm curious.
Kevin
-
Pete O'Hanlon wrote:
Code reviews aren't a magic bullet for solving issues of code quality. If your software has a high defect rate there are deeper issues you need to address.
According to Steve McConnell,author of Code Complete, code reviews are far more effective than testing at eliminating software defects (I can't check this personally as I've never been in an environment that does them.) However, this doesn't mean we shouldn't test. We should do both. Nothing on its own is a magic bullet.
Kevin
Indeed - and I'm a big fan of his work, but code reviews are rarely done by people who can put their egos to one side. And this is a huge problem with code reviews - everybody has to sign up to it, and they have to embrace it. Code reviews have to be equal - everybody has to be treated equally. This is all external process. So, what can you do to improve your code - well, you can actively look to test your code. Exercise it and try to break it. With the advent of tools like nunit, there's no reason not to test. Plus, Code Complete was originally written before automated testing tools such as NUnit were widely available and used. I rather suspect that Steve McConnell would embrace them as well.
Deja View - the feeling that you've seen this post before.
-
I work as a developer in a small software company. The software we produce has a high deffect rate. I want to talk my boss into doing code reviews but the problems that I´m facing are that I don´t find my boss and the other senior developers mature enough to do a real code review instead of a "mine is biggers than yours" code review (they use to talk thrash about other programmer´s code, just because it´s not programmed their way) and that the company promotes individualism insted of team playing (and a code review could be used not to find bugs but to diminish someone else code so they can stand out) So, I do want to get a better software but I´m afraid I could start a monster... Any idea?
Gabriel.P.G wrote:
I´m facing are that I don´t find my boss and the other senior developers mature enough to do a real code review instead of a "mine is biggers than yours" code review (they use to talk thrash about other programmer´s code, just because it´s not programmed their way) and that the company promotes individualism insted of team playing
That sounds like the real problem, and no code review can cure it. Look for another job.
-
Joe Woodbury wrote:
Informal code reviews have worked well
What does an informal code review look like? I'm curious.
Kevin
Just asking a trusted mate to look over your code or bouncing an algorithm off of them. Sometimes it's just scribbling on a white board as you work something out verbally.
Anyone who thinks he has a better idea of what's good for people than people do is a swine. - P.J. O'Rourke
-
Gabriel.P.G wrote:
I´m facing are that I don´t find my boss and the other senior developers mature enough to do a real code review instead of a "mine is biggers than yours" code review (they use to talk thrash about other programmer´s code, just because it´s not programmed their way) and that the company promotes individualism insted of team playing
That sounds like the real problem, and no code review can cure it. Look for another job.
I'll second that opinion. It was getting darned close to that decision for me too where I currently work, all because of a software manager that was the root cause for all of the strife and tension within the group. The day was saved when upper management recognized the situation and in a politically sensitive move, shifted the s/w manager to a project manager position and elevated a project lead with good people skills into the s/w manager's role. It's been great ever since...
-
I work as a developer in a small software company. The software we produce has a high deffect rate. I want to talk my boss into doing code reviews but the problems that I´m facing are that I don´t find my boss and the other senior developers mature enough to do a real code review instead of a "mine is biggers than yours" code review (they use to talk thrash about other programmer´s code, just because it´s not programmed their way) and that the company promotes individualism insted of team playing (and a code review could be used not to find bugs but to diminish someone else code so they can stand out) So, I do want to get a better software but I´m afraid I could start a monster... Any idea?
-
I'll second that opinion. It was getting darned close to that decision for me too where I currently work, all because of a software manager that was the root cause for all of the strife and tension within the group. The day was saved when upper management recognized the situation and in a politically sensitive move, shifted the s/w manager to a project manager position and elevated a project lead with good people skills into the s/w manager's role. It's been great ever since...
geoffs wrote:
The day was saved when upper management recognized the situation and in a politically sensitive move, shifted the s/w manager to a project manager position
Is this a promotion..?
-
geoffs wrote:
The day was saved when upper management recognized the situation and in a politically sensitive move, shifted the s/w manager to a project manager position
Is this a promotion..?
That's a good question! :~ I guess that in the company I am at that the move from s/w mgr to project mgr is considered more of a horizontal move. The important point is that it removed a person without the people skills from a position requiring lots of that to a position requiring much less of that.
-
Hi, I would suggest using a static analyzer tool in case code reviews is not acceptable. Regards, Kiran
There is no better solution to this everlasting problem other than automating the process.
-
I work as a developer in a small software company. The software we produce has a high deffect rate. I want to talk my boss into doing code reviews but the problems that I´m facing are that I don´t find my boss and the other senior developers mature enough to do a real code review instead of a "mine is biggers than yours" code review (they use to talk thrash about other programmer´s code, just because it´s not programmed their way) and that the company promotes individualism insted of team playing (and a code review could be used not to find bugs but to diminish someone else code so they can stand out) So, I do want to get a better software but I´m afraid I could start a monster... Any idea?
I had the same issue with a company where I worked. There were 2 major ego people (the 2 leads on the project). They tried so hard to be the "best". Their egos couldn't fit into the building. They always had issues with people's coding. On EACH project they went, the complained about people (developers). Now my questions is how do you have these 2 people review anything?
-
I work as a developer in a small software company. The software we produce has a high deffect rate. I want to talk my boss into doing code reviews but the problems that I´m facing are that I don´t find my boss and the other senior developers mature enough to do a real code review instead of a "mine is biggers than yours" code review (they use to talk thrash about other programmer´s code, just because it´s not programmed their way) and that the company promotes individualism insted of team playing (and a code review could be used not to find bugs but to diminish someone else code so they can stand out) So, I do want to get a better software but I´m afraid I could start a monster... Any idea?
#1: Don't try to run code reviews in that atmosphere. #2: start looking for a new job. Even if the rest is OK for now, this is no a good place to settle down. #3: Figure out why there are so many bugs. #4: Try unit tests for yourself, see if it works for you, and if it imresses others.
-
Indeed - and I'm a big fan of his work, but code reviews are rarely done by people who can put their egos to one side. And this is a huge problem with code reviews - everybody has to sign up to it, and they have to embrace it. Code reviews have to be equal - everybody has to be treated equally. This is all external process. So, what can you do to improve your code - well, you can actively look to test your code. Exercise it and try to break it. With the advent of tools like nunit, there's no reason not to test. Plus, Code Complete was originally written before automated testing tools such as NUnit were widely available and used. I rather suspect that Steve McConnell would embrace them as well.
Deja View - the feeling that you've seen this post before.
Pete O'Hanlon wrote:
but ...code reviews are rarely done by people who can put their egos to one side.
How true! :( To original poster: Finding a new job would be much better. I've worked in a surrounding where those who were low on the food scale were always blamed for every fault on project.
-
I work as a developer in a small software company. The software we produce has a high deffect rate. I want to talk my boss into doing code reviews but the problems that I´m facing are that I don´t find my boss and the other senior developers mature enough to do a real code review instead of a "mine is biggers than yours" code review (they use to talk thrash about other programmer´s code, just because it´s not programmed their way) and that the company promotes individualism insted of team playing (and a code review could be used not to find bugs but to diminish someone else code so they can stand out) So, I do want to get a better software but I´m afraid I could start a monster... Any idea?
Good question! I would agree with most folks above. When I started in my present position code-reviews were never heard of. But the atmosphere was very friendly with nothing of that ego-stuff. Our coding was happy-go-lucky, with very inconsistent quality. A new guy introduced the idea of reviews, it was not met without resistance, but he didn't give in, and gradually his ideas were embraced. He is one of best programmers I have met, and I think that if he hadn't been respected, his ideas might have fallen to the ground. I would like to ephasize/add/clarify that for code reviews to be helpful everybody has to embrace the idea. Also you will probably need some document with coding guidlines. Those guidelines can include simple stuff such as naming, formatting and commenting conventions. But also they should contain structural guidelines: use of frameworks (STL, Boost etc), avoidance of code duplication. Also organization of common classes etc. (TDD etc is great but it seems very far off in your organization.) All this should be in a document in order to aviod silly discussions like: - I don't think it is necessary to have all constants in upper-case - Well, I disagree So, maybe, your first fight should be for such a document, not the reviews directly. And, find an ally before dropping the bomb! Good Luck!
_____________________________________ Action without thought is not action Action without emotion is not life
-
Just asking a trusted mate to look over your code or bouncing an algorithm off of them. Sometimes it's just scribbling on a white board as you work something out verbally.
Anyone who thinks he has a better idea of what's good for people than people do is a swine. - P.J. O'Rourke
To the extreme end of this, you might even look at pair programming, even if it is only one day a week. If you don't know, pair programming is a type of informal code review where two developers are sat in front of the same computer, one "drives" while the other simply looks over their shoulder and watches for other ways of doing things (not necessarily better, just other ways). The two can switch places whenever they feel it necessary. Be warned that many people are opposed to pair programming, most will say they don't like someone looking over their shoulder or they don't want to lose personal space. There also is a very common misconception that the pair will only get 1/2 the amount of work done. This is not true for experienced teams, as the chance of unknown areas declines with having two programmers, and the code should be more solid, which saves maintenance time. One other terrific way of improving development processes is to set up a continuous build environment such as CruiseControl.NET. Simply set up the server (a 4-6 hour task for someone new to CCNET) and then set up a notification service, be it email, a notification icon (CCTray is the one for CruiseControl.NET), or a pair of lava lamps (I have seen this, it's very effective). The hardest part will then be getting the developers to do constant checkins, as the build is triggered by a checkin. Then, whenever new code is added, a build is automatically started and any unit tests you have setup will be run. If the build or the tests fail, notifications will be sent out, the notification icon will turn red and popup a message, and the team is now responsible for fixing the build. CCNET will list what build failed and why, and what files were changed and by who. Since you know what files to look at, and you know that fairly few lines of code could have changed (b/c of the frequent checkins), the problem should be able to be fixed quickly. Anyways, I hope the above gives you some more ideas on how to proceed. The true test of whether or not you should stay or leave your current job is in management reaction. If management is not willing to invest their time into fixing their procedures, then I would start to wonder if they are a company that's good for you to work for. Thanks, Seth Rowe [MVP] http://sethrowe.blogspot.com/
-
To the extreme end of this, you might even look at pair programming, even if it is only one day a week. If you don't know, pair programming is a type of informal code review where two developers are sat in front of the same computer, one "drives" while the other simply looks over their shoulder and watches for other ways of doing things (not necessarily better, just other ways). The two can switch places whenever they feel it necessary. Be warned that many people are opposed to pair programming, most will say they don't like someone looking over their shoulder or they don't want to lose personal space. There also is a very common misconception that the pair will only get 1/2 the amount of work done. This is not true for experienced teams, as the chance of unknown areas declines with having two programmers, and the code should be more solid, which saves maintenance time. One other terrific way of improving development processes is to set up a continuous build environment such as CruiseControl.NET. Simply set up the server (a 4-6 hour task for someone new to CCNET) and then set up a notification service, be it email, a notification icon (CCTray is the one for CruiseControl.NET), or a pair of lava lamps (I have seen this, it's very effective). The hardest part will then be getting the developers to do constant checkins, as the build is triggered by a checkin. Then, whenever new code is added, a build is automatically started and any unit tests you have setup will be run. If the build or the tests fail, notifications will be sent out, the notification icon will turn red and popup a message, and the team is now responsible for fixing the build. CCNET will list what build failed and why, and what files were changed and by who. Since you know what files to look at, and you know that fairly few lines of code could have changed (b/c of the frequent checkins), the problem should be able to be fixed quickly. Anyways, I hope the above gives you some more ideas on how to proceed. The true test of whether or not you should stay or leave your current job is in management reaction. If management is not willing to invest their time into fixing their procedures, then I would start to wonder if they are a company that's good for you to work for. Thanks, Seth Rowe [MVP] http://sethrowe.blogspot.com/
I totally agree with Seth Rowe. I have set up CruiseControl .net. You do not have to broach the subject of code review initially. Let Cruisecontrol compile the code every time and if possible start putting in the NUnit Tests as much as possible. The quality will start improving immediately.. and those that see their code fail multiple times will have to start thinking of better ways.. code review may eventually follow and it may come from higher up without you really saying anything... CCNET + NUnit really helps ! -Prashant
-Prashant
-
To the extreme end of this, you might even look at pair programming, even if it is only one day a week. If you don't know, pair programming is a type of informal code review where two developers are sat in front of the same computer, one "drives" while the other simply looks over their shoulder and watches for other ways of doing things (not necessarily better, just other ways). The two can switch places whenever they feel it necessary. Be warned that many people are opposed to pair programming, most will say they don't like someone looking over their shoulder or they don't want to lose personal space. There also is a very common misconception that the pair will only get 1/2 the amount of work done. This is not true for experienced teams, as the chance of unknown areas declines with having two programmers, and the code should be more solid, which saves maintenance time. One other terrific way of improving development processes is to set up a continuous build environment such as CruiseControl.NET. Simply set up the server (a 4-6 hour task for someone new to CCNET) and then set up a notification service, be it email, a notification icon (CCTray is the one for CruiseControl.NET), or a pair of lava lamps (I have seen this, it's very effective). The hardest part will then be getting the developers to do constant checkins, as the build is triggered by a checkin. Then, whenever new code is added, a build is automatically started and any unit tests you have setup will be run. If the build or the tests fail, notifications will be sent out, the notification icon will turn red and popup a message, and the team is now responsible for fixing the build. CCNET will list what build failed and why, and what files were changed and by who. Since you know what files to look at, and you know that fairly few lines of code could have changed (b/c of the frequent checkins), the problem should be able to be fixed quickly. Anyways, I hope the above gives you some more ideas on how to proceed. The true test of whether or not you should stay or leave your current job is in management reaction. If management is not willing to invest their time into fixing their procedures, then I would start to wonder if they are a company that's good for you to work for. Thanks, Seth Rowe [MVP] http://sethrowe.blogspot.com/
I think I'll expand a bit on the lava lamp idea, as it really is a good idea. I originally heard about it from Andy Hunt, one of the Agile founders, during one of his presentations. I'm sure some of the exact details are missing, but it basically goes like this: A team set up a pair of green and red lava lamps in the team room with full visibility to management and had their power sources connected to the build server. When a build would succeed, the green lava lamp would be powered, signaling that all was well. If however the build were to fail, the the green lamp would turn off and the power would be switched to the red lamp. The interesting thing is that lava lamps take a bit to warm up and start flowing, so there would be a time span after a broken build when neither lamps were lit. These lamps were in the team room and had full visibility to management. Depending on the status of the lamps, management could tell the following: 1) Green Lamp - All is well on the project. No involvement necessary. 2) No Lamps - A build error has occurred, but the team is handling it. No involvement necessary. 3) Red Lamp - Significant problem. Might be time to check with the team and see if help is needed. Thanks, Seth Rowe [MVP] http://sethrowe.blogspot.com/
-
I work as a developer in a small software company. The software we produce has a high deffect rate. I want to talk my boss into doing code reviews but the problems that I´m facing are that I don´t find my boss and the other senior developers mature enough to do a real code review instead of a "mine is biggers than yours" code review (they use to talk thrash about other programmer´s code, just because it´s not programmed their way) and that the company promotes individualism insted of team playing (and a code review could be used not to find bugs but to diminish someone else code so they can stand out) So, I do want to get a better software but I´m afraid I could start a monster... Any idea?
-
Code reviews aren't a magic bullet for solving issues of code quality. If your software has a high defect rate there are deeper issues you need to address. You need to take a look at the root causes of your defects, and the most likely cause is a lack of rigorous testing. A good start would be to introduce automated unit testing into your regime, and possibly look at things like Test Driven Development. Unit Testing is a discipline that needs to be worked at but, done properly, it is worth doing and does help.
Deja View - the feeling that you've seen this post before.
Pete O'Hanlon wrote:
Code reviews aren't a magic bullet for solving issues of code quality. If your software has a high defect rate there are deeper issues you need to address
Hey, you have to start somewhere and there is eveidence that code reviews help. Unit testing does help but "Unit Testing isn't a magic bullet for solving issues of code quality."
-
I think I'll expand a bit on the lava lamp idea, as it really is a good idea. I originally heard about it from Andy Hunt, one of the Agile founders, during one of his presentations. I'm sure some of the exact details are missing, but it basically goes like this: A team set up a pair of green and red lava lamps in the team room with full visibility to management and had their power sources connected to the build server. When a build would succeed, the green lava lamp would be powered, signaling that all was well. If however the build were to fail, the the green lamp would turn off and the power would be switched to the red lamp. The interesting thing is that lava lamps take a bit to warm up and start flowing, so there would be a time span after a broken build when neither lamps were lit. These lamps were in the team room and had full visibility to management. Depending on the status of the lamps, management could tell the following: 1) Green Lamp - All is well on the project. No involvement necessary. 2) No Lamps - A build error has occurred, but the team is handling it. No involvement necessary. 3) Red Lamp - Significant problem. Might be time to check with the team and see if help is needed. Thanks, Seth Rowe [MVP] http://sethrowe.blogspot.com/
If you do not want to get into lava lamp just yet, Just set up the CruiseControl.net server. It gives you a fantastic and Free webdashboard. Disable the FORCE compile on the web-dashboard and send the link(usually http://yourcomputername/ccnet) to your management. It will do the trick !
-Prashant