Skip to content

Instantly share code, notes, and snippets.

@mtodd

mtodd/code_review_checklists.md

Forked from dideler/code_review_checklists.md
Last active December 19, 2015 06:49
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save mtodd/5914355 to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save mtodd/5914355 to your computer and use it in GitHub Desktop.
Download ZIP

Revisions

  1. mtodd revised this gist Jul 2, 2013. 1 changed file with 3 additions and 3 deletions.
    6 changes: 3 additions & 3 deletions code_review_checklists.md
    View file
    Original file line number Diff line number Diff line change
    @@ -22,14 +22,14 @@ User interface

    Visual design (native desktop app)
    ----------------------------------
    - [ ] Are controls laid out in a way which makes sense given visual scan order?
    - [ ] Are controls laid out in a way which makes sense given visual scan order?
    - [ ] Check for alignment
    - [ ] Check for layout, spacing, and padding
    - [ ] Check for visual consistency

    Correctness (C/C++)
    -------------------
    - [ ] Correctness of algorithms
    - [ ] Correctness of algorithms
    - [ ] Loop iteration and off-by-one
    - [ ] Memory access errors
    - [ ] Memory leaks
    @@ -40,7 +40,7 @@ Correctness (C/C++)

    Security (C/C++)
    ----------------
    - [ ] Array bounds on buffers
    - [ ] Array bounds on buffers
    - [ ] Are files created and read/written? Check permissions, races
    - [ ] All user input sanitized or validated?
    - [ ] Proper, bounded use of `str*`, `printf`, `f*` functions?
  2. @dideler dideler revised this gist Jul 2, 2013. 1 changed file with 4 additions and 8 deletions.
    12 changes: 4 additions & 8 deletions code_review_checklists.md
    View file
    Original file line number Diff line number Diff line change
    @@ -10,7 +10,6 @@ Based on the article: [Using checklists for code review](http://blog.rbcommons.c
    User interface
    --------------

    - [ ] Are screenshots provided for all UI changes?
    - [ ] Does this change introduce any new concepts or models? Evaluate.
    - [ ] How much expertise and context will a prospective user need?
    @@ -22,16 +21,14 @@ User interface
    - [ ] Can the UI layout be localized into different language editions without any changes to the source code?

    Visual design (native desktop app)
    -------------

    ----------------------------------
    - [ ] Are controls laid out in a way which makes sense given visual scan order?
    - [ ] Check for alignment
    - [ ] Check for layout, spacing, and padding
    - [ ] Check for visual consistency

    Correctness (C/C++)
    -----------

    -------------------
    - [ ] Correctness of algorithms
    - [ ] Loop iteration and off-by-one
    - [ ] Memory access errors
    @@ -42,11 +39,10 @@ Correctness (C/C++)
    - [ ] Exception safety

    Security (C/C++)
    --------

    ----------------
    - [ ] Array bounds on buffers
    - [ ] Are files created and read/written? Check permissions, races
    - [ ] All user input sanitized or validated?
    - [ ] Proper, bounded use of `str*`, `printf`, `f*` functions?
    - [ ] Validate remote host cert validity & identity
    - [ ] Crypto use
    - [ ] Crypto use
  3. @dideler dideler revised this gist Jul 2, 2013. 1 changed file with 6 additions and 2 deletions.
    8 changes: 6 additions & 2 deletions code_review_checklists.md
    View file
    Original file line number Diff line number Diff line change
    @@ -10,6 +10,7 @@ Based on the article: [Using checklists for code review](http://blog.rbcommons.c
    User interface
    --------------

    - [ ] Are screenshots provided for all UI changes?
    - [ ] Does this change introduce any new concepts or models? Evaluate.
    - [ ] How much expertise and context will a prospective user need?
    @@ -22,24 +23,27 @@ User interface

    Visual design (native desktop app)
    -------------

    - [ ] Are controls laid out in a way which makes sense given visual scan order?
    - [ ] Check for alignment
    - [ ] Check for layout, spacing, and padding
    - [ ] Check for visual consistency

    Correctness (C/C++)
    -----------

    - [ ] Correctness of algorithms
    - [ ] Loop iteration and off-by-one
    - [ ] Memory access errors
    - [ ] Memory leaks
    (e.g. Do raw pointer allocations have equivalent deallocations that all code paths go through? Use valgrind.)
    - [ ] Memory leaks
    e.g. Do raw pointer allocations have equivalent deallocations that all code paths go through? Use valgrind.
    - [ ] Incorrect behaviour
    - [ ] Switch/break fallthrough
    - [ ] Exception safety

    Security (C/C++)
    --------

    - [ ] Array bounds on buffers
    - [ ] Are files created and read/written? Check permissions, races
    - [ ] All user input sanitized or validated?
  4. @dideler dideler revised this gist Jul 2, 2013. 1 changed file with 27 additions and 27 deletions.
    54 changes: 27 additions & 27 deletions code_review_checklists.md
    View file
    Original file line number Diff line number Diff line change
    @@ -10,39 +10,39 @@ Based on the article: [Using checklists for code review](http://blog.rbcommons.c
    User interface
    --------------
    [ ] Are screenshots provided for all UI changes?
    [ ] Does this change introduce any new concepts or models? Evaluate.
    [ ] How much expertise and context will a prospective user need?
    [ ] Do we do anything similar elsewhere? Can we build on existing knowledge or habits?
    [ ] Do we do anything similar but just different enough to cause problems with old habits?
    [ ] Are there any new multi-step workflows? How long and complex are they?
    [ ] If users make mistakes or errors, what are the failure conditions? Can mistakes be undone?
    [ ] How is the copy? Are sentences well-formed and clear? Any spelling errors or typos?
    [ ] Can the UI layout be localized into different language editions without any changes to the source code?
    - [ ] Are screenshots provided for all UI changes?
    - [ ] Does this change introduce any new concepts or models? Evaluate.
    - [ ] How much expertise and context will a prospective user need?
    - [ ] Do we do anything similar elsewhere? Can we build on existing knowledge or habits?
    - [ ] Do we do anything similar but just different enough to cause problems with old habits?
    - [ ] Are there any new multi-step workflows? How long and complex are they?
    - [ ] If users make mistakes or errors, what are the failure conditions? Can mistakes be undone?
    - [ ] How is the copy? Are sentences well-formed and clear? Any spelling errors or typos?
    - [ ] Can the UI layout be localized into different language editions without any changes to the source code?

    Visual design (native desktop app)
    -------------
    [ ] Are controls laid out in a way which makes sense given visual scan order?
    [ ] Check for alignment
    [ ] Check for layout, spacing, and padding
    [ ] Check for visual consistency
    - [ ] Are controls laid out in a way which makes sense given visual scan order?
    - [ ] Check for alignment
    - [ ] Check for layout, spacing, and padding
    - [ ] Check for visual consistency

    Correctness (C/C++)
    -----------
    [ ] Correctness of algorithms
    [ ] Loop iteration and off-by-one
    [ ] Memory access errors
    [ ] Memory leaks
    (e.g. Do raw pointer allocations have equivalent deallocations that all code paths go through? Use valgrind.)
    [ ] Incorrect behaviour
    [ ] Switch/break fallthrough
    [ ] Exception safety
    - [ ] Correctness of algorithms
    - [ ] Loop iteration and off-by-one
    - [ ] Memory access errors
    - [ ] Memory leaks
    (e.g. Do raw pointer allocations have equivalent deallocations that all code paths go through? Use valgrind.)
    - [ ] Incorrect behaviour
    - [ ] Switch/break fallthrough
    - [ ] Exception safety

    Security (C/C++)
    --------
    [ ] Array bounds on buffers
    [ ] Are files created and read/written? Check permissions, races
    [ ] All user input sanitized or validated?
    [ ] Proper, bounded use of `str*`, `printf`, `f*` functions?
    [ ] Validate remote host cert validity & identity
    [ ] Crypto use
    - [ ] Array bounds on buffers
    - [ ] Are files created and read/written? Check permissions, races
    - [ ] All user input sanitized or validated?
    - [ ] Proper, bounded use of `str*`, `printf`, `f*` functions?
    - [ ] Validate remote host cert validity & identity
    - [ ] Crypto use
  5. @dideler dideler revised this gist Jul 2, 2013. 1 changed file with 27 additions and 1 deletion.
    28 changes: 27 additions & 1 deletion code_review_checklists.md
    View file
    Original file line number Diff line number Diff line change
    @@ -10,13 +10,39 @@ Based on the article: [Using checklists for code review](http://blog.rbcommons.c
    User interface
    --------------
    [ ] Are screenshots provided for all UI changes?
    [ ] Does this change introduce any new concepts or models? Evaluate.
    [ ] How much expertise and context will a prospective user need?
    [ ] Do we do anything similar elsewhere? Can we build on existing knowledge or habits?
    [ ] Do we do anything similar but just different enough to cause problems with old habits?
    [ ] Are there any new multi-step workflows? How long and complex are they?
    [ ] If users make mistakes or errors, what are the failure conditions? Can mistakes be undone?
    [ ] How is the copy? Are sentences well-formed and clear? Any spelling errors or typos?
    [ ] Can the UI layout be localized into different language editions without any changes to the source code?

    Visual design (native desktop app)
    -------------
    [ ] Are controls laid out in a way which makes sense given visual scan order?
    [ ] Check for alignment
    [ ] Check for layout, spacing, and padding
    [ ] Check for visual consistency

    Correctness (C/C++)
    -----------
    [ ] Correctness of algorithms
    [ ] Loop iteration and off-by-one
    [ ] Memory access errors
    [ ] Memory leaks
    (e.g. Do raw pointer allocations have equivalent deallocations that all code paths go through? Use valgrind.)
    [ ] Incorrect behaviour
    [ ] Switch/break fallthrough
    [ ] Exception safety

    Security (C/C++)
    --------

    [ ] Array bounds on buffers
    [ ] Are files created and read/written? Check permissions, races
    [ ] All user input sanitized or validated?
    [ ] Proper, bounded use of `str*`, `printf`, `f*` functions?
    [ ] Validate remote host cert validity & identity
    [ ] Crypto use
  6. @dideler dideler created this gist Jul 2, 2013.
    22 changes: 22 additions & 0 deletions code_review_checklists.md
    View file
    Original file line number Diff line number Diff line change
    @@ -0,0 +1,22 @@
    Based on the article: [Using checklists for code review](http://blog.rbcommons.com/2013/04/24/using-checklists-for-code-review/)

    > In general, people are pretty good at the code review process, but it's sometimes surprising what can slip through. A natural consequence of the way our brains look at the world is that it's easy to pay a lot of attention to small details and code style flubs, and completely miss the big picture.
    > Obviously, not everything is applicable for every change. If the review request isn't making any changes to UI, then skip the first two checklists entirely. If a change is a bug fix, typically don't review it for architecture and design principles.
    > Put the big stuff first (e.g. architecture). You don't want to work through a ton of small issues before realizing that everything has to be rewritten.
    > Do a pass through the code for each and every item in the checklist. By only looking for a very specific type of defect, each pass goes relatively quickly, even for large changes. Focusing on only one type of defect allows an extreme focus that makes it trivial to find issues.
    User interface
    --------------

    Visual design (native desktop app)
    -------------

    Correctness (C/C++)
    -----------

    Security (C/C++)
    --------