#Example:
Example:
#Good:
Good:
#Bad:
Stack Exchange network consists of 183 Q&A communities including Stack Overflow, the largest, most trusted online community for developers to learn, share their knowledge, and build their careers.
Visit Stack ExchangeStack Internal
Knowledge at work
Bring the best of human thought and AI automation together at your work.
Explore Stack Internal#Example:
#Good:
#Bad:
#Example:
#Good:
#Bad:
Sorry for yet another long answer, but I don't think the others have fully addressed the human element of this issue.
sometimes even just asking why something was implemented in a specific way. When I think it is bad, I point out that I would have developed it in another way.
The problem with "Why did you implement it this way?" is that you put the developer immediately on the defensive. The question itself can imply all sorts of things depending on context: Are you too stupid to think of a better solution? Is this the best you can do? Are you trying to ruin this project? Do you even care about the quality of your work? etc. Asking 'why' the code was developed a certain way is only going to be confrontational, and that subverts any pedagogical intent your comment might have had.
Similarly "I would have done this differently..." is also confrontational, because the immediate thought the developer will have is "Well I did it this way... You got a problem with it?" And again, it's more confrontational than it needs to be and turns the discussion away from improving the code.
#Example:
Instead of asking "Why did you choose not to use the constant variable for this value?", simply state "This hard-coded value should be replaced with the constant XYZ in file Constants.h" Asking the question suggests that the developer actively chose not to use the already-defined constant, but it's entirely possible that they didn't even know it existed. With a large enough code base, there's going to be a lot of things each developer doesn't know. This is simply a good learning opportunity for that developer to get acquainted with the project's constants.
There's a fine line to walk with code reviews: you don't need to sugar coat everything you say, you don't need to sandwich bad news with good news, and you don't need to soften the blow. It could be the culture I'm in, but my coworkers and I never compliment each other in code reviews - the parts of the code we develop that are defect-free are enough of a compliment. What you need to do is identify the defect you see, and perhaps give a reason why (the why is less useful when it's obvious/simple).
#Good:
"The name of this variable needs to be changed to match our coding standard."
"The 'b' in this function name needs to be capitalized in order to be PascalCase."
"This function's code isn't indented properly."
"This code is a duplicate of code found in ABC::XYZ(), and should use that function instead."
"You should use try-with-resources so that things are guaranteed to be closed properly in this function, even if errors occur." [I only added a link here so non-java users would know what try-with-resources means]
"This function needs to be refactored in order to meet our n-path complexity standards."
#Bad:
"I think you could improve this code by changing the variable name to match our standard"
"Perhaps it would be better to use try-with-resource to properly close things in this function"
"It might be desirable to take another look at all the conditions in this function. Its N-path complexity is over our standard's maximum allowed complexity."
"Why are the indents here 2 spaces instead of our standard's 4?"
"Why did you write a function that breaks our n-path complexity standard?"
In the bad statements the italicized parts are things that people commonly use when they want to soften the blow, but all it really does in a code review is make you sound uncertain of yourself. If you, the reviewer, aren't certain on how to improve the code, then why should anyone listen to you?
The 'good' statements are blunt, but they don't accuse the developer, they don't attack the developer, they're not confrontational, and they don't question why the defect exists. It exists; here's the fix. They're certainly not as confrontational as that last 'why' question.