Here is a plea
From my heart to you
Nobody knows me
As well as you doDepeche Mode, Ramat Gan Stadium , May 2009, Assuming no new war in the next two months
You don’t have to do code reviews, but if you do, do them the right way.
Code review is a managerial process, IMO, and should be done by managers, not peers. The reviewer does not inspect just the code, but also the development process, guidelines and state. For example. If a peer detects a problem requiring a change in the code, a fix will happen for the specific problem. If the manager detects a difficulty in the code, he can make sure the cause is fixed. Peer review is an interesting tool and has its own benefits, but the two should not be confused.
The great benefit for the reviewing manager is education. He learns about the real problems his engineers are facing, the real status of code, documentation and people skills.It also allows him to stay connected with the source. It is a great trade-off point between easy-to-shout-global-always-true declarations ( “No code with printf, ever”) to the reality of day to day (“printf is ugly, but it works”).Of course, he gets to learn more about core issues in the code worth fixing. The same issues that he never prioritized.
There are some nice guidelines here http://www.macadamian.com/codereview.htm and below are some bullets you might want to use in your own process.
1. Was the change tested to work in real environment?
2. How do you KNOW if does what it is supposed to do?
3. Are there potential similar bugs in the code? Were they fixed?
4. Is the change\feature documented in the code?
5. Are there unit tests to check the new feature?
6. Are there any compilation warnings? Why?
7. Is there an open ticket for the change?
8. Has the change been checked for security issues?
9. Did you check memory allocations and releases?
10. Is the change accompanied with right debug messages and logs?
11. Functions have clear descriptions, return values, input and output
12. Standard type definitions are used
13. No arbitrary constants are used in the code
14. What are the risks involved in the change? How are they minimized?
15. Does the reviewer fully understand what the code does?
16. Are state machines documented as such (clear states and transitions)?
17. How will the change affect performance? Did you run a profiler?
18. Was the design document updated to reflect the change?
19. Textual messages should be check for spelling and clarity
20. How are errors, exceptions and lack of resources dealt with?
21. Can the new functions be useful to other team members? Did you tell them about it ?
Cat Doing Code Review
