Submitted by pedantix on
We need an open forum where developers can talk to one another about issues in one another's code. Every developer in the team supports the same code base, each of the developers works on code written by other developers. When they find an issue they have two options: (1) silently fix it, or (2) tell the original developer (and decide who will fix it). Being non-confrontational types, most often they follow the path of least resistance. But that means the original developer never finds out that his code was buggy. They may not change their ways. Inefficiencies in learning and also in coding - more buggy code continues to be produced.
Any ideas on how I can start a code review / code feedback session between developers?
Revision and irc
we have done that a while ago with a running chat client (irc, hardcore developers) AND a revision control system.
Each time a developer checks his code in he sees differences.
Beside that i would suggest a small 5-10 Minute "peer code review" standing meeting where each developer talks about what he believes is better.
Remeber ther effectiveness of face to face communication, espacially when it comes to confronting people with errors. Depends on the relationships of the team members together
Doing that the whole team benefits from the pearls of wisdom from the "fixer"
If you are the manager,
If you are the manager, require that all code be peer-reviewed before it is checked in. You are using a version control system, right? ;-)
If your'e not the manager and work on a team of high-Cs you could say, "Hey, I found this way where we can spend more time writing new code and less time fixing bugs; 80-90% of the bugs get fixed before testing; and the number of times a customer calls up whining about something is cut in half!"
Having the team choose/adapt/create the process usually gives better results. I've often started out with one of several "standard" peer review processes. The one to use as a base greatly depends upon your current or desired culture - Fagan Inspections being the most formal and "desk checks" being the least, with "Walkthroughs" and "Technical Reviews" being in the middle. There is no "One True Way" to do peer reviews; each has strengths and weaknesses. Some good information can be found at http://www.processimpact.com/pubs.shtml#pr (no affiliation).
Take a step back
Before you jump in and start peer reviews, I would suggest you make sure the problems being addressed are well understood.
If there are team members that are producing buggy code, the manager should be addressing this (ideally with feedback). Peer reviews should not be used as a way to flush out under-performers.
If quality is a concern for the whole team, are there actionable metrics that shed light on the issue? Do you track bugs per deliverable? Do you have dedicated testers? How do you define quality for your organization? I have seen peer reviews turn counter-productive as the line between preference and correct gets blurred.
It is a good idea to have standing meetings to raise technical design issues, especially for deliverables that do not follow an existing pattern. This might be an alternative to a blanket approach of reviewing every line of code. In my experience, the stumbling blocks that lead to missed deadlines and poor quality can be vetted out in a discussion of the approach rather than waiting for the code to be done. It will save you time and headaches, and the coder is less vested in an approach prior to implementation.
I strongly support Mtietel's suggestion of asking the team to own the process, I would just suggest that you have them focus on the real business problem before jumping to "how do we review code".
Hope that helps,
I agree with Mike Hansen
I agree with Mike Hansen that waiting with reviews until the code is "done" is too late, unless you have metrics that show your requirements and design are perfect (I'd bet against that given the mountain of existing industry data :-). Personal experience with data I've collected shows about 50%-30%-20% for the distribution of requirements-, design-, and code-level root cause for defects. Interestingly in this particular area I've seen little difference between Waterfall-ish and Agile-ish methods - while Agile-ish methods shorten the feedback loop and get to what the customer wants more quickly, there are still misunderstood or missing requirements along the way.
A well designed review process will work for other artifacts also, not just code. We've used ours for requirements/stories, architecture, high & low level design, code, test plans, test cases, etc. Each artifact type has a separate checklist that evolves over time as we learn (i.e., stop making the same mistakes and start making new ones ;-)
My intention is around two
My intention is around two aims:
1. That each developer learns from skills the others have, i.e. sharing ideas on how to tackle any individual coding problem
2. That the standards we have get followed, ito anything from variable naming conventions, the use of linked lists vs arrays, the preferrable use of foreign keys in the database, through to reuse of our existing frameworks and components.
I'm the manager, not one of the coders. The forum I'd like to setup would give the developers a chance to discuss and share, even critique, without the fear of criticism. I've been thinking along two separate lines; either coaching some of the seniors in making presentations to the group, or (my question above) one-on-one peer code review.
Some useful ideas here and I'll dig into some of the links and ideas mentioned. Thank you for sharing. It's very much appreciated!
Make it part of your process
BLUF: Make it part of your process.
I think it comes down to the process you follow to get something from an idea to release.
My team have code-review as a process step in everything we release and therefore nobody feels uncomfortable.
Two guys on the team are only 18 months out of Uni, but they're doing a fantastic job doing code review -- even on our Tech Director's code (don't ask .. it's a weird org-chart). Because senior developers review their code and provide feedback on methodology and optimisation as well as a general review, they've learned to do the same thing.
Because it's part of the process, everyone is happy to do it and everyone is happy to receive it. But the best thing of all: the code review knocks down many silos. Once someone reviews your code, they have a fairly good overview of it and could step in and make adjustments or even take over the task if need be.
The only similar issue we have to you is from legacy code that was written years ago before we defined a solid process.
The bottom line again: Make it part of your process. That removes any uncomfortableness.
Read my blog: Geek Herding - Explorations in the art of leading IT professionals
Transforming company culture: A difficult endeavor
If your company has no history of doing peer code reviews, this is going to be a long, difficult journey for you. You're trying to introduce a new quality measure into the workplace. You--like most of us--are convinced of its value. However, most of your coworkers will see it as unnecessary, "in-the-way", wrong-headed, and a waste of time. They will resist you at evey turn.
To transform the way work is done, there are usually two ways:
* If you're a manager, you can decree it, and enforce it. This can work, but the manager has to be a strong leader and have the discretion to make the judgment call.
* If you're not a manager, you have to build grassroots support. Right now, your first goal should be to find a single fellow co-worker who shares your vision. Then, the two of you voluntarily adopt peer reviews as standard technique. Do NOT announce generally that you are doing this. Just do it. Record the results of the reviews. Make sure you record how many defects are found, as well as the predicted impact of letting those defects be submitted. Do this reliably for about 6 months. At that point, look for another participant. If you can find one, loop him into the process. One by one, you can bring everyone in. You'll eventually reach a tipping point where the behavior is expected as normal, rather than being quirky.
It's a long slog. Also, you need to be prepared for the reality that your company will NEVER adopt this practice. If you try for a year and they still don't get it, MOVE ON. A company that can't embrace peer reviews is a company that can't tolerate self-criticism. In the long run, this kind of company always declines.
Implement change control and
Implement change control and ensure that any changes outside the scope of work is tracked and approved by a supervisor. Doing so not only captures these version changes but also ensure that staff follow a methodology and process which invokes the necessary thoughts and questions to each and every change thereby given you full control of the coding process and ultimately the desired end result.
You Career as a Bank Teller