Code Review Best Practices • Hacker News Board

Re: Code Review Best Practices

#101

Post by maguirre » Mon, May 11, 2015, 3:32 AM UTC

dewiz wrote: post #70

maguirre wrote: post #38

I have an interesting problem. A co-worker of mine appears extremely sensitive to his code being reviewed and I honestly don't know how to deal with it. He feels attacked (and becomes defensive during code reviews) because the reviewers focus on the "bad things and mistakes" of his code instead of the accomplishments. Has anyone here dealt with similar issues during reviews?

Try with design sessions upfront, i.e. discuss how the problem should be solved in terms of patterns and architecture. Once that is agreed, with 2-3 people, then the code review becomes simply a matter of style and there you can only invite for consistency with the rest of the code base. Often the problem is in the tools, face to face conversations, pair programming, human interaction help with that. You can also hav…

+1 This is something I've been meaning to introduce. We're slowly moving from the idea of hacking away and making things work to the idea of making production ready code and this mentality is not getting through to everyone

Re: Code Review Best Practices

#102

Post by dsuth » Mon, May 11, 2015, 3:37 AM UTC

pablobaz wrote: post #74

While I agree with all the points listed in the article, it highlights for me a major problem of a lot of code reviews. Most code reviews seem to focus on: 1. Examining what the change does 2. Finding ways to make the change in a nicer way. E.g. Refactoring etc. This leaves out the key step 0 - what is actually trying to be achieved, does it need to be done and is there a better (maybe completely different) way to do…

You should not really be arguing about design and architecture in a code review - that should be done in design review.

Re: Code Review Best Practices

#103

Post by AlisdairO » Mon, May 11, 2015, 10:02 AM UTC

motti wrote: post #99

AlisdairO wrote: post #76

Earlier quoted context omitted.

IE it can help to understand the intent and flow of the code, but also make it harder to debug. If I'm in a situation where I want to follow the exact instructions a large fraction of the code performs, a call stack jumping all over the place can be pretty frustrating.

Step over

...doesn't help if I want to actually see what code is getting run

Re: Code Review Best Practices

#104

Post by nimnio » Fri, May 15, 2015, 6:25 PM UTC

trustfundbaby wrote: post #30

> If the reviewer makes a suggestion, and I don’t have a clear answer as to why the suggestion should not be implemented, I’ll usually make the change This I feel is bad. Code reviews are usually between peers so you shouldn't be afraid to seek out clarification where possible. You shouldn't be making edits to code that goes in production without clearly understanding why. The other thing that wasn't mentioned, that…

From a certain perspective, you just paraphrased the quote you disagreed with:

Him: "If the reviewer makes a suggestion, and I don't have a clear answer as to why the suggestion should not be implemented, I'll usually make the change."

You: "[Do] not act as a blocker for code reviews unless it's absolutely necessary."

I think these express essentially the same philosophy, with some slight differences in the particulars.