Should my colleagues review each others code from source control system?

Question

So that's my story: one of my colleagues uses to review all the code, hosted to revision system. I'm not speaking about adequate review of changes in parts that he belongs to. He watches the code file to file, line to line. Every new file and every modified. I feel just like being spied on!

My guess is that if code was already hosted to control system, you should trust it as workable at least. My question is, maybe I'm just too paranoiac and practice of reviewing each others code is good?

P.S: We're team of only three developers, and I fear that if there will be more of us, colleague just won't have time to review all the the code we'll write.

Answer 1

I would say YES!

Two quick reasons for it:

1) If code is in production, you cannot assume that it is correct. Any change elsewhere in the system can introduce bugs. I think it is very important that code be checked regularly. This way, refactoring is done on a regular basis, keeping the code neat and "more" correct (up to date is probably bettter).

2) Being able to read code is a very important skill if you are going to be a programmer. And a skill it is, something you need to work on. For any programmer starting work on an existing code base, if he is not used to reading other people's code, there is a steep learning curve trying to get up to date with what is going on.

I do not think you should feel spied on. Accept any critique someone gives you (if it is valid of course). And feel free to give other people VALID critique. It is the way we learn. Once we stop learning (or want to stop), then there are big problems.

Answer 2

If said colleague provides good and constructive feedback, this is a fantastic thing and you should appreciate it very much.

This WILL catch bugs you didn't think of when writing it. This WILL result in you writing better code because you know that others will see it.

Answer 3

It would be healthy if the entire team does code reviews instead of one person. Ideally everyone would invite someone to review their code after completion. It's helpful to keep it informal (keep managers away) and let the reviewer talk you through his/her findings. Ideally the reviewer only gives feedback and doesn't do code changes, of course you could pair on it a little.

It really helps to have coding standards to avoid review discussions being about white space and code style constantly. Having some static code analysis on a build machine can be helpful in keeping some discussions out also.

About the time aspect, the theory is that it will save you time. The later faults are found the more expensive they get, fail fast principle. Peer code review can catch quite a bit of problems.

Answer 4

Your colleague sounds like a diligent developer, you should follow his example.

Answer 5

I watch our version control system in a similar way. Our codebase is too large to watch every line, but I try to get a high level feel for most changes. I also watch for checkins that are most likely to have side effects and review those line by line. For the minimal time I spend doing this, the payoff is huge. (Also note: I am not the only developer on our team with this habit.)

This kind of review tends to catch bugs or invoke discussion on a weekly basis. That saves time when doing QA. The discussions range from best practices to algorithm design and more. The key on this front is that everyone views it as constructive.

Personally, it also gives me a greater understanding of what is going on in other parts of the codebase that I don't touch regularly. When others need help, I am able to jump in more quickly. Also, when new ideas appear I can take advantage of them sooner.

Answer 6

You feel it as being spied on(!)? But from your colleague perspective I would say he is doing the right things for his career development. Read others code and find how do they design and implement the logic, this will gain you a lot!

IMHO if some one point out something wrong in your code, you must accept it and learn from them about how to write a good code

Answer 7

During 6-7 months I was doing the same. Not to spy, but to control the quality. Every single line of the code for an actively developed application, committed to the central repository, 2 main languages, a few another languages, huge makefiles for 4 platforms.

It is very bad practice. Some day I found out that I cannot capture everything due to the robustness. The another argument against this is the subjectivity - everybody may get wrong.

It is better when developers review each other's codes and there is someone experienced to make final decisions and define directions.

Answer 8

Code reviews within a team (using fisheye, crucible or other tools) are extremely important and useful. the only thing better is direct pair programming to make sure that code that gets into the system the first time is well thought out and has gone through the brain of more than one person.

Answer 9

This happened in my team once. Unfortunately it resulted in a blame game. People continously waited for others to check-In the code and would always try to find something wrong in it and played the blame game all the time.

I hope you have a more matured audience.

Answer 10

This is quite standard practice in industry. The companies I have worked at have very tight code review guidelines. One even wouldn't let you commit unless code had been reviewed.

Don't take offence, or feel watched. Think of it as a safety net and a learning experience.

Answer 11

At a previous job, the senior developer watched and reviewed all check-ins and I frequently got excellent feedback that helped make me a better developer.

At my current job, I watch many of the check-ins and three days ago I found a bug and notified the developer.

This practice absolutely will catch bugs and make your entire team better, if you embrace it.