Link Search Menu Expand Document

Code review goals

  • Teach best practices.
    • Students should not only learn but also demonstrate their understanding of best software practices by delivering code that meets high standards.
  • Produce quality product for clients and next batch of students.
    • Spark wants continued collaboration with clients.
    • Students should be able to focus on their contribution rather than fixing previous code.

What to do when reviewing a pull requests

  • Ensure pull request is complete. A complete pull request;

    • Must have a clear description of feature or bug.
    • Should include a gif/video of before and after feature was added, if it introduces visual features. Eg: Screenshot of deletion feature

    • Contains code that all contributes to a single feature.
    • Must not be commits behind main branch (or the branch it’s about to merge into).
    • Passes all CI/CD checks.
  • Provide actionable comments. State directly what should be changed and why Egs:

    • Instead of “I am not clear on what’s going here”, you could comment “group lines 23-65 into a function “calculateCoordinatesOfScalingBox()” and move it to a utils.ts file to make your code more readable”.

    • Instead of “Your branch doesn’t seem to be up to date”, you could comment “Merge with main to get latest changes”.

  • Run branch locally to verify functionality.
  • Approve asap.

Code review checklist

  • Project structure

  • Code organization

    • On frontend, adhere to Atomic Design as much as possible.
    • Repeated code should be made into functions.
    • Files shouldn’t be too long.
      • Most files should be less than 100 lines. Ideally, no file should exceed 400 lines.
  • File formatting
    • Code should be formatted using a formatting tool.
      • Stick to prettier.
    • Files should have a consistent naming convention.
  • Git management

    • Has gitignore file.
    • Node_modules, env files, build folders/files (dist etc), log files etc should not be pushed to remote repo.
  • ReadMe
    • Changes should be reflected in
      • A code change that contradicts the readme, makes it obsolete or insufficient, should be accompanied by changes in readme.
      • “Best time to add documentation is before implementation. Worst time is after implementation. At the very least, document during implementation.”
  • Logging
    • Errors should be logged with console.error .
    • Remove all console.logs. They are not necessary in a well organized, commented, documented codebase.
  • Common code smells

    • Unused imports, packages and functions should be removed.
    • Long, repeated If else blocks should be replaced with a switch statement or a map.
    • Magic numbers should be replaced with named constants placed at the top of a file or in a constants.ts/config.ts file, accompanied by necessary comments.
    • Code blocks prone to unpredictable errors should be placed in a try catch block or handled gracefully.
      • Eg are file i/o, network requests, etc
    • if else should be simplified with ternary statements where possible.
    if(age >= 18 ){
       const message =  "You are an adult";
       const message =   "You are not an adult yet";
    // can be replaced with
     const message = (age >= 18) ? "You are an adult" : "You are not an adult yet";
    • See here for more examples of code smells.
  • Common React pitfalls

    • Elements in a list should have keys.
    export const DatasetsView: React.FC<DatasetsViewProps> = ({ datasets, additionalColumns }) => {
      return (
          { Dataset) => (				 // add a key to each elements in list
          <Accordion key={dataset._id} disableGutters>
              <AccordionSummary expandIcon={<ExpandMore />}>
    • Custom hooks
      • Code that resides in a hook (eg useState, useEffect, useCallback etc) and is repeated in multiple files should be refactored into a custom hook.
  • Nits:

    • Comments should add context rather than repeat code.
    • There shouldn’t be confusing spelling mistakes in variable/function names.
    • Variable and function names must be concise yet descriptive enough.
      function getId() {...} // ok
      function getUserId() {...} //better
      const duration = videoRef.current.duration; //ok
      const videoDuration = videoRef.current.duration; //better