Skip to content

Instantly share code, notes, and snippets.

@voznik

voznik/angular_code_review_checklist.md

Forked from mrmeku/CODING_GUIDELINES.md
Last active August 4, 2022 19:23
Show Gist options
  • Save voznik/6dba2088ab7cb351b7cdcc3598b4ec39 to your computer and use it in GitHub Desktop.
Save voznik/6dba2088ab7cb351b7cdcc3598b4ec39 to your computer and use it in GitHub Desktop.
Download ZIP
angular_code_review_checklist
Raw
angular_code_review_checklist.md

Coding guidelines

This document is split into three sections

  1. On General Code Health. This section contains guidelines intended encourage patterns which produce more maintainable, less bug prone, code.
  2. On Angular Best Practices. This section outlines the practices related to Angular, Typescript, RxJs and @ngrx/store. Following these general coding guidelines will help make the application cleaner.
  3. On Angular Performance. This section contains guidelines intended to make an Angular app trigger change detection less often and on less entities.

General Code Health

Copy only once, extract otherwise

By the time you're copying functionality a third time, you can clearly see a useful, reusable, pattern. Its harder to see such a pattern the first time you need to duplicate functionality.

Restrict visibility as much as possible

A Public API is a contract between the author of a library and those who consume that library. The more symobls a library expose, the larger the burdon of the author to maintain particular functionality and behavoir that consumers downstream depend upon. You don't want other libraries to depend on you unless there is a really good reason why they need to, it only makes your life harder.

Enforce common code style with lint rules, not code review

Code review should be spent catching bugs in business logic, not in quibbling over patterns. A pattern needs to be picked at some point, and everyone should use the pattern to be consistent.

Libs should not import from one another (with exceptions)

When you make a change to a library, the depedency graph can be analyzed to such that other affected libraries can be computed. ng serve does this to build the minimal portion of your app that needs to be recompiled. NXs affected:test/lint commands do this to only test/lint the minimal set of files which need to be checked. Ideally, a change to one library affects no others. However, if another library were to import a symbol from one that has been altered, it would also be marked as affected by a change. Thus, we want to decouple dependencies as much as possible.

Datastructures should be flat and use readonly properties

Often we nests objects inside one another. Sometimes those nested objects are nullable. Often, we are forced into a position where we assume that a nested object will not be null and assert that to be the case in our code. That is problematic since our compiler is no longer asserting that we have correct type safety, instead, we as the author are assuming that responsibility. The compiler is more reliable that humans in 100% of cases. We want to make code structures that let the compiler do its job more effectively and makes our code safe. Flat data structures are one way we can enable the compiler to do that. Readonly properites / constants allow the compiler to enforce that mutation has not happened.

Make your app shell as tiny as possible

One of the most impactful features a webapp can have in regards to percienved performance is a time decrease the time to first paint (first rendering of content) to be as small as possible. Thus our goal is to bundle only the minimal amount of code as is strictly needed when the app first starts. We want to authenticate the user and present them with a small shell of placeholder content that we will fill soon after by lazily loading the actual content. The smaller the shell, the quicker the start up time and the quicker the user's perceived performance.

Clean Code Checklist in Angular

Angular is a very powerful framework with a lot of functionality. But if you’re new to the game, it can seem overwhelming with all its different options and whatnot. Hopefully, by following the guidelines outlined here, some of the concepts became more clear to you. Although there are no blueprint for how to write clean code, there’s some key takeaways here:

Follow Styleguide

  • Do use Angular Styleguide
  • Use Angular CLI
  • Do use codelyzer to follow this guide.

Separation of concerns

Encapsulate and limit logic inside your components, services and directives. Each file should only be responsible for a single functionality.

  • Create small reusable components and/or libraries
  • Make component as dumb as possible
  • Extract business logic from components to services
  • Don't repeat the same code and funcitons (DRY)
  • Don't make logic in templates
  • const vs let

Utilize TypeScript

TypeScript provides advanced autocompletion, navigation and refactoring. Having such a tool at your disposal shouldn’t be taken for granted.

  • Consider some guidelines from TypeScript Coding guidelines, e.g. Do not use "I" as a prefix for interface names.
  • Codelyzer core rules
  • Never use type any
  • Use namespace to eliminate the needs to import many interfaces files
  • Use Decorators

Use Prettier with TSLint (and be happy)

TSLint checks if TypeScript code complies with the coding rules in place. Combined with Prettier and Husky makes for an excellent workflow.

  • Create Custom Lint Checks after extending popular configs, like tslint-config-prettier, rxjs-tslint-rules, tslint-eslint-rules etc.
  • Remove formatting rules from tslint.json and let Prettier handle formatting
  • Use pre-commit hooks to ensure clean code

RxJS in Angular

RxJS comes packaged with Angular, it’s to our great advantage to make the most of it.

  • Avoid Promises
  • Avoid memory leaks - don't forget to destroy subscriptions (with take, takeUntil, takeWhile, first or custom decorator)
  • Use of Finnish notation (var$) for functions, methods, parameters, properties and variables
  • Use the Async Pipe
  • Use right ~Map rxjs operator (Higher Order Observables)
  • No subscriptions inside of subscriptions (use another rxjs operators)

Clean up imports

We can clean up our imports considerably by using path aliases to reference our files.

  • Add aliases to your path in main tsconfig.json
  • Always sort imports ( use tslint )
  • Use barrel index files to simplify imports

Bundle your Code into Modules

Modules help to organize your code into smaller bundles to make finding things easier. But they are not only a cosmetic thing. With the help of lazy-loaded modules, you can also increase the user-experience by only downloading the parts of the application, that are required at that moment.

  • Create Core and Shared Modules
  • Create Material or Bootstrap or any chosen UI Module and re-export used modules through it, import once in Shared
  • Keep AppModule clear, re-export dependent modules in Core & Shared
  • Prevent re-import of main Modules
  • Create Feature Module(s)
  • If this is new route component, it must be lazy loaded
  • Never directly import lazy loaded folders

Create Wrappers for Angular HttpClient, ErrorHandler & logs (Adapter pattern)

When handling HTTP calls in Angular consider using an adapter pattern. This means that instead of working with Angular’s HttpClient overall in your code base you wrap the Angular HttpClient in your own HttpClient adapter, e.g. HttpWrapperService. This allow processing all the API requests inside request() method before making the actual API call. Also, this gives us the ability to handle all API server errors at one place

  • Create wrapper for HttpClient
  • Implement your custom ErrorHandler
  • Implement HttpInterceptor
  • Use Caching with RxJs Observables
  • Create wrapper for your console.xxx calls, e.g. LogService

Components

Only a Smart Component should be able to communicate with external APIs by calling their functions and subscribing to their return values/promises/observables

  • Keep Dumb components as dumb as possible - should not be dependant on external services, should not produce any side effects,should not mutate its’ inputs
  • If multiple children are equally Smart, make them Dumb
  • If the Smart one gets too big, divide it into separate Smarts
  • Divide components into Smart and Dumb
  • Use Content Projection to make a component hierarchy flatter and avoid excessive chaining of @Input/@Output to pass state down the component tree and events up
  • Use Reactive Forms
  • Use Renderer2 - avoid manipulate the DOM directly with ElementRef
  • Use structural directives for dynamic templates
  • Use ngIfElse with template if you have boolean condition instead od 2 ngIfs
  • Use ngSwitchCase if you have display some elements based on some expression

State management (NGRX) (if necessary)

State management is a great tool to have at your disposal, but should only be used for large, complex applications where multiple components shares the same state.

General

  • Create a Root/Main StoreModule that bundle together NgRx store logic. Feature store modules will be imported into. Import it AppModule once
  • Feature Module State Composition
  • Only Containers (aka Smart Components) should use state
  • Initialize all selector properties directly when defining them instead of constructor or other places
  • Use selectors! (can provide performance benefits via memoization)
  • Make the state immutable (store-freeze)
  • Implement Generic error action
  • Normalize data if a store feature slice consist of a standard array of type (via @ngrx/entity or manual) (correspondents to Datastructures should be flat)

Actions

  • Use enum for all actions types
  • Use classes for all actions
  • Always name the property of the action payload
  • Keep all related actions in the same file
  • Group actions in a union type

Effects

  • Not all effects have to emit something ({ dispatch: false })
  • Load data with effects
  • Always handle errors

Reducers

They should be clean and don’t have any logic inside. A reducer should only take the data from the action and update the corresponding part in the state with it, it should not make any decisions or any kind of logic that will be hidden there.

  • Keep them simple

Angular Performance

AoT Ahead-of-Time Compilation

AoT can be helpful not only for achieving more efficient bundling by performing tree-shaking, but also for improving the runtime performance of our applications. The alternative of AoT is Just-in-Time compilation (JiT) which is performed runtime, therefore we can reduce the amount of computations required for rendering of our application by performing the compilation as part of our build process.

OnPush change detection

When you opt to use OnPush change detection, you make a contract with Angular: So long as the inputs to this component have not changed and it recieves no outputs from a child, nothing else will have changed. With that contract established, Angular can now skip running change detection on a compnent and, consequently, make the app feel more responsive.

Pure pipes

When you opt to mark a pipe as pure you make the following contract with Angular: This pipe has no sense of state and has no side effects. Given the same input, the output will always remain the same. With that contract established, Angular can now skip re-running a pipe when the input to that pipe has not changed.

ngForTrackBy for every ngFor loop

When you opt to use an ngForTrackBy function, you make the following contract with Angular: If the output of this function does not change for a corresponding DOM node, that DOM node should be reused. With that contract established, Angular can now skip destoying and recreating DOM nodes as you add / remove or mutate other elements of a collection you are iterating over in a template.

| async over .subscribe

A major cause of memory leaks within Angular applications are observable subscriptions that are not unsubscribed from upon a the destruction of the components which subscribed in the first place. Angular's async pipe automates the process of cleaning up these subscriptions.

ngIfAs over multiple | async subscriptions

The more we can limit the number of observable subscriptions, the better performance will be. Here, we can leverage ngIf to cache a subscription value to be made use of in multiple places within a template.

Multicasting to share a subscription

Every time an observable goes through a piped operator, there is an assoicated cost. Data is transformed from one form to another, HTTP calls are made to fetch data, things happen upon subscription that do not come for free. These costly operations should be formed as little times as we can help it. Thus, it is often useful to cache the result of an observable such that if there is more than one subjscriber, the costly operations still only happen once. This strategy of caching a prior result for other subscribers to use is called multicasting. It can be accomplished by using the publishLatest operator

Inspiration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment