Quy Trình Code Review trong Java
1. Nguyên Tắc Code Review
1.1 Mục Tiêu Code Review
- Đảm bảo chất lượng code
- Phát hiện lỗi sớm
- Chia sẻ kiến thức
- Đảm bảo tính nhất quán
- Tuân thủ coding standards
- Cải thiện maintainability
- Tối ưu hóa performance
- Đảm bảo security
1.2 Quy Tắc Code Review
- Review code, không review người
- Tập trung vào logic và thiết kế
- Đưa ra feedback mang tính xây dựng
- Ưu tiên vấn đề quan trọng
- Tránh tranh luận về code style
- Đặt câu hỏi thay vì ra lệnh
- Tôn trọng quan điểm của người khác
- Giải thích lý do khi yêu cầu thay đổi
2. Quy Trình Code Review
2.1 Chuẩn Bị Pull Request
// Checklist trước khi tạo PR:
// 1. Self-review code changes
git diff main...feature-branch
// 2. Cập nhật documentation
/**
* Processes customer orders with validation and notification.
*
* @param request Order request containing customer and items info
* @return OrderResponse with status and order details
* @throws ValidationException if order validation fails
*/
public OrderResponse processOrder(OrderRequest request) {
// Implementation
}
// 3. Thêm/cập nhật unit tests
@Test
void whenProcessValidOrder_thenSuccess() {
// Test implementation
}
// 4. Chạy tests và static analysis
./mvnw clean verify
./mvnw sonar:sonar
// 5. Viết PR description rõ ràng
/*
Title: [JIRA-123] Implement Order Processing Feature
Changes:
- Add OrderProcessor service
- Implement validation logic
- Add unit tests
- Update documentation
Testing:
- Unit tests added
- Integration tests passed
- Manual testing completed
*/
2.2 Review Process
// 1. Code Changes Review
public class OrderService {
// REVIEW: Consider using Optional for nullable returns
public Customer findCustomer(Long id) {
return customerRepository.findById(id);
}
// REVIEW: Method too long, consider breaking down
public Order processOrder(OrderRequest request) {
// 50+ lines of code
}
// REVIEW: Missing validation
public void updateOrder(Order order) {
orderRepository.save(order);
}
}
// 2. Test Coverage Review
@Test
void whenProcessOrder_thenSuccess() {
// REVIEW: Missing edge case tests
OrderRequest request = new OrderRequest();
orderService.processOrder(request);
}
// 3. Documentation Review
/**
* REVIEW: Missing @throws documentation
* @param id customer ID
* @return Customer object
*/
public Customer getCustomer(Long id) {
// Implementation
}
3. Code Review Checklist
3.1 Functional Requirements
- Business logic đúng yêu cầu
- Xử lý edge cases
- Error handling đầy đủ
- Validation logic
- Security considerations
- Performance impacts
- Backward compatibility
3.2 Technical Requirements
// 1. Code Quality
public class OrderService {
// REVIEW: Magic numbers
private static final int TIMEOUT = 5000;
// REVIEW: Hardcoded strings
private static final String ERROR_MESSAGE =
"Order processing failed";
// REVIEW: Complex conditions
public boolean isValidOrder(Order order) {
return order != null &&
order.getItems() != null &&
!order.getItems().isEmpty() &&
order.getCustomer() != null;
}
}
// 2. Testing
@Test
void testOrderProcessing() {
// REVIEW: Missing assertions
orderService.processOrder(request);
}
// 3. Performance
public class CacheConfig {
// REVIEW: Cache settings need review
@Bean
public Cache orderCache() {
return new ConcurrentMapCache("orders");
}
}
4.1 Code Style
// Bad comment
"Fix this code"
// Good comment
"Consider extracting this logic into a separate method for better readability and reusability"
// Bad comment
"Wrong"
// Good comment
"This method might throw NullPointerException when customer is null. Consider adding null check or using Optional"
4.2 Design Issues
// Bad Design
public class OrderService {
// REVIEW: High coupling
private CustomerService customerService;
private InventoryService inventoryService;
private PaymentService paymentService;
private NotificationService notificationService;
public void processOrder(Order order) {
customerService.validate(order.getCustomer());
inventoryService.checkStock(order.getItems());
paymentService.process(order.getPayment());
notificationService.sendConfirmation(order);
}
}
// Better Design
public class OrderService {
private final OrderProcessor orderProcessor;
public void processOrder(Order order) {
orderProcessor.process(order);
}
}
public class OrderProcessor {
private final List<OrderStep> steps;
public void process(Order order) {
steps.forEach(step -> step.execute(order));
}
}
- GitHub Pull Requests
- GitLab Merge Requests
- Gerrit
- Crucible
- Review Board
5.2 Static Analysis
<!-- pom.xml -->
<plugin>
<groupId>org.sonarsource.scanner.maven</groupId>
<artifactId>sonar-maven-plugin</artifactId>
<version>${sonar.version}</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>${checkstyle.version}</version>
<configuration>
<configLocation>checkstyle.xml</configLocation>
</configuration>
</plugin>
6. Best Practices
6.1 Review Size
- Giới hạn số lượng files (< 10 files)
- Giới hạn số lượng changes (< 400 lines)
- Chia PR lớn thành nhiều PR nhỏ
- Focus vào một feature/fix
6.2 Review Timing
- Review code thường xuyên
- Phản hồi nhanh (trong 24h)
- Dành thời gian thích hợp
- Tránh review quá nhiều code một lúc
7. Metrics và Monitoring
7.1 Review Metrics
public class ReviewMetrics {
// Review time
private Duration reviewTime;
// Number of comments
private int commentCount;
// Number of iterations
private int iterationCount;
// Time to merge
private Duration timeToMerge;
// Code coverage change
private double coverageChange;
}
7.2 Quality Gates
# sonarqube-quality-gates.yml
qualityGates:
- name: "Code Review Quality Gate"
conditions:
- metric: new_coverage
op: LT
error: "80"
- metric: new_duplicated_lines_density
op: GT
error: "3"
- metric: new_maintainability_rating
op: GT
error: "1"
- metric: new_reliability_rating
op: GT
error: "1"
- metric: new_security_rating
op: GT
error: "1"
8. Documentation
8.1 Review Guidelines
# Code Review Guidelines
## Pull Request Template
### Description
- What does this PR do?
- Why is this change needed?
- How has it been tested?
### Checklist
- [ ] Code follows style guidelines
- [ ] Tests added/updated
- [ ] Documentation updated
- [ ] Self-review completed
- [ ] All CI checks pass
## Review Response Template
### Addressing Review Comments
- Clear explanation of changes made
- Reasoning for not making suggested changes
- Questions for clarification
// Template for major issues
"[Major] This implementation might cause {issue} because {reason}.
Consider {suggestion} instead."
// Template for minor issues
"[Minor] Consider {suggestion} to improve {aspect}."
// Template for questions
"[Question] Could you explain why {decision} was made instead of {alternative}?"
// Template for praise
"[Good] Nice implementation of {feature}. This is a good example of {principle}."
9. References và Further Reading
9.1 Books và Articles
- Code Complete (Steve McConnell)
- Clean Code (Robert C. Martin)
- The Art of Code Review (Michael Feathers)
- Code Review Best Practices (Martin Fowler)
- Peer Reviews in Software (Karl E. Wiegers)
9.2 Online Resources