Skip to content

Instantly share code, notes, and snippets.

@mrcnc
Last active January 6, 2023 23:17
Show Gist options
  • Save mrcnc/ba3a4a4320a9419f9d38fe50ea174dc3 to your computer and use it in GitHub Desktop.
Save mrcnc/ba3a4a4320a9419f9d38fe50ea174dc3 to your computer and use it in GitHub Desktop.
Some thoughts on what to look for in a code review

Python

Use precise exception types

ValueError

General

Should the code be decomposed further?

In general, code should be broken down into small parts. Making small modular peices of code promotes reuse and composability. It also makes it easier to test. I like Sandi Metz rules on this:

Classes should not be longer than 100 lines of code.
Methods should not be longer than 5 lines of code.
Methods should not accept more than four parameters (and don't try to get around this by accepting a hashmap as a parameter)

These rules are practical and you should stick to them, unless you can explain why it's ok to break the rule. (aka "practicality beats purity") For instance, a longer method may make sense b/c you and the code reviewers agree that a longer method is more readable. But if you're passing in more than 4 parameters, maybe you're doing too much within a function/method.

Separation of concerns

Does the code follow the Unix philosopy of "do one thing, do it well"? Ususally your exeperience will tell you if something has a "separation of concerns" code smell.

You can usually rewrite/refactor something to make it more modular...but be careful of over abstraction.

Can you improve the nomenclature?

We should be striving to make the code easily maintained by people who didn't originally write it. This is why naming things matters. This is why when it comes to naming, I follow the Zen of Python rule: "Explicit is better than implicit." So I tend to spell out words instead of trying to abbrevate. Short or single letter variable names are fine in short methods with few variables but otherwise, stick to explicit names.

Testing

Does your new code have useful tests? Do they run quckily? Ideally the tests will be run on the CI whenever you try to make a PR.

Side effects

Can you remove them to promote a functional style?

Immutability

Prefer immutable values...not immutablity by convention.

Dependency injection

Constructor-based dependency injection will help with testing and also makes the Class's dependencies explicit. Also, using setters is mutating state...so just don't do it.

Encapsulation

Is there global scope? Is it really needed (can you explain why)?

Style preferences

If the language allows, explicitly declare the return types of methods that you expect others to use (aka any public method)

You should have a CI or pre-commit hook that runs a code linter so you don't have to look at code that doesn't pass linting.

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