Skip to content

feat(code-reviewer): wire Java into analyzer + complete the universal/per-language refactor#749

Merged
alirezarezvani merged 3 commits into
devfrom
claude/pr-742-java-improvements
May 26, 2026
Merged

feat(code-reviewer): wire Java into analyzer + complete the universal/per-language refactor#749
alirezarezvani merged 3 commits into
devfrom
claude/pr-742-java-improvements

Conversation

@alirezarezvani

Copy link
Copy Markdown
Owner

Summary

Builds on @mitnick2012's code-reviewer restructure (#742) and finishes the two gaps that review surfaced, so this can merge in place of #742.

1. "Java support" is now real, not documentation-only. #742 advertised --language java and a .java dispatch row, but the scripts were untouched — code_quality_checker.py rejected java as an invalid --language choice and never discovered .java files. This PR wires Java in as a first-class deterministic language:

  • LANGUAGE_EXTENSIONS + function/class/method regex patterns for Java (so it no longer falls back to the Python patterns)
  • a check_java_specific_smells() detector (empty catch, printStackTrace, swallowed InterruptedException, unclosed AutoCloseable, per-call ObjectMapper/Gson) called from analyze_file
  • Java debug signals (System.out, printStackTrace) + @SuppressWarnings added to pr_analyzer.py

2. Completed the refactor / removed the drift. #742 moved the reference content into rules/universal.md + languages/*.md but left the old references/*.md in place (still linked from README) — duplicated content that would drift. This PR deletes the three now-duplicated reference files and repoints README + the C# clean fixture header at the new structure.

3. Java regression fixtures, mirroring the existing C# ones: assets/sample_java_smells.java + sample_java_clean.java with committed expected_outputs/*.json as a drift guard. The "Adding a New Language" guide now documents the optional analyzer-wiring + fixture steps, and a Regression Fixtures section is restored to SKILL.md.

Verification

  • --language java now valid; smells fixture → grade D with all 5 Java smells firing; clean fixture → grade A/100, no Java smells (singleton ObjectMapper correctly not flagged).
  • Committed expected-output JSON matches a fresh run for all 4 fixtures (C# + Java); existing C# fixtures unchanged (no regression).
  • All 3 scripts pass --help; pr_analyzer.py imports clean and the new Java regexes match.
  • No remaining repo links to the deleted references/; no dangling sync symlinks.

Credit

Restructure + languages/*.md + java.md authored by @mitnick2012 (#742). This branch adds the analyzer wiring, fixtures, and cleanup on top. Suggest merging this and closing #742 as superseded.

https://claude.ai/code/session_01DjuELpoFdFbFscr3kAatni


Generated by Claude Code

mitnick2012 and others added 3 commits May 25, 2026 13:15
- Extract commun languages rules in a separate rules/universal.md containing all cross-language rules in one place
- Move language-specific rules inline into each languages/*.md file,
  organised into consistent sections: Security / Async / Resource
  Management / Exception Handling / Performance / Idioms
- Add Java support: languages/java.md with full section coverage
- Every review now requires exactly 2 file reads: universal.md +
  one language file
- Add "Adding a new language" guide to SKILL.md: one file to create,
  nothing else changes
Builds on @mitnick2012's universal+per-language restructure (PR #742).

- Add Java as a first-class deterministic language in code_quality_checker.py
  (LANGUAGE_EXTENSIONS + function/class/method patterns + check_java_specific_smells),
  so the documented `--language java` command works instead of erroring on an
  invalid choice. Add Java debug + @SuppressWarnings signals to pr_analyzer.py.
- Add Java regression fixtures (sample_java_smells/clean.java) with committed
  expected_outputs JSON, mirroring the existing C# fixtures.
- Delete references/{code_review_checklist,coding_standards,common_antipatterns}.md,
  now duplicated by rules/universal.md + languages/*.md; repoint README and the
  C# clean fixture header at the new structure.
- Document the optional analyzer-wiring + fixture steps in the "Adding a New
  Language" guide and restore a Regression Fixtures section in SKILL.md.

https://claude.ai/code/session_01DjuELpoFdFbFscr3kAatni
@alirezarezvani alirezarezvani marked this pull request as ready for review May 26, 2026 14:04
@alirezarezvani alirezarezvani merged commit 17fe849 into dev May 26, 2026
8 checks passed
@alirezarezvani alirezarezvani deleted the claude/pr-742-java-improvements branch May 26, 2026 14:04
@claude

claude Bot commented May 26, 2026

Copy link
Copy Markdown

Code Review — PR #749

Overall: Solid, well-scoped PR. The Java wiring is real and follows the established C# pattern correctly. The documentation refactor is clean. A few issues worth fixing before merge.


Bugs / Correctness

1. Hardcoded absolute path in expected-output JSON breaks the regression harness

Both new expected-output files embed /home/user/claude-skills/... as the "file" field. Any user following the README/SKILL.md instructions will get a diff failure on the very first field — the regression test silently always fails. The C# fixtures have the same problem. The cleanest fix is to exclude the file field from the committed JSON entirely, since the interesting signal is whether smells and score are correct, not which path was passed.

2. False positives in resource_hint for multi-line try-with-resources

The negative lookahead ^(?!\s*try\b) only prevents matching lines that start with try. Multi-line try-with-resources blocks trigger a false positive on continuation lines:

try (InputStream in = new FileInputStream("a");
     OutputStream out = new FileOutputStream("b")) {  // flagged incorrectly

OutputStream out = ... does not start with try, so the detector fires. Suggest acknowledging this as a known limitation in a comment so future maintainers understand the false positive reports without a long investigation.

@claude

claude Bot commented May 26, 2026

Copy link
Copy Markdown

Minor Issues

3. _strip_csharp_comments called from check_java_specific_smells

The inline comment explains the call is safe (same // and /* */ syntax), but a reader encountering this the first time will be confused. Consider renaming to _strip_c_style_comments and updating the C# caller too — it is a shared utility.

4. README and SKILL.md use different diff commands

README uses the temp-file form (> /tmp/check.json && diff); SKILL.md uses the cleaner pipe form. Worth aligning — the pipe form is better and avoids a temp file.

5. Deleted references/code_review_checklist.md removes Pre-Review and Testing sections with no equivalent

The old checklist had "Pre-Review Checks", "Testing", "Documentation", and "Review Process Tips" sections that have no equivalent in the new rules/universal.md. If dropping the checklist format is intentional, a note in SKILL.md would help users who relied on those sections.


What Is Well Done

  • The Java function/class regex correctly mirrors the C# modifier-required approach, distinguishing declarations from invocations.
  • check_java_specific_smells covers the right five patterns. The static field exemption in the heavy-object check correctly avoids false positives on the clean fixture's singleton form.
  • All 7 language guides use an identical section structure (Security / Async / Resource Management / Exception Handling / Performance / Idioms), making them easy to scan and straightforward to extend.
  • The dispatch table in SKILL.md clearly communicates the two-file loading model.
  • Java fixtures are well-designed: every smell is labeled inline, and the clean fixture explicitly demonstrates each fix.

Bottom line: Items 1 (hardcoded path in JSON) and 2 (multi-line TWR false positive) are worth fixing before merge. Items 3–5 are polish. The core changes are correct and well-executed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants