Skip to main content
Software Engineering

Return to Answer

Commonmark migration
Source Link
Community Bot
Community Bot
  • 1

I'm surprised it hasn't been mentioned in any answer so far, and perhaps my experience with code reviews is the unusual one, but:

Why are you reviewing the entire merge request in a single message?

My experience with code reviews is via GitLab; I always imagined that other code review tools would work similarly.

When I am giving a code review, I comment on specific, individual lines of the diff. This is extremely unlikely to be received as personal criticism, because it is commentary on the code—and is actually displayed as commentary on the code, shown directly below the code it is a commentary on.

I can also comment on the entire merge request, and often do. But specific points can be pointed out on specific lines of the diff. (Also, when a new commit is added such that the diff changes, the comments on the "outdated diff" are hidden by default.)

With this workflow, code reviews become much more modular and less cohesive. A line from a code review can simply say,

Nice approach, wrapping this in a dedicated function!

Or it might say,

This object name doesn't really match the purpose of the object; maybe we could use a name like 'XYZ' instead?

Or if there are major problems with a section, I might write:

I see that this code works (and I see why it works) but it requires focused study to understand it. Could you please refactor it into separate functions so it will be easier to maintain in the future?

 

(Example: Function ABC is actually doing three things here: bazzing the foo, barring the boz and fripping the zorf. Those could all be separate functions.)

After writing all of the above, I can make a summary comment on the entire merge request, something like:

This is a great new feature and it will be really useful once merged. Can you please clean up the function naming, and handle the refactoring mentioned in the individual comments I've made, and then let me know to look at it again? :)

Even if the merge request is a complete dog's breakfast, the individual comments can each be simple. There will just be more of them. Then the summary comment might say:

I'm sorry, but this code isn't really up to snuff. There are a great many edge cases (as detailed in individual comments) that will be handled incorrectly and give a bad user experience, or even data corruption in one case. (See comment on commit 438a95fb734.) Even some normal use cases will result in extremely bad application performance (specifics noted in individual comments on the diff for somefile.c).

 

To be production ready this feature will need a full rewrite with more attention given to what assumptions are safe to make at every point in the flow. (Hint: No assumptions are safe unless they've been checked for.)

 

I'm closing the merge request pending a full rewrite.

Summary: Review the technical aspects of the code as comments on individual lines of code. Then summarize those comments in an overall comment on the merge request. Don't get personal—just deal in the facts, and in your opinion about the code, not about the coder. And base your opinion on facts, accurate observation, and understanding.

I'm surprised it hasn't been mentioned in any answer so far, and perhaps my experience with code reviews is the unusual one, but:

Why are you reviewing the entire merge request in a single message?

My experience with code reviews is via GitLab; I always imagined that other code review tools would work similarly.

When I am giving a code review, I comment on specific, individual lines of the diff. This is extremely unlikely to be received as personal criticism, because it is commentary on the code—and is actually displayed as commentary on the code, shown directly below the code it is a commentary on.

I can also comment on the entire merge request, and often do. But specific points can be pointed out on specific lines of the diff. (Also, when a new commit is added such that the diff changes, the comments on the "outdated diff" are hidden by default.)

With this workflow, code reviews become much more modular and less cohesive. A line from a code review can simply say,

Nice approach, wrapping this in a dedicated function!

Or it might say,

This object name doesn't really match the purpose of the object; maybe we could use a name like 'XYZ' instead?

Or if there are major problems with a section, I might write:

I see that this code works (and I see why it works) but it requires focused study to understand it. Could you please refactor it into separate functions so it will be easier to maintain in the future?

 

(Example: Function ABC is actually doing three things here: bazzing the foo, barring the boz and fripping the zorf. Those could all be separate functions.)

After writing all of the above, I can make a summary comment on the entire merge request, something like:

This is a great new feature and it will be really useful once merged. Can you please clean up the function naming, and handle the refactoring mentioned in the individual comments I've made, and then let me know to look at it again? :)

Even if the merge request is a complete dog's breakfast, the individual comments can each be simple. There will just be more of them. Then the summary comment might say:

I'm sorry, but this code isn't really up to snuff. There are a great many edge cases (as detailed in individual comments) that will be handled incorrectly and give a bad user experience, or even data corruption in one case. (See comment on commit 438a95fb734.) Even some normal use cases will result in extremely bad application performance (specifics noted in individual comments on the diff for somefile.c).

 

To be production ready this feature will need a full rewrite with more attention given to what assumptions are safe to make at every point in the flow. (Hint: No assumptions are safe unless they've been checked for.)

 

I'm closing the merge request pending a full rewrite.

Summary: Review the technical aspects of the code as comments on individual lines of code. Then summarize those comments in an overall comment on the merge request. Don't get personal—just deal in the facts, and in your opinion about the code, not about the coder. And base your opinion on facts, accurate observation, and understanding.

I'm surprised it hasn't been mentioned in any answer so far, and perhaps my experience with code reviews is the unusual one, but:

Why are you reviewing the entire merge request in a single message?

My experience with code reviews is via GitLab; I always imagined that other code review tools would work similarly.

When I am giving a code review, I comment on specific, individual lines of the diff. This is extremely unlikely to be received as personal criticism, because it is commentary on the code—and is actually displayed as commentary on the code, shown directly below the code it is a commentary on.

I can also comment on the entire merge request, and often do. But specific points can be pointed out on specific lines of the diff. (Also, when a new commit is added such that the diff changes, the comments on the "outdated diff" are hidden by default.)

With this workflow, code reviews become much more modular and less cohesive. A line from a code review can simply say,

Nice approach, wrapping this in a dedicated function!

Or it might say,

This object name doesn't really match the purpose of the object; maybe we could use a name like 'XYZ' instead?

Or if there are major problems with a section, I might write:

I see that this code works (and I see why it works) but it requires focused study to understand it. Could you please refactor it into separate functions so it will be easier to maintain in the future?

(Example: Function ABC is actually doing three things here: bazzing the foo, barring the boz and fripping the zorf. Those could all be separate functions.)

After writing all of the above, I can make a summary comment on the entire merge request, something like:

This is a great new feature and it will be really useful once merged. Can you please clean up the function naming, and handle the refactoring mentioned in the individual comments I've made, and then let me know to look at it again? :)

Even if the merge request is a complete dog's breakfast, the individual comments can each be simple. There will just be more of them. Then the summary comment might say:

I'm sorry, but this code isn't really up to snuff. There are a great many edge cases (as detailed in individual comments) that will be handled incorrectly and give a bad user experience, or even data corruption in one case. (See comment on commit 438a95fb734.) Even some normal use cases will result in extremely bad application performance (specifics noted in individual comments on the diff for somefile.c).

To be production ready this feature will need a full rewrite with more attention given to what assumptions are safe to make at every point in the flow. (Hint: No assumptions are safe unless they've been checked for.)

I'm closing the merge request pending a full rewrite.

Summary: Review the technical aspects of the code as comments on individual lines of code. Then summarize those comments in an overall comment on the merge request. Don't get personal—just deal in the facts, and in your opinion about the code, not about the coder. And base your opinion on facts, accurate observation, and understanding.

Source Link
Wildcard
Wildcard
  • 544
  • 3
  • 15

I'm surprised it hasn't been mentioned in any answer so far, and perhaps my experience with code reviews is the unusual one, but:

Why are you reviewing the entire merge request in a single message?

My experience with code reviews is via GitLab; I always imagined that other code review tools would work similarly.

When I am giving a code review, I comment on specific, individual lines of the diff. This is extremely unlikely to be received as personal criticism, because it is commentary on the code—and is actually displayed as commentary on the code, shown directly below the code it is a commentary on.

I can also comment on the entire merge request, and often do. But specific points can be pointed out on specific lines of the diff. (Also, when a new commit is added such that the diff changes, the comments on the "outdated diff" are hidden by default.)

With this workflow, code reviews become much more modular and less cohesive. A line from a code review can simply say,

Nice approach, wrapping this in a dedicated function!

Or it might say,

This object name doesn't really match the purpose of the object; maybe we could use a name like 'XYZ' instead?

Or if there are major problems with a section, I might write:

I see that this code works (and I see why it works) but it requires focused study to understand it. Could you please refactor it into separate functions so it will be easier to maintain in the future?

(Example: Function ABC is actually doing three things here: bazzing the foo, barring the boz and fripping the zorf. Those could all be separate functions.)

After writing all of the above, I can make a summary comment on the entire merge request, something like:

This is a great new feature and it will be really useful once merged. Can you please clean up the function naming, and handle the refactoring mentioned in the individual comments I've made, and then let me know to look at it again? :)

Even if the merge request is a complete dog's breakfast, the individual comments can each be simple. There will just be more of them. Then the summary comment might say:

I'm sorry, but this code isn't really up to snuff. There are a great many edge cases (as detailed in individual comments) that will be handled incorrectly and give a bad user experience, or even data corruption in one case. (See comment on commit 438a95fb734.) Even some normal use cases will result in extremely bad application performance (specifics noted in individual comments on the diff for somefile.c).

To be production ready this feature will need a full rewrite with more attention given to what assumptions are safe to make at every point in the flow. (Hint: No assumptions are safe unless they've been checked for.)

I'm closing the merge request pending a full rewrite.

Summary: Review the technical aspects of the code as comments on individual lines of code. Then summarize those comments in an overall comment on the merge request. Don't get personal—just deal in the facts, and in your opinion about the code, not about the coder. And base your opinion on facts, accurate observation, and understanding.