Code reviews

[Steve Naidamast]

I have promoted software engineering concepts for years in two different companies with absolutely no success. Stephen McConnell, one of the leading software engineering analysts in the United States today has often stated that attempting to bring in such concepts and techniques is often a very difficult and unsuccessful endeavor. Software engineering requires a level of intelligence that is often not found with technical management...

Steve Naidamast Black Falcon Software, Inc. blackfalconsoftware@ix.netcom.com

[DumpsterJuice]

In my 20+ years, I have never seen code reviews do more than correct minor programming errors, and waste time. I know this is going to rile up some folks, but based on the reality of my experience, I have decided that code reviews are a nice concept, but never productive. The longer I code the more I recognize the shiny things that make my work slower... like introducing more and more process and admin, that magically reduces my coding hours...in theory. We have not moved very far in the software world, I think we all agree that the physical construction business is one we envy for their "Blue-Prints" and repeatable, modular techniques. Software can never really get to this level due to many factors... not the least of which is that our construction techniques, and process change far to often and too radically. Not just my opinion. My experience. I know you have different experiences, and thats ok... I still believe somewhere out there someone is doing all these things right. I just have never seen it.

Where there's smoke, there's a Blue Screen of death.

[ewshaffer]

I worked for over 8 years at a company that had a formal code review process that was essentially integrated into the development life cycle (they worked on contracts requiring quality standards like CMMI and such). It worked very well there as it was a REQUIRED part of the process where a separate Quality Assurance engineer who understood code well enough to read it but did not actually write it and thus had no ego invested. The developer would put together a review package which was sent to other team members, the team lead, the QA engineer, and (when we could get one) someone outside the team (thus, they had no ego invested in that code either). We had a management-approved coding standard and they also agreed to having code review time (and unit testing) built into the schedule. QA also had the authority (though they never needed to exercise it) to prevent the entire contract from advancing to the next stage because they were required to sign off on it. If the QA mentioned that they were concerned about someone who resisted the process, you can be sure that management would get on the develper's case and they would either adjust their attitude, get reassigned, or start looking for another job. Similary, we would try to enforce constructive reviews and focused on looking for functional (not style) problems or made suggestions about alternatives (XX wrote a utility class which would simplify that, ...). If someone became nasty or non-constructive, they would get a visit from management. This worked because of complete management backing and the fact that developers were not allowed to short change reviews and testing due to the schedule (maybe because the customer originating the contract had to accept it with a formal test procedure approved by separate test engineers and QA as well, and it was also not tied to things like marketing and release dates). I've also been working for another large company that does not do them, but has occasionally expressed interest in it. I think that it will not really get anywhere until management really throws support behind it as it needs to become more of a "cultural" part of the process with time allocated towards the process. QA here is also more involved with testing in multiple environments and trying to find problems, but they never look at code (and I doubt most if any of them would really be able to understand it well enough to even "read" it)

[Jim SS]

Code reviews are not great for debugging code, compilers and testing are much more efficient at that. Where code reviews are very useful is with algorithms, efficiency, and teaching. We think we know a lot when we start out programming, but experience brings a lot to the table. People with more experience on the platform or the field you are programming in can easily catch faulty assumptions that you would never see. Also, new guys can bring in ideas that you are unaware of because you took your last class twenty years ago. I did a lot of code reviews at IBM, CSC, WordPerfect, but haven't done any in a long time. You have to put your ego aside, but I learned a lot from them. Now my code reviews consist of looking at someone else's code to see if I can find a bug that he can't see. I think we (in my current employment situation) are missing some great teaching and learning opportunities.

SS => Qualified in Submarines "We sleep soundly in our beds because rough men stand ready in the night to visit violence on those who would do us harm". Winston Churchill

[Lost User]

Code reviews are a key facet of any mature development process, as are unit tests, integration testing with published test plans, functional testing, performance engineering, etc. I work in the financial industry where defect management is very important, although perhaps not as important as the airline industry, nuclear power, automobile, etc. where people's lives could be at stake, but close. So process is critical to ensuring quality. What is required is mature leadership, which sounds to be lacking at your organization. There are lots of models to follow, and frankly everyone's model is a bit different even if it goes by the same name. Our particular model takes advantage of lower cost offshore developers with project leads/managers both on site and offshore. We have junior and senior devs offshore, but frankly, the seniors are still pretty fresh, and the juniors are really raw beginners in a lot of cases. Still we seem to make it work because we're loaded with process and everything, from assessments and SWAGs to QA strategy to functional test plans to production code, everything gets reviewed. And nobody takes it personally, from the senior architect (me) to the designers, QA personnel, and lowest programmer. Everyone buys into the philosophy when they join the team or they don't join. And we've never had a problem with this.

[Dave Buhl]

Everyone has provided great comments but I wanted to throw in a bit of personal experience. There is no magic bullet for fixing your software defect issues, and applying a tool in the wrong way can be more damaging than helpful. You can't take a cookie cutter process and apply it to every organization and expect it to work. There are some basic concepts that do apply that can be very helpful. First things first, if you are trying to change an organization from the bottom up you have to start slow and fix yourself first. I would recommend trying out Test Driven Development where you write your unit tests based on requirements before writing a single line of code. Each time you think you satisfy a requirement, build and run the tests. You will write better code faster and only add features that are required (it helps you focus). Once you can show how it has improved your coding, have a mate (not the same one each time so as to get a lot of exposure) look over your code occasionally for an informal code review and introduce them to the TDD if they are open to listening. If you can start to sell the process to a few folks, you will see productivity start to grow and defects start to drop in the group for those that are using the process. Now you can approach management with what you have done and show the results, they will be more willing to listen because it is no longer academic theory but visible results. When you go to a manager to try to get something to change, bring them a plan to make the change and something to show that the change will be worthwhile, if you just say we need a change but dont provide a plan and evidence it is hard to decide to drop what the manager is working on to figure out what you are trying to do. Remember, change is a lot of work and no one wants more work unless it means less work in the medium/long term. I recommend the following books for ideas on improving processes and code in general: Code Complete 2nd Edition by Steve McConnell[^] Extreme Software Engineerin

[dilton_dalton]

AHAAAAAAAAAAAAAAAAAAAAARRRRRRGGGGGGGGGGGG! Lack of testing is the root cause of defective code!!!!! I'm sorry, but I think I may have fallen down the wrong rabbit hole. To: Gabriel.P.G People make mistakes. In software development we call these defects. A lot of people believe that the rate of defect introduction is a function of experience but studies have shown that if there is a relationship between experience and defects, it is so much smaller than the person to person variation that it is not significant. If the code produced by your organization starts out with a lot of defects, start looking at who puts them into the work products in the first place (work products include requirements, designs, code and test cases). Then worry about verification. Code has to be tested. No argument there. But code reviews are the best way to find defects. Inspections are better than informal reviews. The formal definition of an inspection is, "the visual comparison of a work product to its parent specification or applicable standard." A proper inspection process will find a larger fraction of the defects present than testing can and it will cost less. The cost aspect is something that your management should be worried about even if they are not totally mature. You can't build a perfect system out of defective parts. So it is very important to focus on the components that are built before they are integrated. Inspection followed by unit test coupled with a reasonable acceptance criteria will get the majority of the defects before integration. Testing during the integration process will find a reasonable fraction of those remaining. Once the system is integrated, testing can only be used to confirm that the developers did a good job. Nobody has the resources to test a significant sized system enought to find a significant fraction of the remaining defects. If the system is buggy at that point, you have to go back to inspections and unit testing (and perhaps redesign). If you try things like test driven development, be careful to make sure that the change actually improves things.

[CDMTJX]

I can only suggest finding an open minded buddy who might help out by finding issues. Unless you have a few people who see the worth of code reviews, your boss will probably think the effort worthless. After all, it takes time which could be spent writing bugs. And the "individuals" will resist like you say...

[Stuart Rubin]

Let me clarify my position and experience a little. There are two main points. 1. My experience is that a code review is most effective as a code "talk through" with the author and two other people. (More is a waste.) The bugs are more often found by the presenter/author of the code. When you talk about what you've done in English (or whatever your spoken language is, not C/C++/Java/...) you very often catch small mistakes. It's a lot quicker to detect and fix a problem this way than when you're running real tests. (This is well documented in the research.) Also, if you can't "explain" line-by-line what your code does without floundering, it's probably poorly crafted. 2. The longer-term upside of code reviews is that you begin to construct code that you know other people are going to read. You begin to become more conscientious about good practices. Your code naturally becomes more debugable, manageable, scaleable, etc.

[eric_tometa]

When I worked for Hewlett Packard, we looked upon reviews and criticism as valuable feedback, and thanked the giver. This should be made part of the company culture. It worked extremely well at a company known for its quality. When you bring this up to the boss, emphasize the idea that useful feedback is the best way to write flawless software (and therefore save $$$). The "mine is bigger than yours" review is less than useless - it actually hurts the culture of the company, and puts the reviewed person on the defensive, so he/she is less likely to change the software for the better, and more likely to "dig in" and find ways to advocate for "their way".

Eric Cox, Software Architect Playxpert, LLC. http://www.playxpert.com

[Grimolfr]

Pete O'Hanlon wrote:

the most likely cause is a lack of rigorous testing.

In my experience the most likely cause is incompetent developers. The only thing you'll get from more rigorous testing is a higher defect count. (Which is not to say that I don't believe in rigorous testing, I just don't believe it will solve the root problem.)

Grim MCDBA, MCSD, MCP+SB SELECT * FROM users WHERE clue IS NOT NULL (0 row(s) affected)

[unchecked]

Find a new company that will satisfy you, that will hear you, that promotes team playing.

On Error Jump Off the Building