[2025-03-20T20:35:02.272Z] code review started
[2025-03-20T20:35:02.567Z] owner: JabRef
[2025-03-20T20:35:02.568Z] repo: jabref
[2025-03-20T20:35:02.568Z] repoUrl: https://github.com/JabRef/jabref
[2025-03-20T20:35:02.568Z] author: AashifAmeer
[2025-03-20T20:35:02.568Z] listing pull request files
[2025-03-20T20:35:12.269Z] total file count: 1
[2025-03-20T20:35:12.468Z] eligible file count: 1
[2025-03-20T20:35:14.547Z] pro user privilege applied
[2025-03-20T20:35:14.548Z] getting project rules
[2025-03-20T20:35:14.561Z] model: claude-3-5-sonnet-20240620
[2025-03-20T20:35:14.561Z] on rule review mode: true
[2025-03-20T20:35:14.561Z] glob ignore:
[2025-03-20T20:35:14.561Z] pull number: 12653
[2025-03-20T20:35:14.561Z] projectId: 885262de-6ea4-406f-ab33-0a18e077aaf9
[2025-03-20T20:35:19.667Z] Found 0 existing review comments
[2025-03-20T20:35:19.668Z] file: src/main/java/org/jabref/gui/frame/JabRefFrame.java
[2025-03-20T20:35:19.668Z] reading file blob
[2025-03-20T20:35:27.268Z] filteredRules: 1. If a method has JavaDoc and code of the method has changed, the JavaDoc has to be updated accordingly. No need to add JavaDoc for "trivial" exceptions
2. If code in org.jabref.model or org.jabref.logic has been changed, tests need to be adapted or updated accordingly
3. The code should follow the fail fast principle by immediately handling invalid states and returning early instead of nesting logic inside else branches.

Example:

Bad:

if (path.isEmpty()) {
return false;
} else {
// other code
}

Good:

if (path.isEmpty()) {
return false;
}

// other code
4. Assertion statements must not include the message parameter - the method name should already convey the expected behavior.
5. The pull request title should contain a short title of the issue fixed (or what the PR adresses) and not just "Fix issue xyz"
6. The "Mandatory checks" are Markdown TODOs. They should be formatted as that. Wrong: - [ x]. Either - [ ] or - [x].
7. New methods (and new classes) should follow the Single-responsibility principle (SRP).
8. There should be JavaDoc for complex methods.
9. "Magic" numbers or strings should be constants - or at least have a Java comment. Exception: JavaFX height and widths.
10. Avoid code duplication
11. Use modern Java best practices, such as Arguments.of() instead of new Object[] especially in JUnit tests or Path.of() instead of Paths.get(), to improve readability and maintainability.
12. Exceptions should be used for exceptional states - not for normal control flow
13. Follow the principles of "Effective Java"
14. In JabRef, localized strings are done using Localization.lang("string"). More information at https://devdocs.jabref.org/code-howtos/localization.html.
15. No use of Java SWING, only JavaFX is allowed as UI technology
16. @DisplayName for tests should only be used if absolutely necessary: The method name itself should be comprehensive enough.
17. Comments should add new information (e.g. reasoning of the code). It should not be plainly derived fro the code itself.

Example for trivail comments:

    // Commit the staged changes
    RevCommit commit = git.commit()

18. Instead of Files.createTempDirectory @TempDir JUnit5 annotation should be used.
19. Do not catch the general java java.lang.Exception. Catch specific exeptions only
20. Avoid exclamation marks at the end of a sentence. They are more for screaming. Use a dot to end the sentence.
21. All labels and texts should be sentence case (and not title case)
22. New public methods should not return null. They should make use of java.util.Optional. In case null really needs to be used, the JSpecify annotations must be used.
23. Use "BibTeX" as spelling for bibtex
24. New strings should be consistent to other strings. They should also be grouped semantically together.
25. Existings strings should be reused instead of introducing slightly different strings
26. Comments on/above methods should be JavaDoc.not simple Java comments //. Three /// are OK (because this is Java23 and later)
27. The CHANGELOG.md entry should be for end users (and not programmers).
28. User dialogs should have proper button labels: NOT yes/no/cancel, but indicating the action which happens when pressing the button
29. GUI code should only be a gateway to code in org.jabref.logic. More complex code regarding non-GUI operations should go into org.jabref.logic. Think of layerd archicture.
30. null should never be passed to a method (except it has the same name).
31. Do not add extra blank lines in CHANGELOG.md
32. Remove commented code. (To keep a history of changes git was made for.)
33. Do not use Objects.requireNonNull. Use JSpecify @NonNull annotation if needs be.
34. Do not throw unchecked exceptions (e.g., do not throw new RuntimeException, do not throw new IllegalStateException)

Reason: This tears down the whole application. One does not want to loose data only because "a corner" of the application broke.
35. When adding JavaDoc, the text should be non-trivial.

Example for trivial JavaDoc:

 * @param backupDir the backup directory
 * @param dbfile the database file
 * @throws IOException     if an I/O error occurs
 * @throws GitAPIException if a Git API error occurs

36. Code should not be reformatted only because of syntax. There need to be new statements added if reformatting.
37. If @TempDir is used, there is no need to clean it up

Example for wrong code:

    @AfterEach
    void tearDown() throws IOException {
        FileUtils.cleanDirectory(tempDir.toFile());
    }

38. Assert the contents of objects (assertEquals), not checking for some Boolean conditions (assertTrue/assertFalse)

Example for wrong code:

        assertTrue(
                entry.getFiles().stream()
                     .anyMatch(file -> file.getLink().equals(newFile.getFileName().toString()) ||
                             file.getLink().endsWith("/" + newFile.getFileName().toString()))
        );

39. No "new Thread()", use "org.jabref.logic.util.BackgroundTask" and its "executeWith"
40. try blocks shoud cover as less statements as possible (and not whole methods)
41. use "throws Exception" in the method declaration instead of try-catch and fail/log/...
42. When creating a new BibEntry object, instead of setField, withField methods should be used.
43. In case Java comments are added, they should match the code following. They should be a high-level summary or guidance of the following code (and not some reandom text or just re-stating the obvious)
44. Use the methods of java.util.Optional. ifPresent.

NOT

Optional<String> resolved = bibEntry.getResolvedFieldOrAlias(...);
String value = resolved.orElse("");
doSomething(value)

Following is fine:

Optional<String> resolved = bibEntry.getResolvedFieldOrAlias(...);
resolved.ifPresent(value -> doSomething(value));

45. If the java.util.Optional is really present, use use get() (and not orElse(""))
46. Use Java Text blocks (""") for multiline string constants
47. Use compiled patterns (Pattern.compile)

NOT: listOfNames.matches(".\s{2,}.")

BUT: private final static PATTERN = ... - and athen PATTERN.matches(...)
48. Use placeholders if variance is in localizaiton:

BAD: Localization.lang("Current JabRef version") + ": " + buildInfo.version);

GOOD: Localization.lang("Current JabRef version: %0", buildInfo.version);
49. Log exceptions using exception logging capabilities

BAD: LOGGER.info("Failed to push: ".concat(e.toString()));

GOOD: LOGGER.info("Failed to push", e);
50. Boolean method parameters (for public methods) should be avoided. Better create two distinct methods (which maybe call some private methods)
51. Use modern Java data structures

BAD: new HashSet<>(Arrays.asList(...))

GOOD: Sef.of(...)
52. Use logger conversion to log objects - do not do manual conversion. Especially, java.util.Optional can be logged directly, no need for get(), ,getOrElse(), ...

BAD:

        LOGGER.info(String.valueOf(path.get()));
        LOGGER.info("Not a git repository");

GOOD:

       LOGGER.info("Not a git repository", path);

53. Java 21 introduced SequencedCollection and SequencedSet interfaces. Use it instead of LinkedHashSet (where applicable)
    [2025-03-20T20:35:56.467Z] filtering out non-relevant issues
    [2025-03-20T20:35:56.569Z] broad list of issues found
    [2025-03-20T20:35:56.569Z] score: 6
    ⚠️ [2025-03-20T20:35:56.569Z] this log will only exist in github checkrun logs
    [2025-03-20T20:35:56.569Z] Reason: Using error level logging for an expected application state is inappropriate. This should be a debug or info level log since it's not an exceptional error condition.
    [2025-03-20T20:35:56.569Z] filtering out low importance issues
    [2025-03-20T20:35:56.569Z] writing comments to pr...
    [2025-03-20T20:35:56.570Z] files scanned: 1
    [2025-03-20T20:35:56.570Z] lines scanned: 21
    [2025-03-20T20:35:56.570Z] rules for project: 53
    [2025-03-20T20:35:56.570Z] issues caught by your rules: 0