Appearance
Code Review
- Help look for bugs, logic problems, or uncovered edge cases.
Core Review chart
These are some sentiments toward code.
| - | Appreciated | Interesting | Perplexing | Removable | Problematic |
|---|---|---|---|---|---|
| Bug Squasher | Wow, this documentation is great! | ||||
| Nice Docs | Wow, this documentation is great! | ||||
| Good recall | Glad you remembered to do this! | ||||
| Hard Work | This looks like it was hard work, thanks for taking it on. | ||||
| Security | Way to shore up our security here. | ||||
| Debt Reduction | Thank you for reducing the tech debt here, it will benefit us in the long term. | ||||
| TIL | - | Thank you -- reading this code taught me something new. | |||
| Watching | - | I'm looking forward to watching what you do next with this code. | |||
| Reminder | Adding this code reaction so that I can find this line and review it later | ||||
| Smart | Smart approach here. | ||||
| Favorite | Wow, this was my favorite code change I've seen in the past month! | ||||
| Hard to adapt | This code looks like it will be difficult to adapt/maintain in the future, because... | ||||
| Needs docs | Could you add documentation for this? | ||||
| Not findable | Can we make this easier to find for future maintainers? | ||||
| Too dense | This code is too dense for me to easily follow. Can we break it apart into multiple lines or its own method? | ||||
| Ensure Test | Can you ensure a test exists for this? | ||||
| DRY it? | This code seems redundant, can we reuse existing code instead? | ||||
| Move | I'd propose we consider moving this code to... | ||||
| Magic Number | Can we remove this magic number in favor of a named constant? | ||||
| Creates debt | This seems like it will contribute to tech debt, can we clean it up a bit? | ||||
| FUD | Have you tried removing what looks like it may be FUD here? | ||||
| Problematic | Is this a bug? | ||||
| Security | A security consideration to take into account: ... | ||||
| Performance | Can we speed this up? It doesn't look like it will be sufficiently performant for its use case. | ||||
| Internationalization (i18n) | There is an internationalization concern here: | ||||
| Convention Mismatch | This is an unorthodox solution. The more conventional way to accomplish this would be.... |
Code Review Best Practices for everyone
- Be kind.
- Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and reach a resolution quickly.
- Ask questions; don’t make demands. (“What do you think about naming this
:user_id?”) - Ask for clarification. (“I didn’t understand. Can you clarify?”)
- Avoid selective ownership of code. (“mine”, “not mine”, “yours”)
- Avoid using terms that could be seen as referring to personal traits. (“dumb”, “stupid”). Assume everyone is intelligent and well-meaning.
- Be explicit. Remember people don’t always understand your intentions online.
- Be humble. (“I’m not sure - let’s look it up.”)
- Don’t use hyperbole. (“always”, “never”, “endlessly”, “nothing”)
- Be careful about the use of sarcasm. Everything we do is public; what seems like good-natured ribbing to you and a long-time colleague might come off as mean and unwelcoming to a person new to the project.
- Consider one-on-one chats or video calls if there are too many “I didn’t understand” or “Alternative solution:” comments. Post a follow-up comment summarizing one-on-one discussion.
- If you ask a question to a specific person, always start the comment by mentioning them; this ensures they see it if their notification level is set to “mentioned” and other people understand they don’t have to respond.