playbooks

home / skills / pedrosantiagodev / buildup / review-changes

review-changes skill

/.claude/skills/review-changes

This skill guides code reviewers to evaluate Java and Spring changes for robustness, maintainability, and best-practice adherence across tests and

npx playbooks add skill pedrosantiagodev/buildup --skill review-changes

Review the files below or copy the command above to add this skill to your agents.

Files (1)
SKILL.md
2.6 KB
---
name: review-changes-java
description: Systematic code review workflow to evaluate changes against Java and Spring standards. Use when reviewing Pull Requests, commits, or diffs. Ensures robustness, maintainability, and adherence to best practices.
---

# Review Changes (Java/Spring Boot)

Systematic workflow for reviewing Java/Spring code against established standards.

## 1. Understand the Change
- What problem does this solve?
- What is the scope of impact?
- Are tests included and do they cover the use cases?

## 2. Java Best Practices Review
- **Robustness:**
    - Is `try-with-resources` used to close resources?
    - Is exception handling correct?
- **Immutability:**
    - Are fields declared `final` where possible?
    - Could the class be immutable?
- **Types:**
    - Are Raw Types (e.g., `List`) avoided?
    - Is `Optional` used correctly (e.g., `isPresent()` followed by `get()`, or prefer `orElse`, `map`)?
- **Contracts:**
    - If `equals()` is implemented, is `hashCode()` also implemented?
- **Lambdas/Streams:**
    - Is the streams code readable?
    - Are Method References used where possible?
- **Naming:**
    - Clear and descriptive names (no abbreviations)?

## 3. Spring Patterns Review
- **Dependency Injection:**
    - Is injection done via **constructor**? (Avoid `@Autowired` on fields).
- **Configuration:**
    - Is it using `@ConfigurationProperties` (preferred) or `@Value`?
- **Layers:**
    - Is business logic in the `@Service`?
    - Is the `@Controller` only orchestrating the call (no business logic)?
    - Is the `@Repository` called directly by the `Controller`? (Should not be).
- **Transactions:**
    - Is `@Transactional` used correctly (generally on public `Service` methods)?
- **Web:**
    - Is `WebClient` used instead of `RestTemplate`?
- **Security:**
    - Are endpoints secured by Spring Security?
    - Is sensitive data excluded from logs?

## 4. Testing
- **Unit Tests:** Use Mockito correctly?
- **Test Slices:** Is the correct slice used (e.g., `@WebMvcTest`)?
- **Testcontainers:** Is it used for integration tests with a real database?
- **Assertions:** Are assertions clear and testing behavior, not implementation?

## 5. Feedback Format

### Required Changes (Blocking)
- Security vulnerabilities.
- Violations of Java contracts (e.g., equals/hashCode).
- Use of Raw Types.
- Field-based dependency injection.

### Suggested Improvements (Non-blocking)
- Refactoring opportunities (e.g., use Method Reference).
- Naming could be clearer.
- Convert `@Value` to `@ConfigurationProperties`.

### Positive Feedback
- Great use of Testcontainers!
- Good application of the Builder pattern.

Overview

This skill provides a systematic code review workflow for Java and Spring Boot changes. It helps reviewers evaluate pull requests, commits, or diffs for robustness, maintainability, and adherence to Java and Spring best practices. The goal is to catch blocking issues, suggest improvements, and highlight positive patterns.

How this skill works

The workflow inspects change intent and scope, then runs targeted checks across Java idioms, Spring patterns, and testing quality. It identifies blocking issues (security, contract violations, raw types, field injection) and non-blocking suggestions (refactors, naming, configuration improvements). Finally, it formats feedback into required changes, suggested improvements, and positive notes.

When to use it

  • Reviewing pull requests that add or modify Java/Spring code
  • Evaluating commits or diffs before merge to main branches
  • Auditing service-layer changes for transaction and DI correctness
  • Checking tests and integration setups (Testcontainers, slices)
  • Validating security-sensitive changes and logging of secrets

Best practices

  • Verify resource handling with try-with-resources and correct exception flow
  • Prefer constructor injection and avoid field-based @Autowired
  • Use @ConfigurationProperties for structured config instead of many @Value fields
  • Ensure equals() and hashCode() are implemented together and consistently
  • Avoid raw types; prefer generics and use Optional idiomatically
  • Keep controllers thin, services stateful-free of web orchestration

Example use cases

  • A PR introducing a new REST endpoint: check controller orchestration, service placement, and endpoint security
  • A refactor replacing RestTemplate with WebClient: verify async usage and error handling
  • A feature adding database access: confirm @Repository usage, transactional boundaries, and Testcontainers integration
  • A bugfix changing domain equality: ensure equals/hashCode contract and related collections behavior
  • New configuration properties: recommend converting scattered @Value annotations into a @ConfigurationProperties class

FAQ

What counts as a blocking issue?

Security vulnerabilities, violations of Java contracts (like equals/hashCode mismatch), use of raw types, and field-based dependency injection are blocking.

When should I prefer @ConfigurationProperties?

@ConfigurationProperties is preferred when you have grouped or many related settings; it improves typing, validation, and testing versus scattered @Value usages.