# Some thoughts on reviewing code

- Try to be kind :)
- Talk with the PR author, ask for explanations, ask how things work and explain your point as well, then ask for feedback. Request PR description if it's not there
- Don't leave out things you don't understand. Gather a broader context if necessary
- Aesthetic issues sometimes indicate relevant problems. Don't ignore them, but if they are purely aesthetic, leave the author the freedom to have a different taste ;)
- If you're about to suggest a big change, consider creating an issue and moving that to a separate PR
- Resolve comments when they are fixed. This makes it easier to see what should be still improved.
- If the PR is based on some large RFC/specification, have a brief look and use it to verify things. However, avoid spending too much time on it - ask the PR author for an explanation instead
- If a PR is big, try to focus on particular functionalities separately
- Also, don't look at every aspect of the PR at once, especially if it's a big one. Instead, divide the process into phases and focus only on particular aspects in each phase. Submitting the review before proceeding to the next phase may be profitable, as you don't flood the author with comments and they can focus on what's most important. That also reduces the risk of comments becoming outdated by changes made for other comments' sake. Here comes a suggested way of splitting the review:

- At first, look at the public API. The public API is whatever a user of the given thing will have access to - public modules' names, functions, data types and documentation. The public API should be:
- Small - containing only things that are really needed
- Extensible - it should be possible to add new options/features without breaking backwards compatibility
- Simple and predictable - no unnecessary magic should be involved
- Intuitive - it should be easy to grasp what's going on. If it's not straightforward for you, it may not be for others too.
- Documented - with typespecs, examples, links to resources etc. Generate the documentation and see how it looks like

- Secondly, look at the architecture. Make sure that:
- Each module has a clear, single responsibility
- Private modules have reasonable API as well
- Data flow is intuitive - no spaghetti code
- Architecture is consistent, proper abstractions are created to ensure that
- There are no abstractions that complicate things instead of simplifying
- Chosen tools and mechanisms are appropriate for the needs - for example using processes is not appropriate for code organisation

- Finally, look at the code quality. Make sure that:
- The code is as easy to read and understand as possible
- Any unavoidable quirks have comments explaining them
- Variables, functions, attributes and whatever else can be named is named in a way that the name reflects what that thing is or does
- No business logic is duplicated
- Complexity is reasonably spread along modules and functions (keep private functions in mind as well):
- There are no extremely long and complex modules and functions
- Modules and functions are complex enough, so that the reader doesn't have to jump between them too much to understand what's happening
- Errors are handled properly
- The code is well tested