Skip to content

Instantly share code, notes, and snippets.

@haacked
Last active March 9, 2020 19:28
Show Gist options
  • Star 26 You must be signed in to star a gist
  • Fork 10 You must be signed in to fork a gist
  • Save haacked/7204241 to your computer and use it in GitHub Desktop.
Save haacked/7204241 to your computer and use it in GitHub Desktop.
Code Review Checklist

General

  1. Unit tests: Review unit tests first. Unit tests are a fantastic way to grasp how code is meant to be used by others and to learn what the expected behavior is. Are there any test gaps that should be there?
  2. Method arguments" Make sure arguments to methods make sense and are validated. Mentally test boundary conditions and edge cases.
  3. Null References" (Yah yah, we know. Use F# and this goes away. We get it already.) Null references are a bitch and it’s worth looking out for them specifically.
  4. Conventions Consistency" Make sure naming, formatting, etc. follow our conventions and are consistent. I like a codebase that’s fairly consistent so you know what to expect.
  5. Disposables: Make sure disposable things are disposed. Look for usages of resources that should be disposed but are not.
  6. Security: There is a whole threat and mitigation review process that falls under this bucket. In simple terms, ask yourself how this code could be exploited. The STRIDE Threat Model contains a list of potential threats to consider.

C# Library Code

  1. async keyword Review library methods that use the async keyword to see if they actually need it as it can introduce extra uncessary cost. See this gist for an example.
  2. async void methods These are a red flag and should probably return Task. See this post for more information.
  3. ConfigureAwait(false) Library methods that return Task should also call ConfigureAwait(false). See this article for more details.

WPF + ReactiveUI

  1. Updating UI: Make sure view model properties are updated on the main scheduler (UI thread). This is because we bind the UI to these properties and the UI can only be updated on the UI thread.
  2. ObserveOn: When kicking off truly async operations, make sure we ObserveOn(RxApp.DeferredScheduler) soon after (related to previous).
  3. Over subscriptions: Make sure we're not over subscribing to deferred observables etc that could lead to multiple operations.
  4. ???

JavaScript (Electron App)

  1. ??? (Please help!)
@forki
Copy link

forki commented Oct 30, 2013

Please keep this list updated.

@haacked
Copy link
Author

haacked commented Apr 28, 2016

@forki will do! Got any F# specific tips?

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