Java Code Review

decebals/claude-code-java/skills/java-code-review

作者 decebals0d98fe9bd62923e819568ee1e041a1bf320f74d4MIT751 个星标收录于 2026年10月9日更新于 2026年10月9日仓库4周前更新

Systematic code review for Java with null safety, exception handling, concurrency, and performance checks. Use when user says "review code", "check this PR", "code review", or before merging changes.

AI 生成的概览

系统化的 Java 代码审查清单,涵盖空值安全、异常、并发、惯用法、资源、API 设计与性能。

功能
为 Java 代码提供结构化的审查清单,分为空值安全、异常处理、集合与流、并发、Java 惯用法、资源管理、API 设计和性能等九类。它规定了快速浏览、逐项检查、汇总的审查策略,并给出按严重程度(从严重到轻微)分组的 Markdown 输出格式,其中包含正面反馈。它还提供严重程度判定准则和令牌优化建议,例如通过 git diff 聚焦改动行。
适用场景
当用户要求审查代码、检查拉取请求或进行代码审查时使用,也适用于合并变更前或 Java 项目实现功能之后。
运行要求
无需脚本或特殊工具,仅为指令型技能。可选地引用 git diff 以聚焦改动行。

Java Code Review Skill

Systematic code review checklist for Java projects.

When to Use

  • User says "review this code" / "check this PR" / "code review"
  • Before merging a PR
  • After implementing a feature

Review Strategy

  1. Quick scan - Understand intent, identify scope
  2. Checklist pass - Go through each category below
  3. Summary - List findings by severity (Critical → Minor)

Output Format

markdown
## Code Review: [file/feature name]
### Critical- [Issue description + line reference + suggestion]
### Improvements- [Suggestion + rationale]
### Minor/Style- [Nitpicks, optional improvements]
### Good Practices Observed- [Positive feedback - important for morale]

Review Checklist

1. Null Safety

Check for:

java
// ❌ NPE riskString name = user.getName().toUpperCase();
// ✅ SafeString name = Optional.ofNullable(user.getName())    .map(String::toUpperCase)    .orElse("");
// ✅ Also safe (early return)if (user.getName() == null) {    return "";}return user.getName().toUpperCase();

Flags:

  • Chained method calls without null checks
  • Missing @Nullable / @NonNull annotations on public APIs
  • Optional.get() without isPresent() check
  • Returning null from methods that could return Optional or empty collection

Suggest:

  • Use Optional for return types that may be absent
  • Use Objects.requireNonNull() for constructor/method params
  • Return empty collections instead of null: Collections.emptyList()

2. Exception Handling

Check for:

java
// ❌ Swallowing exceptionstry {    process();} catch (Exception e) {    // silently ignored}
// ❌ Catching too broadcatch (Exception e) { }catch (Throwable t) { }
// ❌ Losing stack tracecatch (IOException e) {    throw new RuntimeException(e.getMessage());}
// ✅ Proper handlingcatch (IOException e) {    log.error("Failed to process file: {}", filename, e);    throw new ProcessingException("File processing failed", e);}

Flags:

  • Empty catch blocks
  • Catching Exception or Throwable broadly
  • Losing original exception (not chaining)
  • Using exceptions for flow control
  • Checked exceptions leaking through API boundaries

Suggest:

  • Log with context AND stack trace
  • Use specific exception types
  • Chain exceptions with cause
  • Consider custom exceptions for domain errors

3. Collections & Streams

Check for:

java
// ❌ Modifying while iteratingfor (Item item : items) {    if (item.isExpired()) {        items.remove(item);  // ConcurrentModificationException    }}
// ✅ Use removeIfitems.removeIf(Item::isExpired);
// ❌ Stream for simple operationslist.stream().forEach(System.out::println);
// ✅ Simple loop is cleanerfor (Item item : list) {    System.out.println(item);}
// ❌ Collecting to modifyList<String> names = users.stream()    .map(User::getName)    .collect(Collectors.toList());names.add("extra");  // Might be immutable!
// ✅ Explicit mutable listList<String> names = users.stream()    .map(User::getName)    .collect(Collectors.toCollection(ArrayList::new));

Flags:

  • Modifying collections during iteration
  • Overusing streams for simple operations
  • Assuming Collectors.toList() returns mutable list
  • Not using List.of(), Set.of(), Map.of() for immutable collections
  • Parallel streams without understanding implications

Suggest:

  • List.copyOf() for defensive copies
  • removeIf() instead of iterator removal
  • Streams for transformations, loops for side effects

4. Concurrency

Check for:

java
// ❌ Not thread-safeprivate Map<String, User> cache = new HashMap<>();
// ✅ Thread-safeprivate Map<String, User> cache = new ConcurrentHashMap<>();
// ❌ Check-then-act race conditionif (!map.containsKey(key)) {    map.put(key, computeValue());}
// ✅ Atomic operationmap.computeIfAbsent(key, k -> computeValue());
// ❌ Double-checked locking (broken without volatile)if (instance == null) {    synchronized(this) {        if (instance == null) {            instance = new Instance();        }    }}

Flags:

  • Shared mutable state without synchronization
  • Check-then-act patterns without atomicity
  • Missing volatile on shared variables
  • Synchronized on non-final objects
  • Thread-unsafe lazy initialization

Suggest:

  • Prefer immutable objects
  • Use java.util.concurrent classes
  • AtomicReference, AtomicInteger for simple cases
  • Consider @ThreadSafe / @NotThreadSafe annotations

5. Java Idioms

equals/hashCode:

java
// ❌ Only equals without hashCode@Overridepublic boolean equals(Object o) { ... }// Missing hashCode!
// ❌ Mutable fields in hashCode@Overridepublic int hashCode() {    return Objects.hash(id, mutableField);  // Breaks HashMap}
// ✅ Use immutable fields, implement both@Overridepublic boolean equals(Object o) {    if (this == o) return true;    if (!(o instanceof User user)) return false;    return Objects.equals(id, user.id);}
@Overridepublic int hashCode() {    return Objects.hash(id);}

toString:

java
// ❌ Missing - hard to debug// No toString()
// ❌ Including sensitive datareturn "User{password='" + password + "'}";
// ✅ Useful for debugging@Overridepublic String toString() {    return "User{id=" + id + ", name='" + name + "'}";}

Builders:

java
// ✅ For classes with many optional parametersUser user = User.builder()    .name("John")    .email("[email protected]")    .build();

Flags:

  • equals without hashCode
  • Mutable fields in hashCode
  • Missing toString on domain objects
  • Constructors with > 3-4 parameters (suggest builder)
  • Not using instanceof pattern matching (Java 16+)

6. Resource Management

Check for:

java
// ❌ Resource leakFileInputStream fis = new FileInputStream(file);// ... might throw before close
// ✅ Try-with-resourcestry (FileInputStream fis = new FileInputStream(file)) {    // ...}
// ❌ Multiple resources, wrong ordertry (BufferedWriter writer = new BufferedWriter(new FileWriter(file))) {    // FileWriter might not be closed if BufferedWriter fails}
// ✅ Separate declarationstry (FileWriter fw = new FileWriter(file);     BufferedWriter writer = new BufferedWriter(fw)) {    // Both properly closed}

Flags:

  • Not using try-with-resources for Closeable/AutoCloseable
  • Resources opened but not in try-with-resources
  • Database connections/statements not properly closed

7. API Design

Check for:

java
// ❌ Boolean parametersprocess(data, true, false);  // What do these mean?
// ✅ Use enums or builderprocess(data, ProcessMode.ASYNC, ErrorHandling.STRICT);
// ❌ Returning null for "not found"public User findById(Long id) {    return users.get(id);  // null if not found}
// ✅ Return Optionalpublic Optional<User> findById(Long id) {    return Optional.ofNullable(users.get(id));}
// ❌ Accepting null collectionspublic void process(List<Item> items) {    if (items == null) items = Collections.emptyList();}
// ✅ Require non-null, accept emptypublic void process(List<Item> items) {    Objects.requireNonNull(items, "items must not be null");}

Flags:

  • Boolean parameters (prefer enums)
  • Methods with > 3 parameters (consider parameter object)
  • Inconsistent null handling across similar methods
  • Missing validation on public API inputs

8. Performance Considerations

Check for:

java
// ❌ String concatenation in loopString result = "";for (String s : strings) {    result += s;  // Creates new String each iteration}
// ✅ StringBuilderStringBuilder sb = new StringBuilder();for (String s : strings) {    sb.append(s);}
// ❌ Regex compilation in loopfor (String line : lines) {    if (line.matches("pattern.*")) { }  // Compiles regex each time}
// ✅ Pre-compiled patternprivate static final Pattern PATTERN = Pattern.compile("pattern.*");for (String line : lines) {    if (PATTERN.matcher(line).matches()) { }}
// ❌ N+1 in loopsfor (User user : users) {    List<Order> orders = orderRepo.findByUserId(user.getId());}
// ✅ Batch fetchMap<Long, List<Order>> ordersByUser = orderRepo.findByUserIds(userIds);

Flags:

  • String concatenation in loops
  • Regex compilation in loops
  • N+1 query patterns
  • Creating objects in tight loops that could be reused
  • Not using primitive streams (IntStream, LongStream)

9. Testing Hints

Suggest tests for:

  • Null inputs
  • Empty collections
  • Boundary values
  • Exception cases
  • Concurrent access (if applicable)

Severity Guidelines

SeverityCriteria
CriticalSecurity vulnerability, data loss risk, production crash
HighBug likely, significant performance issue, breaks API contract
MediumCode smell, maintainability issue, missing best practice
LowStyle, minor optimization, suggestion

Token Optimization

  • Focus on changed lines (use git diff)
  • Don't repeat obvious issues - group similar findings
  • Reference line numbers, not full code quotes
  • Skip files that are auto-generated or test fixtures

Quick Reference Card

CategoryKey Checks
Null SafetyChained calls, Optional misuse, null returns
ExceptionsEmpty catch, broad catch, lost stack trace
CollectionsModification during iteration, stream vs loop
ConcurrencyShared mutable state, check-then-act
Idiomsequals/hashCode pair, toString, builders
Resourcestry-with-resources, connection leaks
APIBoolean params, null handling, validation
PerformanceString concat, regex in loop, N+1

来源与署名

来源:decebals/claude-code-java位于skills/java-code-review提交0d98fe9

许可证: MIT

内容归原作者所有。SourceWeft 从公开仓库中收录这些内容。

举报或申请下架