Thursday, February 19, 2015

A story of one code review

P1010524


As told by a developer: 

A. and D. told me that the changes that were done to method “E” on class “C” are not needed, and also those two test cases for these changes were not needed. So A. and I removed those. I promoted these changes to my stream.
I don’t really understand why. 
Also A. said that the test “T” should make sure we are asserting that those values are really still there. I thought it did do that, but I guess not.
Also D. suggested an improvement to the code that I don’t fully understand: In class “U” method “D” we can pass the “c” into the “T” method on line 35 and then we don’t need to pass it in on line 39. I didn’t catch exactly what he was saying to do.
I may be missing some other things too.


I hope the code was improved as a result of the changes made based on this code review.  But even so, this is not the best way to run code reviews.

Code review is there to get all the team members on the same page in regards to writing good code, catch hard-to-detect issues, and learn from each other.  The discussion that happens in code review is an invaluable tool in building a strong technical community, and ensuring the technical quality immediately and over the medium- and longer-term. It is important that all participants gain an understanding of what changes would make better code and why, in addition to simply following typing instructions.

Here's a slide deck "Effective Code Review" for a discussion about code review practices , that focus on learning, creating a community with a common vision, and building social capital.

P1020736

No comments:

Post a Comment