Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code review #396

Merged
merged 3 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,16 @@
"Armon",
"AWSCLIV",
"azurerm",
"blaa",
"Bento",
"blaa",
"boto",
"Brolliar",
"burnup",
"cfssl",
"cfssljson",
"cifs",
"CNCF",
"Codecademy",
"Codecov",
"dbservers",
"devel",
Expand Down
88 changes: 88 additions & 0 deletions docs/5-software-development-practices/5.5.7-code-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
---
docs/5-software-development-practices/5.5.7-code-review.md:
category: Software Quality
estReadingMinutes: 10
---

# Code Review

Automated tools are helpful in ensuring code quality, but these tools can only get you so far. When you're developing in a team, one of the biggest assets you have to ensure the quality of the code entering your codebase is each other.

Whenever you're attempting to add code to a codebase, chances are you'll need to have a peer review your code. Oftentimes a code review will be required to merge a pull request into your main branch. Receiving feedback on your code can feel intimidating, especially if a group or team doesn't have any agreed upon guidelines for code reviews.

## General Etiquette

Code review guidelines and procedures will likely look different from organization to organization, but there are some general guidelines everyone should follow when engaging in code reviews, both from the perspective of the reviewer and the developer.

### Everyone

- **Be respectful in your comments.** There are humans on both sides of the screen.
- **Have good intentions.** The reviewer and developer should both be going into the process with the intention of ensuring and maintaining quality code in the codebase.

### Developer

- **Separate yourself from your work.** Remember that criticism of your code is not critisms of you.
- **Be open to feedback.** This might seem obvious, but when you're asking for a code review, be ready to take into account any feedback you get rather than just expecting a rubber stamp from the reviewer.

### Reviewer

- **Comment only on the code, not on the developer.** Again, keep your comments respectful and releveant. Avoid statements about the developer, and keep your statements about the code.
- **Consider adding positive feedback.** It can help build rapport and trust between you and the developer if you point out positive things you see in someone's code rather than just negative things.

## Before the Review

When you're developing code, it's good to keep in mind that someone will need to review it. Try to write your code in such a way that it can be understood by others.

Before making a pull request, consider if your code is ready to be merged into the codebase. Are there testing requirements for this repository, and does your code meet these? Is your code complete, or is it a work in progress? If you want input but your code isn't quite ready, consider making a [draft pull request](https://github.blog/2019-02-14-introducing-draft-pull-requests/).

If you've decided it's the right time to raise a pull request, it's worth taking a look at some things you can do before the review to make the review easier both for yourself and the reviewer. If it's not already integrated into the repository, you could run a linter on your code to catch small, easy to fix things like styling, or look into other automated tools that assist with code review.

When you're raising a pull request, you should leverage the description field not only to document what changes you're making, but also to help the reviewer understand why this pull request is being raised and what it's supposed to do. If this pull request addresses a ticket, you should reference that ticket to give additional context.

## Finding a Reviewer

Making a pull request doesn't necessarily guarantee someone will see it. Depending on the organization you're working in, there may be different ways of notifying potential reviewers, like through your typical communication platform (Slack, Microsoft Teams, etc) or by requesting reviews from specific users through Github.

When seeking out a reviewer, you should try to get a review from someone who is at least generally familiar with the project you're contributing to. That being said, you should still try to write understandable code and add a through description.

## Reviewing Code

When you're reviewing code, your goal is to help find any potential defects in the code and help the developer make their code be the best it can be. This can feel pretty intimidating if you haven't done it before, but there are some things you can do to try to give structure to your review.

- **Consider writing a checklist.** You could write a checklist of things you're looking for when you're going into a code review. This list could include things like whether or not the code is secure, if it accomplishes the task it says it's meant to, etc.

- **Think about if now is the right time to do a review.** Sometimes you might not be in the right headspace to do a code review. Doing a code review while your tired or frustrated could result in you missing bugs/errors in the code, or making less than polite comments.

- **Denote nitpicks.** If you have an opinion about the way a piece of code is written, feel free to share that opinion, but make sure you make it clear when you're presenting an alternative way to do something and when you're commenting on a blocking issue.

## Specific Guidelines

If you're working in a small team without dedicated code review guidelines, consider taking some time to formalize your groups approach. Some good questions to ask to guide what you want your process to look like could include:

- How should a developer seek out a reviewer?

- Are you going to integrate any automated tools to assist with the code review process?

- How should a reviewer denote blocking issues and nitpicks?

- Should reviewers put forth particular code suggestions?

- Who's responsibility is it to make changes to PRs that need small fixes?

If you're working in an organization that already has code review customs put into place, take some time to familiarize yourself with that particular process before reviewing code or asking for a code review.

## Examples and Other Resources

[How to Write the Perfect Pull Request - Github](https://github.blog/2015-01-21-how-to-write-the-perfect-pull-request/)

[Code Review Developer Guide - Google](https://google.github.io/eng-practices/review/)

[5 Code Review Best Practices - Atlassian](https://www.atlassian.com/blog/add-ons/code-review-best-practices)

[How To Review Someone Else’s Code: Tips and Best Practices - Codecademy](https://www.codecademy.com/resources/blog/code-review-best-practices/)

# Deliverables

- Assume you're reviewing some code, and you find a line that you would have written differently. Give some examples of a good way to comment on this, and a bad way to comment on this.

- Say you find a potential bug in the code. Give some examples of a good way to comment on this, and a bad way to comment on this.
13 changes: 7 additions & 6 deletions docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -454,19 +454,17 @@ docs/4-cloud-computing/4.3.6-aks.md:
- Azure Kubernetes Service (AKS)
- Azure Container Registry (ACR)
- Node.js

docs/4-cloud-computing/4.3.7-app-service.md:
category: Cloud Computing
estReadingMinutes: 10
exercises:
-
name: Deploy Simple Web Application to App Service
- name: Deploy Simple Web Application to App Service
description: Create A Web App and Scale and Monitor
estMinutes: 240
technologies:
- Azure
- Azure App Service
- Azure CLI
- Azure
- Azure App Service
- Azure CLI
docs/5-software-development-practices/5.1-overview.md:
category: Agile Development
estReadingMinutes: 90
Expand Down Expand Up @@ -580,6 +578,9 @@ docs/5-software-development-practices/5.5.6-sonarqube.md:
technologies:
- SonarQube
- GitHub Actions
docs/5-software-development-practices/5.5.7-code-review.md:
category: Software Quality
estReadingMinutes: 10
docs/5-software-development-practices/5.6-hello-devops.md:
category: Agile Development
estReadingMinutes: 5
Expand Down
1 change: 1 addition & 0 deletions docs/_sidebar.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
- [5.5.4 - Code Coverage](5-software-development-practices/5.5.4-code-coverage.md)
- [5.5.5 - Test Automation](5-software-development-practices/5.5.5-test-automation.md)
- [5.5.6 - SonarQube](5-software-development-practices/5.5.6-sonarqube.md)
- [5.5.7 - Code Review](5-software-development-practices/5.5.7-code-review.md)
- [5.6 - Hello DevOps](5-software-development-practices/5.6-hello-devops.md)

- **Chapter 6**
Expand Down
Loading