Code Review Checklists

code_review.md

General

• Does this cover any code you've personally worked on in the past 6 months? -- If so, you'll want to do a more thorough review to ensure it doesn't cause you any later headaches
• Will you have to work on this code or area of the codebase in the future? -- Also a good reason to do a more thorough review of the code base.
• Is the code cleanly formatted? (whitespace, code organization, commenting, etc)
• Do the tests pass?
• Is there commented code that can be removed?
• If function calls are removed, do those functions get called anywhere else? and if not can the function itself be removed?
• Does this code introduce any new dependencies?

Python

• Are there kwarg defaults with objects?
• Has pylint been run on this code? Is there anything concerning to address?
• If database models are added or updated, do they follow the right denormalization/or normalization patterns for that particular case? E.g. most operational tables should be mostly denormalized
• inside of new or existing classes is there a lot of intertwined self logic? Do they overabuse the use of internal state making for difficult to test code?
• does the new code follow typical python conventions. (e.g. ryan's hack of arg_dict instead of properly using kwargs)
• be wary of too much setattr or hasattr usage that can obfuscate or create too much magic code. magic code is fine if it's well documented and succinct, but randomly put into regular operating classes leads to big headaches when others try to understand, test, or debug that code.

Django

• has a migration been added that uses RunPython? If so ensure that it uses app.get_model for any database access, e.g. don't use call_command in a migration

Scala

• are there large try blocks? What do they catch and why?
• are there large if/blocks? Can the logic be refactored to be a bit more functional
• are there extraneous comments added by an IDE?
• is there unnecessary use of the var keyword?
• is there abuse (or lack of use) on the type system?
• does the code use an import thing._, if so why?
• is there use of scala mutable collections?