Very informative and though-through article, Tom. Can you, please, comment more on actual code review tools you are using or can suggest to use in order to achieve better results. Thank you in advance.
Hi Natali, I don't know about Google but I know that we have used Crucible/Fisheye as well as Bitbucket Pull requests, both integrate well with Jira and Confluence since they're all Atlassian products.
Hey, Natali, thanks for the kind words! I'm afraid I can't really advise on which tools to use. Google's main code review tool is proprietary. Android OS code goes via gerrit, which is alright, but I don't know how it compares to other externally-available code review tools.
Hmm, it sounds as if Docs is perhaps stuck in offline mode, or that your device is offline at the time you're clicking to the printer friendly doc. Does that sound possible?
Thanks for the great article! I was just looking at tickets in our tracking tool today that had 25 pull requests on some and others had hundreds of lines of code across a dozen files. I'll definitely bring this to the team so we can discuss being more effective on Code Reviews and more respectful of others' time.
Wonderful article. I really like the comment that says "respect your code reviewer's time, its a limited resource", many programmer just ignore that fact that code reviewer also has to code and he also has to deliver. My following these practice, you will learn more and also save time. Btw, I have also share my 2 cents on code review @ http://javarevisited.blogspot.com/2011/09/code-review-checklist-best-practice.html#axzz4keRfZjdH . Some of you may find worth reading. Thanks
That's the time to go speak to the reviewer face-to-face. Don't try to hash out arguments through a review tool. Calmly discuss why you think your way is better, ask them for their reasons for suggesting the changes, and come to a mutual agreement. If that's not possible, ask a neutral third-party (like a tech lead) to consider both sides and give their opinion.
liked the comment "Step back and try to rethink the whole thing—can the design be cleaned up? Especially if it’s late in the day, see if a better approach occurs to you the next morning"--> many just push the raw code as dead line requirements. its better be late than being worst.
I think it is important to point out that you may be getting too many comments because your CL is too big. I used to make very big CLs (1000+ loc) and my reviewer would say that is fine (I don't think it is ever fine, unless you're submitting a big data file) and go ahead and review, but the review would take very long and toil away at both our toleration limits, since we would run into many sticky points that required additional discussion.
Very informative and though-through article, Tom. Can you, please, comment more on actual code review tools you are using or can suggest to use in order to achieve better results.
ReplyDeleteThank you in advance.
Hi Natali, I don't know about Google but I know that we have used Crucible/Fisheye as well as Bitbucket Pull requests, both integrate well with Jira and Confluence since they're all Atlassian products.
DeleteThank you, Paul. Actually we are trying them out within our organization as well.
DeleteHey, Natali, thanks for the kind words! I'm afraid I can't really advise on which tools to use. Google's main code review tool is proprietary. Android OS code goes via gerrit, which is alright, but I don't know how it compares to other externally-available code review tools.
DeleteThanks for the article. Also kindly check the "printer-friendly version" link. It seems to be broken.
ReplyDeleteThanks for the heads up! It appears to be working for us. What happens when you click the link?
DeleteI goes to Google Docs page showing
Delete"You're offline.
This page isn't available offline. Connect to the Internet and refresh to view it online."
Hmm, it sounds as if Docs is perhaps stuck in offline mode, or that your device is offline at the time you're clicking to the printer friendly doc. Does that sound possible?
DeleteGood article really appreciate it!!
ReplyDeleteThanks for the great article! I was just looking at tickets in our tracking tool today that had 25 pull requests on some and others had hundreds of lines of code across a dozen files. I'll definitely bring this to the team so we can discuss being more effective on Code Reviews and more respectful of others' time.
ReplyDeleteWonderful article. I really like the comment that says "respect your code reviewer's time, its a limited resource", many programmer just ignore that fact that code reviewer also has to code and he also has to deliver. My following these practice, you will learn more and also save time. Btw, I have also share my 2 cents on code review @ http://javarevisited.blogspot.com/2011/09/code-review-checklist-best-practice.html#axzz4keRfZjdH . Some of you may find worth reading. Thanks
ReplyDeleteWhat if code reviewer is not right, and forcing to make changes before being approved. It delays the things a lot.
ReplyDeleteThat's the time to go speak to the reviewer face-to-face. Don't try to hash out arguments through a review tool. Calmly discuss why you think your way is better, ask them for their reasons for suggesting the changes, and come to a mutual agreement. If that's not possible, ask a neutral third-party (like a tech lead) to consider both sides and give their opinion.
Deleteliked the comment "Step back and try to rethink the whole thing—can the design be cleaned up? Especially if it’s late in the day, see if a better approach occurs to you the next morning"--> many just push the raw code as dead line requirements. its better be late than being worst.
ReplyDeleteI think it is important to point out that you may be getting too many comments because your CL is too big. I used to make very big CLs (1000+ loc) and my reviewer would say that is fine (I don't think it is ever fine, unless you're submitting a big data file) and go ahead and review, but the review would take very long and toil away at both our toleration limits, since we would run into many sticky points that required additional discussion.
ReplyDelete