Code Review Exercise

Goals

Review code review practices
Practice code review on React components using GitHub pull requests

Background

There is no one right or wrong development workflow and approach to code review. In class we outlined an approach using Git feature branches and GitHub pull requests (PR). It is similar to this process (albeit without the interactive rebase at the end, we will merge via the GitHub interface.) Today we will practice the code review-portion of that process in class. Review the GitHub documentation for performing code review via a GitHub PR.

A code review is not intended to be an adversarial process. You and the reviewer - your teammate - share the same goal: making your application the best it can be. Review the following example guide for reviewing and having your code reviewed for considerations to make that process more effective and productive. I want to highlight some of their suggestions:

Accept that many programming decisions are opinions, i.e., there is usually a tradeoff
Ask good questions; don’t make demands
Good questions avoid judgment and avoid assumptions about the author’s perspective
Ask for clarification
Avoid selective ownership of code, e.g., “mine”, “not mine”, “yours”
Avoid using terms that could be seen as referring to personal traits, e.g., “dumb”
Avoid diminishing words, e.g., “simple”, “just”
Be explicit
Be humble
Don’t use hyperbole, e.g., there are few things that are an “always” in this work
Don’t use sarcasm (it doesn’t work well online)
Keep it real, i.e., be true to yourself and how you communicate. For example, you don’t have to use emojis if that is not your style, or hold back if it is.
Talk synchronously if there are too many “I didn’t understand” similar comments
If you learned something new, share your appreciation (not all comments need to be about changes)
Avoid the “since you’re at it” attitude, i.e, stay focused on this feature

For the reviewer, recall that your role is to provide feedback and a second set of eyes, not serve as a “gatekeeper”. Start by understanding what this PR is trying to accomplish. Then look for opportunities to simplify the code (while still solving the problem), opportunities to setup the team for future success (e.g., that function/component may be useful elsewhere), error cases that maybe weren’t considered, and more. When suggesting improvements or possible error cases, start from the assumption that the author already considered those ideas and so you are trying to learn what they think about those topics, e.g., “What do you think about null values here?...”.

Our code reviews are generally not about style, and specifically not the aspects of style that are the purview of Prettier (i.e., this is not the place to litigate indentation approaches). All code should have been run through our styling and linting tools.

Practicing code review

First, one member of your group should click through to accept the assignment from GitHub Classroom. As part of that process they will create a group, e.g., “Group 1”, that others can join. Once one person has created the group and the shared repository, other group member should click through to GitHub classroom to accept the assignment but join the existing group. You should all be able to see the same shared repository.

In our typical workflow, we create a PR to merge a feature branch into main. Here we will do the opposite to create a PR from existing code in a way that works with GitHub classroom (this isn’t a process we need to emulate in the future).

One group member should do the following:

Clone the repository create by GitHub classroom as usual. It should have two commits.
Create a new branch named initial from the first commit by executing the following in a terminal:
```
💻 git checkout -b initial f89ea44
```
Push your newly created branch to GitHub, i.e., 💻 git push origin initial.
In the response from GitHub you should see a link to create a PR. Follow that link. If not, you can use the “Pull Request” tab on the site for you GitHub repository.
Create a PR to merge main into initial like shown below. This PR should contain the single commit with the ProductCard.

Your focus is the ProductCard component (and associated tests) implemented in src/components/ProductCard.js and src/components/ProductCard.test.js. ProductCard is intended for an e-commerce application. It shows relevant information, including discounts, to the shopper and enables them to add that item to their cart (if they are logged in and if it is available in the desired quantity). To keep the scope manageable for class, this feature is not completely implemented (i.e. there is a placeholder for a fetch, no styling, etc.) Focus your attention on the aspects that are implemented.

As a group (see note below), perform a code review via the GitHub PR, following the guidelines above. Provide your feedback as comments on specific lines in those two files (see the image below for the mechanics). There is nothing to be submitted and you don’t need or want to merge the PR; this is an in-class exercise in which we will discuss the feedback we provided as a class. There is not necessarily a “right” answer. Recall that our goal is to practice performing a thoughtful and productive code review.

As an example, we might include the following feedback in our review for line 57, the img element:

I am seeing a ESLint warning suggesting using next/image for the product image. What do you think? Should we make this switch? Or perhaps disable that rule here or generally?

Does this work like Google docs? And single comment vs. review?

The “group” approach is not how code review is typically performing, instead it is asynchronous individual activity. The GitHub review interface is designed for that workflow. It does not, to my knowledge, support collaborative editing, like Google Docs. It will be an awkward fit for our class exercise. Talk through the comments as a group, but one person will need to be the scribe.

GitHub PRs support both individual comments and a “review” with multiple comments. A review as a whole can have a status, e.g., “Comment”, “Approve”, “Request changes”, that can control the next step for the PR. For example, repository administrators can require PRs be reviewed and approved before GitHub will allow merging. As with many aspects of our process, that you do code review is more important than which specific feature(s) of GitHub you use. I encourage each project team to define a workflow that makes sense for them.

Where did this code come from?

This code was generated by a large language model (LLM), specifically Claude, for this exercise. It is not necessarily an example of best practices (or the best code that AI can generate), nor intended as a purely “cautionary tale”. It is intended as an opportunity to review code you didn’t write that contains multiple meaningful opportunities for feedback.