18

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
  • 6
    how can you be sure if it is working fine without review by someone that doesn't know the idiosyncrasies you are used to when doing your own testing Commented Jun 2, 2014 at 10:40
  • because they review code after full testing of module by testing team.
    – Himanshu
    Commented Jun 2, 2014 at 10:42
  • 15
    @Himanshu: Review after testing is definitely too late. Review(s) should be done on work in progress.
    – Jan Hudec
    Commented Jun 2, 2014 at 12:18
  • 3
    Embrace this practice. I desperately wish we were doing this on my teams. It helps eliminate knowledge silos (a huge problem for us), and it ensures your teammates can work with your code. If your reviews mean your code gets rewritten sometimes, that's a good thing. Even great programmers write bad code sometimes; some of us would love more time to go back and clean it up. This should be a chance to have a great learning experience with people more experienced than you. Don't take it as people attacking your code; take it as people trying to help grow you into a more experienced programmer.
    – jpmc26
    Commented Jun 3, 2014 at 3:55
  • 1
    If it's common that code that is "working fine" is ripped to pieces during code review to the extent that team members feel that lots of momentum is being lost, then you should perhaps consider pair programming, so the code gets reviewed while it is being written.
    – Buhb
    Commented Jun 3, 2014 at 7:00

3 Answers 3

38

"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
  • 1
    I would add that testing the code doesn't imply there is no bug, limit cases where the software would crash for instance.
    – dyesdyes
    Commented Jun 2, 2014 at 15:23
  • 3
    I agree, and the automatic tests should also be code reviewed as well to be sure they are testing the right thing... Turtles all the way down.
    – Xavier T.
    Commented Jun 2, 2014 at 15:35
12

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.

3

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
  • 1
    Integration tests are also extremely useful for detecting breakage.
    – jpmc26
    Commented Jun 3, 2014 at 4:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.