# Code Review Guidelines

Code review is a fundamental part of FeDIY's development process. It's an opportunity to share knowledge, catch issues early, and build a stronger codebase together. This guide outlines how we conduct reviews in a respectful, inclusive, and constructive way.

## Core Principles

### 1. **Assume Good Intent**

Contributors come to FeDIY to build something good. Assume they're doing their best and want their code to be correct.

### 2. **Focus on the Code, Not the Person**

Review code and ideas, not the developer. Avoid comments like "This is stupid" or "You should know better." Instead: "This approach could lead to X issue. Could we try Y instead?"

### 3. **Respect Diverse Perspectives**

Different developers have different experiences, backgrounds, and approaches. Sometimes there's more than one right way to solve a problem.

### 4. **Be Constructive**

Your job as a reviewer isn't to reject ideas but to help the contributor improve their work. Ask questions, suggest alternatives, and explain your reasoning.

### 5. **Respond Promptly**

Aim to provide initial feedback within 48 hours. This keeps momentum and shows contributors we value their work.

### 6. **Be Kind**

Code review happens asynchronously and in writing. Words can be misinterpreted. Err on the side of kindness and clarity.

## What to Review

### Correctness

- Does the code do what it claims to do?
- Are edge cases handled?
- Could this code fail in unexpected ways?
- Are there potential security, performance, or accessibility issues?

### Design and Architecture

- Does this change align with our [ARCHITECTURE.md](ARCHITECTURE.md)?
- Does it follow our design patterns?
- Could this be implemented more simply?
- Will this change make the codebase harder to understand or maintain?

### Tests

- Are there tests? Do they cover the happy path and edge cases?
- Do tests actually test the claimed behavior?
- Would a reviewer understand what the code should do from the tests?

### Documentation

- Is the code documented? Is the documentation clear?
- Are public APIs documented?
- Are complex algorithms explained?
- Is there a comment explaining *why* a particular approach was taken?

### Code Quality

- Does it follow our style guide?
- Is it formatted correctly (`cargo fmt`)?
- Does it pass `cargo clippy`?
- Are variable and function names clear?

## How to Review

### Do

✅ **Ask questions:** "What does this function return when X happens?"

✅ **Explain your reasoning:** "I'm concerned about this approach because it could lead to Y. Have you considered Z?"

✅ **Suggest alternatives:** "Another way to solve this might be..."

✅ **Praise good work:** "Great catch on this edge case!" or "I like how you handled this—it's elegant."

✅ **Admit uncertainty:** "I'm not sure about this part. Can you help me understand?"

✅ **Provide context:** "This pattern conflicts with how we do it in module X. Let's align them."

✅ **Acknowledge limitations:** "This might be a trade-off between X and Y. Let's discuss."

### Don't

❌ **Nitpick style:** Linters and formatters handle this. Focus on substance.

❌ **Demand perfection:** Good is good enough. Encourage incremental improvement.

❌ **Approve blindly:** Actually review the code.

❌ **Leave vague feedback:** "This doesn't seem right" isn't helpful. Be specific.

❌ **Require your preferred style:** There's often more than one right way.

❌ **Comment on things outside the PR's scope:** Stay focused.

❌ **Use harsh language:** Code review isn't a place for snark or condescension.

## Example Reviews

### ❌ Not Great

```
This is wrong. Fix it.
```

(Not helpful, unclear, and discouraging.)

### ✅ Better

```
I'm concerned about this approach. The function reads `self.state` but doesn't 
check if it's been initialized. In our current code, this could panic. 

Have you considered checking `is_initialized()` first, or would it make sense 
to initialize in the constructor? 

I'm happy to help think through this if you'd like!
```

(Specific, explains the problem, suggests solutions, and is encouraging.)

## Review Checklist

Before approving a PR, confirm:

- [ ] Code is correct and handles edge cases
- [ ] Changes align with project architecture
- [ ] Tests are adequate and pass
- [ ] Documentation is clear
- [ ] Code follows style guidelines (`cargo fmt` and `cargo clippy` pass)
- [ ] Commit messages are clear
- [ ] No security, performance, or accessibility red flags
- [ ] Feedback has been respectful and constructive

## Handling Disagreement

Sometimes reviewers and contributors disagree. This is normal and healthy. When this happens:

1. **State your position clearly.** Explain why you think something should be different.
2. **Ask for their perspective.** They might have information you don't.
3. **Seek common ground.** Often there's a compromise that works.
4. **Escalate if needed.** If you can't agree, involve a maintainer.
5. **Respect the decision.** Once a maintainer decides, everyone moves forward.

Remember: disagreement about code is not personal. We're all trying to build something good.

## Feedback on Feedback

Code reviewers are also learning. If you feel feedback was disrespectful or unhelpful:

1. **Assume good intent.** The reviewer probably wasn't trying to be harsh.
2. **Ask for clarification.** "I'm not sure what you mean. Could you elaborate?"
3. **Suggest improvement.** "It would help me to understand the concern better if you explained..."
4. **Report if needed.** If the reviewer violates our Code of Conduct, report it.

## Special Cases

### First-Time Contributors

Spend a bit more time with PRs from first-time contributors. They're learning the project and may not know all our conventions yet. A bit of extra patience now builds a better long-term contributor.

### Large Changes

Big PRs are harder to review. If you're submitting a large change:

- Open an issue or discussion first to get buy-in on the approach
- Consider breaking it into smaller PRs
- Explain the overall goal clearly

As a reviewer of large changes, don't hesitate to ask for clarification or suggest breaking it up.

### Sensitive Topics

Some PRs touch on moderation, content policy, security, or inclusion. These need thoughtful, careful review:

- Take time to understand the context
- Consider asking multiple reviewers for input
- Be respectful of different perspectives
- Remember the human impact of these decisions

## Resources for Better Reviewing

- [Google Code Review Guidelines](https://google.github.io/eng-practices/review/)
- [Thoughtbot Code Review Etiquette](https://thoughtbot.com/blog/code-review-etiquette)
- [Rust API Guidelines](https://rust-lang.github.io/api-guidelines/)
- [OWASP Secure Coding Practices](https://owasp.org/www-project-secure-coding-practices-quick-reference-guide/)

## Questions?

- Unsure about a review? Ask for help in a comment.
- Feedback on the review process? Open an issue tagged `review-process`.
- Code of Conduct concerns? See [CODE_OF_CONDUCT.md](CODE_OF_CONDUCT.md).

---

Thank you for contributing your time and expertise through code review. It makes FeDIY better!

**Last Updated:** May 2026