aizatto.com
  • aizatto.com
  • Table of Contents
  • Portfolio, Projects, Tools, Toys
  • Interview Guide
    • Choosing A Company
    • Job Boards
    • Practice
    • Technical Interview Cheatsheet
    • Interview Process
      • Questions to Ask
      • Coding
      • Soft Skills
      • Rejection
      • Negotiation / Deciding
      • Accepting, Joining
    • FAQ
  • Engineering Code
    • Communication
    • Different Types of Coding
    • Commit Messages
    • Reviewing Code
      • Requesting Changes
    • Writing Code
      • Consistency
      • Writing for a code base of 1,000,000+ Lines
      • Write Code Knowing It Will Be Refactored
      • Naming
        • Versioning
        • Create Searchable Names
      • Commenting
        • Don't commit commented code
      • Make It Easy To Reproduce
      • Scripts
      • 80 character limit
      • Exit Early
      • Be careful of enum in switch statements
      • Be careful about chaining conditions
      • Be careful of chaining ternary operators
      • Write Code Knowing You Will be Blamed
      • Hacks
      • Bad Practices
      • Logs
      • Time
      • Other rules
    • Engineering Code
    • Engineering Data
    • Pipelines
    • Configuration Files
    • Site Reliability Engineering (SRE)
    • Best Engineers
  • Engineering Management
    • Hiring
    • New Reports
    • 1:1s
      • Calibration
      • Expectations
      • Mentorship / Learning / Growing
      • Task Management
      • Teams
    • Interviewing Candidates
    • Messenger Groups
    • Resources
  • Why GitBook?
  • Getting into Tech
    • Terminology
  • Personal Goals
  • Daily Drivers
  • Contacting Me
  • Notes
    • JavaScript
      • Array
      • Async & Await / Promises
      • Booleans
      • Collections
      • Cons/Dislikes
      • fetch
      • Map
      • Modules
      • Object
      • Regex
      • Set
      • Style Guides
      • Versions
    • Node.js
      • Best Practices
      • DraftJS
      • eslint
      • GraphQL
      • Relay
      • Hapi
      • Knex
      • Koa
      • TypeScript
      • Webservers
    • Technical Due Diligence
    • Archive
      • Amazon Echo Dot (3rd Gen) with clock
      • Apple
        • AirPods Pro
        • Apple Notes
        • Apple Watch Series 4
        • iPad Pro 11" 2018
        • MacBook Pro 15" 2017
        • macOS
      • Audible
      • Balance
        • Growth vs Contentment
        • Leading vs Following
        • Mindful vs Mindless
        • New vs Old
      • Bags
      • Bandwidth Requirements
      • B2B/B2C
      • Blockchain
      • Board Games
        • Bang
      • Broadway
      • Cheap, Good, Fast
      • CLI
        • git
        • ufw
        • xargs
      • Cloud Providers
        • GCP
      • Communication
        • Asking Questions / Making Requests
        • Making Edits
        • Synchronous vs Asynchronous
        • Change Management
        • Problem Definition
      • Company
        • All Hands
        • The Problematic CTO
        • Organizational Structure
      • Content Creation
      • COVID 19/Corona Virus
      • Coworking Spaces
      • Daily Routine
      • Dating
      • Displays / Monitors
      • DNS
      • Domain Registrars
      • Driving
      • eCommerce
      • Empire Building
      • Facebook for Developers
      • Fever
      • Fiverr
      • Flights
      • Gaming Tablet
      • GitHub
      • GTD
      • Go Lang
      • Headsets
      • Hiking
        • Chamang Waterfalls
        • Kanching Waterfalls
        • Kota Damansara Community Forest Reserve
        • Sungai Chilling
      • Home Device Calling
      • iCalendar
      • Keyboards
        • Ergodox Ez
      • Malaysia Insurance
      • Mental Health Malaysia
      • Multiroom Wireless Speaker System
      • Musicals
      • Mouse
      • Movies
      • Password Managers
      • Phabricator
      • Physical Health
        • Cardio
      • Podcasts
      • Programming Bootcamps
      • Property
      • Productivity
        • Note Taking
      • Redang
      • Relationships
      • Referral Codes
      • Remote Calls
      • Remote Work
        • Comparison
      • Road Trips
      • Ruby / Ruby on Rails
      • Scraping
      • Slack
      • Stripe
      • Singapore
      • UX
      • Venture Builder
      • Video Games
      • Virtual Personal Assistant
      • VPN
      • WebDAV / CalDAV
      • WebSocket
      • Withings
      • Xiaomi Roborock Mijia
      • Old Hardware
        • Netgear R7000P
      • Web Development
        • React
        • SSO Providers
      • Software Engineering
        • Software Architectures
          • Monolithic
          • Non-Monolithic
            • Microservice
            • FaaS (Functions as a Service) or Serverless
        • Repository Management
  • More on Notion
Powered by GitBook
On this page
  • Overview
  • Death by a thousand cuts/commits/diffs
  • Don't pollute commit history
  • Test Plan
  • Don't create technical debt
  • Things to communicate as a reviewer
  • Resources

Was this helpful?

  1. Engineering Code

Reviewing Code

PreviousCommit MessagesNextRequesting Changes

Last updated 4 years ago

Was this helpful?

Reviews should always provide constructive feedback.

When reviewing, always suggest a better possible state.

Review for clarity.

Review knowing that it will be read later.

More time will be spent reading and understanding code, than writing it. Reading to writing ratio is really high.

Review so that it is easier to understand.

Review to reduce "WTFs/minute".

Review with the aim to reduce surprises.

Review code that is easy to test and reproduce.

Don't Review complex code. Review simple code.

Becareful of death by a thousand cuts/commits/diffs. It is very easy to lose track of the architecture.

Don't create technical debt.

See .

Overview

The hardest part about writing code is reviewing it.

Reviewing code is hard because:

  • it takes more effort than writing it.

  • you have to understand the context of why this commit is required. What files or frameworks it is using.

To write about:

  • Consider the architecture.

  • Are they using the descriptive names (variables, functions, classes, etc...)?

  • Check the directory structure. It's really easy to miss this.

  • Dont blame break history.

Death by a thousand cuts/commits/diffs

Becareful of death by a thousand cuts/commits/diffs. It is very easy to lose track of the architecture of a code base.

Don't pollute commit history

Commit history is really important in learning and debugging code. It is important that the commit history is kept as clean as possible.

See "Commit Messages".

Do not just change code to change the style of the code. Unless of course the style is really bad. If the style of the code is consistent with the codebase. Don't change the style. Don't go out of style. Changing the code makes you lose blame.

Consider blame history

Always add trailing commas

If your programming language supports trailing commas. Always include them.

This means that if you ever have to add an additional element/argument to the list, you dont have to change that line.

Consider file history

Take for example, you are just adding new lines to another file which you don't use. New lines look harmless, but they can pollute the file history. If someone were to look at the file history there would be an entry for the erroneous lines you added.

Test Plan

How easy is it to reproduce the commit/diff? The easier it is to reproduce, the better it is.

Check for:

  • screenshots (if applicable)

  • URLs to anything relevant

  • what configurations need to be changed

  • shell commands ran

Don't create technical debt

Things to communicate as a reviewer

  • If things are a nonblocker

  • If things can be fixed in another PR

    • This allows them to maintain momentum

Resources

WTFs/minute is the only valid measurement of code quality.
"Writing Code"
https://mtlynch.io/human-code-reviews-1/
https://www.netlify.com/blog/2020/03/05/feedback-ladders-how-we-encode-code-reviews-at-netlify/
https://google.github.io/eng-practices/review/reviewer/