Skip to content

Refactor JournalAbbreviationRepository class to use on-disk maps #12606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Remove cache size and unused variable

b9ba783
Select commit
Loading
Failed to load commit list.
Draft

Refactor JournalAbbreviationRepository class to use on-disk maps #12606

Remove cache size and unused variable
b9ba783
Select commit
Loading
Failed to load commit list.
Trag bot / Trag Review succeeded Mar 17, 2025 in 2m 5s

Trag Code Review

Reviewed files details

Details

[2025-03-17T20:46:33.054Z] code review started
[2025-03-17T20:46:33.055Z] owner: JabRef
[2025-03-17T20:46:33.055Z] repo: jabref
[2025-03-17T20:46:33.055Z] repoUrl: https://github.com/JabRef/jabref
[2025-03-17T20:46:33.055Z] author: marwanemad07
[2025-03-17T20:46:33.055Z] listing pull request files
[2025-03-17T20:46:33.432Z] file count: 3
[2025-03-17T20:46:34.175Z] pro user privilege applied
[2025-03-17T20:46:34.175Z] getting project rules
[2025-03-17T20:46:38.874Z] model: claude-3-5-sonnet-20240620
[2025-03-17T20:46:38.874Z] on rule review mode: true
[2025-03-17T20:46:38.874Z] glob ignore:
[2025-03-17T20:46:38.874Z] pull number: 12606
[2025-03-17T20:46:38.874Z] projectId: 885262de-6ea4-406f-ab33-0a18e077aaf9
[2025-03-17T20:46:52.875Z] file: src/main/java/org/jabref/cli/JournalListMvGenerator.java
[2025-03-17T20:46:52.875Z] reading file blob
[2025-03-17T20:47:08.874Z] 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[] in JUnit tests, 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()
  1. Instead of Files.createTempDirectory @TempDir JUnit5 annotation should be used.
  2. Do not catch the general java java.lang.Exception. Catch specific exeptions only
  3. Avoid exclamation marks at the end of a sentence. They are more for screaming. Use a dot to end the sentence.
  4. All labels and texts should be sentence case (and not title case)
  5. 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.
  6. Use "BibTeX" as spelling for bibtex
  7. New strings should be consistent to other strings. They should also be grouped semantically together.
  8. Existings strings should be reused instead of introducing slightly different strings
  9. Comments on/above methods should be JavaDoc.not simple Java comments //. Three /// are OK (because this is Java23 and later)
  10. The CHANGELOG.md entry should be for end users (and not programmers).
  11. User dialogs should have proper button labels: NOT yes/no/cancel, but indicating the action which happens when pressing the button
  12. 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.
  13. null should never be passed to a method (except it has the same name).
  14. Do not add extra blank lines in CHANGELOG.md
  15. Remove commented code. (To keep a history of changes git was made for.)
  16. Do not use Objects.requireNonNull. Use JSpecify @NonNull annotation if needs be.
  17. Never throw unchecked exceptions (e.g., throw new RuntimeException)
  18. 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
  1. Code should not be reformatted only because of syntax. There need to be new statements added if reformatting.
  2. 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());
    }
  1. 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()))
        );
  1. No "new Thread()", use "org.jabref.logic.util.BackgroundTask" and its "executeWith"
  2. try blocks shoud cover as less statements as possible (and not whole methods)
  3. use "throws Exception" in the method declaration instead of try-catch and fail/log/...
  4. When creating a new BibEntry object, instead of setField, withField methods should be used.
  5. 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)
  6. 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));
  1. If the java.util.Optional is really present, use use get() (and not orElse(""))
  2. Use Java Text blocks (""") for multiline string constants
  3. 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);
[2025-03-17T20:47:21.448Z] filtering out non-relevant issues
[2025-03-17T20:47:21.449Z] broad list of issues found
⚠️ [2025-03-17T20:47:21.449Z] rule violated
[2025-03-17T20:47:21.449Z] score: 7
⚠️ [2025-03-17T20:47:21.449Z] this log will only exist in github checkrun logs
[2025-03-17T20:47:21.449Z] Reason: Multiple map operations are performed without checking for null values from getter methods. This could lead to NullPointerException if any of the getter methods return null.
[2025-03-17T20:47:21.449Z] filtering out low importance issues
[2025-03-17T20:47:21.449Z] --------------------------------
[2025-03-17T20:47:21.449Z] writing comments to pr...
[2025-03-17T20:47:21.449Z] file: src/main/java/org/jabref/logic/journals/Abbreviation.java
[2025-03-17T20:47:21.449Z] reading file blob
[2025-03-17T20:47:32.444Z] 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[] in JUnit tests, 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()
  1. Instead of Files.createTempDirectory @TempDir JUnit5 annotation should be used.
  2. Do not catch the general java java.lang.Exception. Catch specific exeptions only
  3. Avoid exclamation marks at the end of a sentence. They are more for screaming. Use a dot to end the sentence.
  4. All labels and texts should be sentence case (and not title case)
  5. 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.
  6. Use "BibTeX" as spelling for bibtex
  7. New strings should be consistent to other strings. They should also be grouped semantically together.
  8. Existings strings should be reused instead of introducing slightly different strings
  9. Comments on/above methods should be JavaDoc.not simple Java comments //. Three /// are OK (because this is Java23 and later)
  10. The CHANGELOG.md entry should be for end users (and not programmers).
  11. User dialogs should have proper button labels: NOT yes/no/cancel, but indicating the action which happens when pressing the button
  12. 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.
  13. null should never be passed to a method (except it has the same name).
  14. Do not add extra blank lines in CHANGELOG.md
  15. Remove commented code. (To keep a history of changes git was made for.)
  16. Do not use Objects.requireNonNull. Use JSpecify @NonNull annotation if needs be.
  17. Never throw unchecked exceptions (e.g., throw new RuntimeException)
  18. 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
  1. Code should not be reformatted only because of syntax. There need to be new statements added if reformatting.
  2. 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());
    }
  1. 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()))
        );
  1. No "new Thread()", use "org.jabref.logic.util.BackgroundTask" and its "executeWith"
  2. try blocks shoud cover as less statements as possible (and not whole methods)
  3. use "throws Exception" in the method declaration instead of try-catch and fail/log/...
  4. When creating a new BibEntry object, instead of setField, withField methods should be used.
  5. 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)
  6. 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));
  1. If the java.util.Optional is really present, use use get() (and not orElse(""))
  2. Use Java Text blocks (""") for multiline string constants
  3. 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);
[2025-03-17T20:47:39.275Z] filtering out non-relevant issues
[2025-03-17T20:47:39.475Z] broad list of issues found
[2025-03-17T20:47:39.475Z] filtering out low importance issues
[2025-03-17T20:47:39.475Z] --------------------------------
[2025-03-17T20:47:39.475Z] writing comments to pr...
[2025-03-17T20:47:39.475Z] file: src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java
[2025-03-17T20:47:39.475Z] reading file blob
[2025-03-17T20:47:58.474Z] 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[] in JUnit tests, 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()
  1. Instead of Files.createTempDirectory @TempDir JUnit5 annotation should be used.
  2. Do not catch the general java java.lang.Exception. Catch specific exeptions only
  3. Avoid exclamation marks at the end of a sentence. They are more for screaming. Use a dot to end the sentence.
  4. All labels and texts should be sentence case (and not title case)
  5. 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.
  6. Use "BibTeX" as spelling for bibtex
  7. New strings should be consistent to other strings. They should also be grouped semantically together.
  8. Existings strings should be reused instead of introducing slightly different strings
  9. Comments on/above methods should be JavaDoc.not simple Java comments //. Three /// are OK (because this is Java23 and later)
  10. The CHANGELOG.md entry should be for end users (and not programmers).
  11. User dialogs should have proper button labels: NOT yes/no/cancel, but indicating the action which happens when pressing the button
  12. 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.
  13. null should never be passed to a method (except it has the same name).
  14. Do not add extra blank lines in CHANGELOG.md
  15. Remove commented code. (To keep a history of changes git was made for.)
  16. Do not use Objects.requireNonNull. Use JSpecify @NonNull annotation if needs be.
  17. Never throw unchecked exceptions (e.g., throw new RuntimeException)
  18. 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
  1. Code should not be reformatted only because of syntax. There need to be new statements added if reformatting.
  2. 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());
    }
  1. 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()))
        );
  1. No "new Thread()", use "org.jabref.logic.util.BackgroundTask" and its "executeWith"
  2. try blocks shoud cover as less statements as possible (and not whole methods)
  3. use "throws Exception" in the method declaration instead of try-catch and fail/log/...
  4. When creating a new BibEntry object, instead of setField, withField methods should be used.
  5. 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)
  6. 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));
  1. If the java.util.Optional is really present, use use get() (and not orElse(""))
  2. Use Java Text blocks (""") for multiline string constants
  3. 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);
[2025-03-17T20:48:11.841Z] filtering out non-relevant issues
[2025-03-17T20:48:12.175Z] broad list of issues found
[2025-03-17T20:48:12.378Z] score: 8
⚠️ [2025-03-17T20:48:12.378Z] this log will only exist in github checkrun logs
[2025-03-17T20:48:12.378Z] Reason: The MVStore instance is not properly closed, which could lead to resource leaks. The class should implement AutoCloseable to ensure proper resource management.
[2025-03-17T20:48:12.378Z] score: 7
⚠️ [2025-03-17T20:48:12.378Z] this log will only exist in github checkrun logs
[2025-03-17T20:48:12.378Z] Reason: The MVStore is opened without any error handling for file access issues or corruption. This could lead to runtime failures without proper error propagation.
[2025-03-17T20:48:12.378Z] score: 6
⚠️ [2025-03-17T20:48:12.378Z] this log will only exist in github checkrun logs
[2025-03-17T20:48:12.378Z] Reason: Constants are exposed as public for external use in JournalMvGenerator.java, which violates encapsulation. Should use proper accessor methods instead.
⚠️ [2025-03-17T20:48:12.378Z] rule violated
[2025-03-17T20:48:12.378Z] score: 5
⚠️ [2025-03-17T20:48:12.378Z] this log will only exist in github checkrun logs
[2025-03-17T20:48:12.378Z] Reason: Multiple nested null checks create unnecessary complexity. Should use Optional.ofNullable().or() chain to simplify the logic and improve readability.
[2025-03-17T20:48:12.378Z] filtering out low importance issues
[2025-03-17T20:48:12.378Z] --------------------------------
[2025-03-17T20:48:12.378Z] writing comments to pr...
[2025-03-17T20:48:12.378Z] files scanned: 3
[2025-03-17T20:48:12.379Z] lines scanned: 207
[2025-03-17T20:48:12.379Z] rules for project: 48
[2025-03-17T20:48:12.379Z] issues caught by your rules: 0

View more details on Trag bot