In my company, before delivery of any project, my boss asks my seniors to review programs written by me or other team members or sometimes boss also sits with us for review.
I think it is a good way of getting knowledge, but sometimes when programs are working fine, they don’t work same after review and I need to look again into my program.
They says that review helps to optimize program and query execution, but can we prefer optimization over actual functioning of program?
11
“Working fine” is indeed a great metric, but if you are the only one in the team able to decipher what you wrote, and thus maintain it, the code is close to worthless for the company for the mid or long-term.
A good code is at least :
- working as intended
- human-readable / clear
- easily maintainable
- easily extensible for future changes
- safe
- without unneeded dependencies
- handling correctly non nominal cases
- etc
(Some of these requirements are actually overlapping but are good to consider individually…)
Code reviews serve the purpose beyond the “working” part, which can be done through automatic tests.
I personally know this is annoying to have something working being teared apart, and having to rebuild it from the ground up. But, often, this is due to a miscommunication from the senior/tech lead. So, if you think you have to rewrite too often, next time, go to the reviewer before writing a single line and try to get as much information as possible on what he is expecting, in every detail. It could also be great if the team of code reviewers summarize their expectation in a formal document that every dev can refer to.
On a more positive side, a session could also be an occasion to share great practices/designs.
2
I interpreted your question as “Can my functioning code be butchered in a review to a point where it doesn’t even compile anymore?”.
Yes, it can. Generally, during a review you look at how your code does what it does. When you want to hand in your code, you say you have finished a certain part of the program.
You say it works. Testing is then done to verify this. A module passing tests does not mean that module should not be touched again.
A module appearing functional can still be a disaster waiting to happen, either at runtime or in a few months when you or someone else has to perform maintenance on it. By changing your code in a review and pointing out what was wrong with it, your reviewer is (hopefully) trying to actually teach you something.
Peer reviews are no doubt a great way of learning. Someone may see something different, they have different experience to you and should be able to contribute improvements. This shouldn’t be disparaging, I’d expect any developer to be able to comment and constructively criticize anyone’s code!
It sounds to me like some of these “improvements” are actually making breaking changes because (as you’d expect) the reviewing developer has less experience with the software than the author.
This trend is it’s self feedback, perhaps your code is difficult to follow or maintain? Are your reviews valuable? Absolutely! I can see how it can be frustrating, to have working code which your peers then appear to break, you shouldn’t be disheartened – you should work to protect your code against these changes.
The question then becomes how to protect the functionality of your programs so you know the functionality is still working after you’ve completed your reviews. My suggestion would be to ensure you have decent unit test coverage. That way whenever you/your reviewer/your successor changes the code they can be confident that the changes they’ve made are safe.
ETA: I’ve just spotted one of your comments, I’m sure this goes without saying but code reviews should be done before the test team get their hands on it. Otherwise they’re not testing the final product.
1