loading...

Code Review Checklist (Brain Backup/Export)

tomerbendavid profile image Tomer Ben David ・3 min read

Following is a kinda backup-export of the back of my mind (IMHO) code review checklist. This export serves as a backup for my brain which is overloaded in anyway ;) I get back to it from time to time, to refresh and reimport.

Code Review Checklist:

  1. Logging
  2. Monitoring
  3. Scale
  4. Traceability
  5. Troubleshooting
  6. Reporting
  7. UX
  8. Incorrect problem definition
  9. Time management
  10. Ignoring other features.
  11. Focusing on today needs.
  12. Alerting
  13. Metrics
  14. Testing
  15. Deployment
  16. Code cleanup
  17. Latency
  18. No design review
  19. No customer review
  20. Concurrency
  21. Simple bugs
  22. Mutation
  23. Readability
  24. Naming
  25. Documentation (motivation)
  26. Audit
  27. Security
  28. Complex if-then logic
  29. Suspicious loop 'break'
  30. Negative logic
  31. Regexp - long input could cause issues.
  32. Nulls
  33. More than 3 arguments
  34. Line length
  35. Shared resources abuse
  36. Cache (think hard)
  37. Variable name length
  38. Commit message
  39. Squash if needed
  40. Build time change
  41. "Random" usage smell
  42. Open Close principle
  43. Code duplication
  44. OverComplexity (ex. Types where string is enough)
  45. UnderComplexity (ex. strings where type is required)
  46. Bulk test fail smell
  47. Backward compatibility
  48. Forward compatibility
  49. API no version
  50. Incorrect db type usage
  51. Adding indexes instead of search engine view
  52. Ignoring Failures
  53. Expecting feature would work
  54. Too verbose error handling
  55. Sensitive info in logs
  56. Missing "main" flow
  57. Feature toggle (if needed)
  58. Dependencies (collisions? new one?)
  59. Comments smell
  60. Dependent Teams
  61. Plural/Singular correctness
  62. Consistency with codebase conventions
  63. Revert implications
  64. Either simple MultiReturn or Single
  65. Time to understand code smell

[Update] Post AgentDenton comment: The way I use the list is just scan from time to time to ignite some neurons running in my rather dead brain, so when I get to code reviews, I use those few friendly code review neurons. Meaning I don't sit or use this list explicitly in code reviews, it's just the sitting on the back of my rather unorganized brain waiting for a code review to appear! ;)

I have yet to see a feature that was coded with the right requirements, if the developer did not question hard the requirements, it seems like always there is a slight change in requirement's if you think about the problem hard enough, if not reviewed properly, sometimes the requirements change altogether. If you didn't monitor, log, make sure you have tracing, back office in place, you will suffer in future. Did it effect reporting? if yes take care of it. Is the UX correct? Did you estimate the task correctly time wise. Are you focusing on today's need's too much, neglecting the future? it would cost you more than you think.

Clarification No "main" flow - I always like to see little mains so I can see the flow, (that is instead of a function that calls a function that calls a function). If someone manages to get a proper concise short "main" then everyone looking at his logic would know what the system does.

No chance I get to all above during a code review, that's why it's sitting on the back of the mind and not on the frontal cortex, I trust our unconscious brain to pop up the right thing at the right time, and if not, it's time to meditate to increase the chances of that happening. ;)

Posted on by:

tomerbendavid profile

Tomer Ben David

@tomerbendavid

Check out my podcast programmers quickie - https://podcasts.google.com/?feed=aHR0cHM6Ly9hbmNob3IuZm0vcy8xMzMwMjI0L3BvZGNhc3QvcnNz&ep=14

Discussion

pic
Editor guide
 

That is quite the list you've got there. It's not 100% clear to me how you use it in code review... Do you categorize comments into these?

If that's the case, I've adopted a similar style, but with fewer categories.

  • Convention
    • Naming conventions, patterns, process, etc.
  • Error
    • Blatant error in code / logic / understanding.
  • Logic
    • Purpose of the code is not clear from simply reading it.
  • Simplify
    • Opportunity to refactor code into a method or replace code with framework method.

I use these as prefixes to my comment, so that it's rather easy to infer the sentiment behind the comment which follows.

Perhaps mine is too simple, but I've found it works for much of the time spent reviewing code :)

I like your expanded list, and I might adopt a few when the opportunity presents itself :)

 

Thanks I like that yours is categorized and concise. The way I use mine is more like as a general intuition snapshot reminder, I just scan it for a few seconds from time to time and when i get to code review some things from the list pop up and i use them. We could call it brain-pop-up-list to scan fro m time to time. ;)

 

Ah, I see now. It's not so much the comments itself as it is the things you are looking at when reviewing code.

In that case, this is quite handy. I do bear a lot of these in mind when reviewing code, but it is quite implicit in my mind already; many years of doing this sort of thing just does that :-)

But I'm actually about to introduce a code review process at a new company, and this would help explain to the rest what to look out for.

Great post!