How do you apply code review feedback to different types of code, such as legacy, test, or documentation?

Scope is key. Most of the time, we find opportunities of refactoring that are technically out of scope of the current task. When that happens, I document the need for a refactor in whatever task management tool I’m using. I also try to give the best contextual information in the ticket to justify its need and to give whoever is going to implement the ticket enough information to understand what needs to be done. On that note, I usually don’t decide the scope by myself. The decision is very often done with at least someone else. Preferably with the tech leader and/or a product owner. That’s important because managing priorities and future plans are key information when deciding the proper moment to refactor.

Prioritize Critical Feedback: - Fix Bugs: Addressing bugs should be the top priority. - Security Issues: Security vulnerabilities in legacy code can pose serious risks. - Performance Problems: Enhancing performance is crucial for legacy systems. Gradual and Incremental Refactoring: - Automated Tests: Implement automated tests to ensure that changes do not break existing functionality. - Tools: Utilize refactoring tools to automate parts of the process. Effective Communication: - Context and History: Understand the context and history of the legacy code. - Clarification and Feedback: Maintain open communication with reviewers and team members.

It's easy to pay less attention to tests, but there is a world of difference between good tests and bad tests. Good tests have clear descriptions of test cases, and each test case should be independently readable (DAMP, not DRY; https://enterprisecraftsmanship.com/posts/dry-damp-unit-tests/).

Consistency with Production Code: Ensure test code adheres to the same naming conventions, formatting, documentation, and structure as production code. Comprehensive Coverage: Create test cases that cover expected behavior, edge cases, and error scenarios. Descriptive Test Names: Use clear and descriptive names for test cases to convey their purpose. Unique Test Logic: Ensure each test case addresses a unique aspect of the code, avoiding redundancy. Isolation of Dependencies: Utilize fixtures, mocks, and stubs to isolate test code from external dependencies and control inputs.

One of the main things that makes documentation changes different from code changes is that the risk of breakage is practically zero. Sometimes it may make sense to allow committing or merging before review. The advantage of this might be promoting more changes and more rapid small changes to documentation (for example, allowing readers to quickly fix one little thing without a heavy review process) which may lead to better documentation. That said, review is still important, and getting a teammate to read and make suggestions leads to better and more readable documentation.

Fostering a Culture of Continuous Improvement: Example: In every project, promoting an environment where feedback is viewed positively has been key. For instance, during the fraud detection system implementation, we encouraged team members to see feedback as an opportunity for growth rather than criticism. Consideration: Cultivate an open, constructive feedback culture to drive continuous improvement and innovation.