
Code Reviews: A Layered Workflow - pizza_pleb
https://charcoalbin.com/posts/code-review.html
======
Smotko
The article makes some good points. Here are my thoughts:

> 1\. Preliminary Checks

+1 for this, but I would also add that you need to add as much
formatting/linting into this step as possible. You shouldn't be pointing out
formatting issues in your code review.

> 2\. Understanding

This is super important! Pull requests that do not explain their purpose in
the description should not be reviewed.

The PR description is also really important when git bisect leads you to the
PR when debugging a new bug.

> 3\. Usability Test

A lot of debate regarding this, but I believe this is extremely important if
not the most important part of your code review. For details read this
section[0]

> 4\. Code Review

A few good tips in the article, I'd just add that you should be polite and try
not to waste people's time.

[0] [https://blog.codereview.chat/2019/06/27/code-reviews-and-
you...](https://blog.codereview.chat/2019/06/27/code-reviews-and-your-company-
goal.html#1-catch-code-defects-before-the-code-is-shipped-to-qa-or-prod)

~~~
pizza_pleb
Thanks for the feedback!

> you need to add as much formatting/linting into [step 1] as possible

Yes, project formatting is a whole other topic but this assumes the project
already has the team's linting/formatting rules baked into the build. As a
reviewer, we are pointing out failing builds here, whatever they may be. If
there are deeper issues/disagreements with the build tasks, that would be a
separate issue.

Thanks for the link by the way.

------
rinchik
IMHO, #3 - Usability Test should never be part of the code review, the author
of the PR should make sure that changes do what they are supposed to do
_BEFORE_ opening a PR.

Developers should be responsible for their own QA. QA is not what code reviews
are about _.

_ There are always exceptions, of course. Use of the common sense should be
highly encouraged, e.g. if proposed change is complex or reviewer want to
visually confirm this change, feel free to checkout this branch, but it is
_NOT_ a requirement for a good review.

~~~
awinder
I once worked in an environment that I liked a lot where after review, the
reviewer would deploy the code out. I think this enforced some nice ideas like
-- code reviews are point of collectivizing the code in the team (so now this
is the teams code, and I'm deploying it, so I'm incentivized to not phone it
in on the review). It also made sure that the reviewer understood the impacts
of code changes as they verified/promoted through staging environment.

~~~
pizza_pleb
Thanks for the input. I plan on creating a version for authors too.

If it's possible in your organization or release strategy, self-deployment is
good for the reasons you say, and it corroborates Larry Wall's virtue of
hubris [0].

The PR author is like a salesman. You should be making the lives of your
customers easier and incentivizing them to look at your product in order to
move things forward. That being said, you should prioritize team profit and
not your own (i.e. don't be a _crooked_ salesman).

[0]: [http://threevirtues.com/](http://threevirtues.com/)

------
gregmac
I'd add to the "Understand" part: should include samples (screenshots, logs,
output files, etc), where appropriate.

This helps with understanding for the review, and also if it's referenced
again in the future (eg, someone is coming back to fix a bug that's later
found to be introduced by the change). It takes only a minute or two extra to
do this, while saving the reviewer a lot of time (checking out and running)
and/or mental gymnastics (reasoning about what CSS/html changes will actually
look like).

Obvious example of where this is useful is for a UI change (with before+after
and/or annotated screenshots), but I've also found this useful for timing bugs
(eg logs), and data-driven code (eg where output depends heavily on data).

