Code reviews with junior engineers

Leadership

I enjoy reviewing code, especially if it's from someone more junior in the team. Given that a lot of the comments made in these reviews are generally applicable, I thought it might be useful to have a running list:

Note - Our team works asynchronously in very different time zones, so the comments are more detailed. In distributed teams, I've found that targeted over-sharing can be productive.

Namespacing

Scenario

There is a TypeScript string enum in the codebase that houses various attribute selectors used for Cypress tests:

export enum DataCySelectors {
  modal_title = 'modal-title'
}

and the author creates a new property, submit_button_modal.

Comment

Change submit_button_modal to modal_button_submit. Some reasons why we have this convention (largest context to smallest, left to right) are:

DRY integration tests

Scenario

The author creates a generically named utility function that is called in more than one test in an effort to make tests more DRY.

Comment

Consider how to improve the ergonomics of these test utility functions. For example, the name of this function is selectProduction but looking at the function declaration, it can/should only be used in the dashboard. A couple options for how to do this:

export const CyDashboardUtil = {
  selectProduction,
}

and use it l like:

import { CyDashboardUtil } from '../utils';

it('should do something', () => {
  CyDashboardUtil.selectProduction();
  ...
  // The rest of your test
});

For clarity, I think a healthy convention for us is to think of custom Cypress commands as fragments of functionality that can be used in all tests, whereas these utils are fragments of functionality that can be used in a subsection of tests.

Before creating either a custom command or a util, it's a good idea to carefully consider the tradeoffs (which I think you've done a good job of in these later commits). For example, if (and when) this test breaks:

If after all these considerations you think it's still worth it, then go ahead. Keep in mind though that the best tests are easy to read with no prior context and quick to fix, so extracting stuff out can easily throw a wrench in those goals.

Rebasing

Scenario

After a series of reviews that require changes, the author has added multiple commits and merged another branch into his.

Comment

Can you rebase your branch on canary please, it's hard to see which changes are yours.

You may know how to do this already, but just in case here are the steps:

  1. git checkout master, then git pull to get the latest master changes
  2. git checkout [your-branch-name], then git rebase -i master and follow the prompts
  3. When done, git branch to make sure you're still on your branch, then git push -f to overwrite your old remote branch history with your newly squashed branch history

When rebasing you should:

If you screw up in the middle of it (we all do this), you can close the terminal, open a new one and git rebase --abort to undo everything. If you're especially worried you can always make a local duplicate of your branch before rebasing as backup (your history will still be on your remote branch until you force push but sometimes its helpful to have insurance).

You can also do this whole process with whatever git client you use but I like the built in interactive cli rebase.

Destructuring variables

Scenario

A variable is declared:

const comment = ['Some comment text'];

and later referenced with comment[0].

Comment

Consider how we can make these local variables more convenient to use. If there will be only one value you can initialize the variable with a string value, but if you expect there to be more than one value you can communicate more clearly by using specifically named variables (instead of having to reference it like comment[0]). Some options:

const comment = {
  someUsefulKeyName1: 'First comment',
  someUsefulKeyName2: 'Second comment',
};

and reference like comment.someUsefulKeyName1, or the fancy destructured way:

const { someUsefulKeyName1, someUsefulKeyName2 } = {
  someUsefulKeyName1: 'First comment',
  someUsefulKeyName2: 'Second comment',
};

and use the properties directly (someUsefulKeyName1).

const [someUsefulKeyName1, someUsefulKeyName2] = ['First comment', 'Second comment'];

Keep in mind though that unlike object destructuring, array destructuring depends on the order of the array items. This makes it generally less used.

Thanks for reading!

If you'd like to get in touch, reach out @doestyhopp.

Otherwise feel free to peruse some other published notes 📚