Happy New Year!

On my first day back to work at the end of 2021 after being on maternity leave for three months, I was handed a few PRs to review. I started jotting down my list of things to look out for during code reviews as it was a perfect way for me to get back up to speed. Decided to share these in case they would be helpful to others.

In this post, I’ll be sharing my code review lookout points for our React/TypeScript web frontend projects, for which, we also use material-ui as the styling library. Please comment to let me know your thoughts on doing code reviews for React projects!

General

• Whether the code is expressive and communicates its intent. For example:
  • If there’s mysterious naming, I would suggest renaming them into something more descriptive; it can be generic or context-specific based on the usage of the variable or the function.
  • If there are implicit constants or magic numbers, I would suggest lifting them as constants to the top of the file, or extracting them into a separate file if being shared.
  • Is there’s misuse for const and let—use const in most cases unless the variable gets updated later. Note that for array that’s being pushed to later, we should still use const instead of let.
  • Whether the code can be refactored to be more clean, readable, or efficient. For example:
    • Whether function shorthands, object destructing, or lodash helper can be applied.
    • Whether long if statements may be split, or refactored using case & switch statements.
    • Whether code conditionals make sense and if falsey checks cover the intended use cases.
• How the new code would impact the overall codebase. For example:
  • Does the folder structure and file naming follow our convention?
  • Is there unnecessary repetition and duplications? Could we delete some code via reusing existing functionalities or extracting some helpers?
  • Is complicated code logic accompanied by comments? (If hard to grasp even after proper abstraction is applied.)
  • Whether abstractions make sense.
  • Whether typings are defined and aligned with feature requirements and if there are proper component tests & cypress integration tests in place.

React Specific

• If component composition makes sense, specifically:

  • Is business logic and styling mixed?

    I find it helpful to extract stylistic components so business logic is apparent.

    • On the root level, I like to create reusable Atom- and Molecule- components based on our design system. (See here for atomic design philosophy.) Some atoms and molecules can be created from scratch, others can be wrapper components of material-ui components, for which I have an example in the section below.
    • On a feature level, we can have smaller components that accept styling props, and have an overall component handle business logic and pass down styling if styling is impacted by business logic. It’s not one rule applies to all, when components are smaller and simpler, I think business logic, data fetching, and styling can co-locate. But for more complicated components, I find locating business logic in one overall file improves readability.

  • Is data fetching and component mixed?

    Our codebase follows a containerized structure–data fetch happens in an index.tsx file, and then that fetched data gets passed in as a prop into the component file. We don’t follow this pattern strictly, if the data fetch is simple, then the container file can be index.ts with default export. Though the container pattern comes in handy when handling loading state. For example:
    

    // index.tsx
    function Container({
     // some props
    }) {
        const data = [someDataFetcher];
    
        useEffect(
            // use the data fetcher to fetch data based on fetch conditions
        )
    
        if (!data || isLoading(status)) { 
        // isLoading & status comes from our data fetcher
           return <div>Loading</div>; // or other loading UI such as a spinner
        }
    
        return (
          <Component data={data}/>
        }
    }
    

    • Along the lines of loading state, code splitting with suspense + lazy loading is nice to group things that need loading state and error boundaries together.

  • Whether props make sense

    • Naming: props should be named according to the use case that’s understandable by this component as opposed to assumed context. In a piece of code I reviewed recently, a table row is clickable if the data is not managed by synced client data, then instead of naming the prop syncManaged, it would make more sense to name it clickable or editable as the table row component itself only needs to know how to behave, the overall component can handle the why and that the row component is more adaptable if the conditions for clickable change in the future.

• Whether the component is composed in a way that considers changeability and maintainability.

  • For components with strict contracts, I would define every single prop. Otherwise I would opt in for a {children} prop to make the component more extensible.
  • Prop drilling v.s. global state v.s state co-location
    • Whether the state is located closest to where it’s relevant–in the component itself, or in the closest parent that makes sense to have the shared state.
    • Whether React context can be used to avoid prop drilling, though should still put the context close to where it’s most relevant.
    • Global context would makes sense for things that apply to the whole app to avoid having to pass props down every single level, for example themed styles.
  • Whether a set of hooks can be abstracted into a custom hook that describes the purpose of the code better and can be used and changed easier.

• Whether useEffects are done right, as it’s one of the most reached hook:

  • Is it kept simple—if one big effect hook can be broken down into multiple ones so when to re-render is more manageable.

  • Are the dependencies all necessary. The linter can make sure all dependencies needed are included, but it won’t tell us what’s extra. Additionally:

    • Functions and variables only needed for the useEffect should live inside instead of outside the effect block to reduce the need of being included in the dep array.
    • Should include the specific property on an object instead of the whole object in the dep array–e.g. use item.name instead of item will prevent object equality comparison return false or the change of other properties of item unrelated to this effect causing unnecessary re-renders.
    • Things like useCallback and setState don’t need to be in the dep array.

    • ref shouldn't be in the dep array. Here’s how I would use the useRef hook
      

      const someRef = useRef(null);
      
      useEffect(() => {
        const someRefElement = someRef.current;
      
        if (someRefElement !== null) {
          // some logic here
        }
      
        // some other logic for when ref is null when first rendered
      
        return () => {
          // some clean up function
        };
      }, [
      // neither ref nor ref.current should to be in here
      ]);
      

      • Another way is to not use effect and just use a callback ref. This post does a great job explaining it.

• Whether things are “cached” properly

  • Whether fetched data has been “cached” properly and only re-fetch when needed in useEffect.
  • Whether useMemo has been applied to cache expensive calculations.
  • Whether memo has been used to cache components that don’t need to be re-rendered when the parent component changes and if useCallback has been used in the parent component for the functions being passed to the memoized child components.

• Other bits to look out for:

  • Whether keys in a mapped component array are unique and stable–should avoid using object or pure indices as keys.
  • Use React.Fragment instead of html element tag if a tag is not needed. Note that though the Fragment can be shorthanded into <></>, when needing to apply key, should do <React.Fragment key='some key'>.

Material UI & Styling Specific

We are on v4 of material UI and use the createTheme API for overall theming and the useStyles/makeStyles API for component styling. We allow clients to customize the application with their brand colors. Therefore, When reviewing the styling side of things, I mainly pay attention to theming and design system implications. Specifically:

• Whether or not a component styling has taken theming into consideration, for example:
  • If themed colors are used instead of arbitrary colors.
  • If margins and paddings are specified with theme.spacing() instead of random numbers.
  • If SVG icons have been converted using the mui SvgIcon API and stripped off the fill property—I normally use the SVGR playground to convert the SVG file first, then replace the outside tag of the converted result with SvgIcon tag and take out any fill properties since those will be passed in via the component and be populated via theming.
  • If any !important overrides supposed to be applied to the whole theme or if there’s a color, typography, spacing, or shadow that’s not within our design system–should make sure to have checked with our product designer so that the design system updates in Figma and the update to the theming files in our codebase are aligned. In our overall theme file, we have imports of our custom palette and typography files, in addition to spacing, shadow, and component specific overrides. So where to make the update would be based on the impact of this override.
    • If it’s a global theming property on mui default theme, we can use global theming override instead of adding new properties, otherwise add the property to the global theme.
    • If it’s global to a specific mui component, we use component style override, e.g. here’s the style props to look for when overriding a DialogContent component.
    • If it’s a set of components from a feature that has specific theming requirements, we can extract a shared styles file.
• Whether the place for applying styles makes sense–I would go with this order: included material ui props → classes in the useStyles block → and then if the style is impacted by component props, I would first consider passing it down to the useStyles block as custom style props, while sometimes it makes more sense to apply the variables directly inline, for example, when using the width css prop directly.

• When the stylistic part of the component is being shared amongst multiple components, it can be extracted into an atom or molecule. When using material ui for atom and molecule components, I like to create wrapper components–see below for example–I think these wrapper components provides flexibility as the component API is defined by us and not confined by the mui props, so we can switch out the mui component used without impacting the components using this atom or molecule.
  

  // atoms/PaperCard.tsx
  import React, { forwardRef } from 'react';
  import makeStyles from '@material-ui/styles/makeStyles';
  import classNames from 'classnames';
  
  interface Props extends React.ComponentProps<'div'> {
    children: React.ReactNode;
  }
  
  const PaperCard = forwardRef((props: Props, ref: React.Ref<any>) => {
    const classes = useStyles();
    const { className, children, ...restOfProps } = props;
  
    return (
      <div ref={ref} className={classNames(classes.root, className)} {...restOfProps}>
        {children}
      </div>
    );
  });
  
  const useStyles = makeStyles(theme => ({
    root: {
      // our custom styles
    },
  }));
  
  export default PaperCard;
  

• Whether responsive design and accessibility (we also use Lighthouse to audit) are considered.

• Finally, see if there are anything that needs documenting and communicating further.

That’s it for Now

These are what I have had a chance to jot down in the bits of time after work and between caring for my adorable baby 😆

By no means is this list meant to be comprehensive, hopefully it serves more as a general guide for things of note. And of course, the most important thing is that the feature should work per product requirements–which requires pulling the feature, testing it, and checking product specs and collaborating with the product team as a whole.

Please comment to let me know if I missed anything. I look forward to continuing to get back up to speed and sharing my follow up thoughts in future posts.