Skip to content

Minor code cleanup#14816

Merged
calixtus merged 5 commits intomainfrom
subhramit-misc
Jan 8, 2026
Merged

Minor code cleanup#14816
calixtus merged 5 commits intomainfrom
subhramit-misc

Conversation

@subhramit
Copy link
Copy Markdown
Member

@subhramit subhramit commented Jan 6, 2026

User description

  • use modern java List.of() and Map.of() instead of Collections.emptlyList() and Collections.emptyMap()
  • declare types instead of using var

Steps to test

Mandatory checks


PR Type

Enhancement


Description

  • Replace Collections.emptyList() and Collections.emptyMap() with modern List.of() and Map.of()

  • Replace var keyword with explicit type declarations throughout codebase

  • Add missing import statements for explicit types

  • Improve code readability and maintainability


Diagram Walkthrough

flowchart LR
  A["Collections API<br/>emptyList/emptyMap"] -->|"Replace with"| B["Modern Java<br/>List.of/Map.of"]
  C["var keyword<br/>Type inference"] -->|"Replace with"| D["Explicit type<br/>declarations"]
  B --> E["Modernized<br/>codebase"]
  D --> E
Loading

File Walkthrough

Relevant files
Enhancement
12 files
PdfDocumentViewer.java
Replace var with explicit InputStream type                             
+2/-1     
JumpToFieldViewModel.java
Replace Collections.emptyList with List.of                             
+1/-2     
MultiSelectionMenuBuilder.java
Replace var with explicit type declarations                           
+5/-5     
SidePanePreferences.java
Replace Collections.emptyMap with Map.of                                 
+1/-2     
GroupNodeViewModel.java
Replace var with explicit Optional type                                   
+1/-1     
KeyBindingsRestartWarningTest.java
Replace Collections.emptyList with List.of                             
+2/-2     
AbstractJabKitTest.java
Replace var with explicit PrintStream types                           
+2/-2     
ParserResult.java
Replace Collections.emptyMap with Map.of                                 
+2/-3     
OpenAlex.java
Replace var with explicit ProgressInputStream type             
+2/-1     
AutomaticDateGroup.java
Replace var with explicit LinkedHashSet type                         
+1/-1     
EuropePmcFetcherTest.java
Replace var with explicit List type declaration                   
+2/-1     
LayoutEntryTest.java
Replace var with explicit List type declaration                   
+1/-1     

Signed-off-by: subhramit <subhramit.bb@live.in>
Signed-off-by: subhramit <subhramit.bb@live.in>
Signed-off-by: subhramit <subhramit.bb@live.in>
Signed-off-by: subhramit <subhramit.bb@live.in>
@subhramit subhramit requested a review from koppor January 6, 2026 17:28
@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

qodo-free-for-open-source-projects Bot commented Jan 6, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

qodo-free-for-open-source-projects Bot commented Jan 6, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Re-evaluate the removal of var

Reconsider removing the var keyword. This change may contradict the goal of
using modern Java features and could decrease code readability by increasing
verbosity.

Examples:

jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/MultiSelectionMenuBuilder.java [93-95]
                List<LinkedFileViewModel> localLinkedFiles = selection.stream()
                                                       .filter(MultiSelectionMenuBuilder.this::isLocalAndExists)
                                                       .toList();
jabgui/src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java [611]
                Optional<GroupTreeNode> parent = node.getParent();

Solution Walkthrough:

Before:

// jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/MultiSelectionMenuBuilder.java
public void execute() {
    List<LinkedFileViewModel> localLinkedFiles = selection.stream()
                                                   .filter(MultiSelectionMenuBuilder.this::isLocalAndExists)
                                                   .toList();
    // ...
    Optional<Path> exportDirectory = dialogService.showDirectorySelectionDialog(directoryDialogConfiguration);
    // ...
    for (LinkedFileViewModel linkedFileViewModel : localLinkedFiles) {
        Optional<Path> sourcePath = linkedFileViewModel.getFile().findIn(databaseContext, preferences.getFilePreferences());
        // ...
    }
}

After:

// jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/MultiSelectionMenuBuilder.java
public void execute() {
    var localLinkedFiles = selection.stream()
                                    .filter(MultiSelectionMenuBuilder.this::isLocalAndExists)
                                    .toList();
    // ...
    var exportDirectory = dialogService.showDirectorySelectionDialog(directoryDialogConfiguration);
    // ...
    for (LinkedFileViewModel linkedFileViewModel : localLinkedFiles) {
        var sourcePath = linkedFileViewModel.getFile().findIn(databaseContext, preferences.getFilePreferences());
        // ...
    }
}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly questions a major, codebase-wide change in the PR, pointing out that removing the modern var keyword contradicts the stated goal of modernization and may reduce readability.

Medium
General
Refactor switch statement to expression

Refactor the switch statement in the createSubgroups method to use a more
concise switch expression.

jablib/src/main/java/org/jabref/model/groups/AutomaticDateGroup.java [48-71]

 @Override
 public Set<GroupTreeNode> createSubgroups(BibEntry entry) {
-    LinkedHashSet<GroupTreeNode> out = new LinkedHashSet<GroupTreeNode>();
+    LinkedHashSet<GroupTreeNode> out = new LinkedHashSet<>();
 
     DateGroup.extractDate(field, entry).ifPresent(date -> {
-        switch (granularity) {
-            case YEAR -> {
-                date.getYear().ifPresent(year -> {
-                    String key = "%04d".formatted(year);
-                    out.add(new GroupTreeNode(new DateGroup(key, GroupHierarchyType.INDEPENDENT, field, key)));
-                });
-            }
-            case MONTH ->
-                    DateGroup.getDateKey(date, "YYYY-MM").ifPresent(key -> {
-                        out.add(new GroupTreeNode(new DateGroup(key, GroupHierarchyType.INDEPENDENT, field, key)));
-                    });
-            case FULL_DATE ->
-                    DateGroup.getDateKey(date, "YYYY-MM-DD").ifPresent(key -> {
-                        out.add(new GroupTreeNode(new DateGroup(key, GroupHierarchyType.INDEPENDENT, field, key)));
-                    });
-        }
+        Optional<String> key = switch (granularity) {
+            case YEAR -> date.getYear().map("%04d"::formatted);
+            case MONTH -> DateGroup.getDateKey(date, "YYYY-MM");
+            case FULL_DATE -> DateGroup.getDateKey(date, "YYYY-MM-DD");
+        };
+        key.ifPresent(k -> out.add(new GroupTreeNode(new DateGroup(k, GroupHierarchyType.INDEPENDENT, field, k))));
     });
     return out;
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly proposes refactoring the switch statement to a more modern and concise switch expression, which improves code readability and maintainability.

Low
  • Update

Signed-off-by: subhramit <subhramit.bb@live.in>
@subhramit subhramit added the dev: code-quality Issues related to code or architecture decisions label Jan 6, 2026
public List<String> getFieldNames() {
if (entryEditor.getCurrentlyEditedEntry() == null) {
return Collections.emptyList();
return List.of();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collections.emptyLIst returns an immutable list

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does list.of I think

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot_2026-01-07-08-05-44-91_3aea4af51f236e4932235fdada7d1643

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this(
EnumSet.of(SidePaneType.WEB_SEARCH, SidePaneType.GROUPS), // Default visible panes (OPEN_OFFICE omitted)
Collections.emptyMap(), // Default preferred positions
Map.of(), // Default preferred positions
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, Collections.emptyMap is better

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above... ;-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot_2026-01-07-08-07-25-67_3aea4af51f236e4932235fdada7d1643

Copy link
Copy Markdown
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collections.empty... better transfers the semantic that we expect an immutable list

@github-actions github-actions Bot added the status: changes-required Pull requests that are not yet complete label Jan 6, 2026
@subhramit
Copy link
Copy Markdown
Member Author

Collections.empty... better transfers the semantic that we expect an immutable list

It was just to maintain consistency with the rest of the places where we return empty lists (whether in code or tests)
I used to think the mutable vs immutable distinction existed only in thee case of Collectiors.toList and just toList. I should have avoided the initial refactoring which changed the other empty lists and maps couple of months back

@calixtus calixtus enabled auto-merge January 7, 2026 04:02
@koppor
Copy link
Copy Markdown
Member

koppor commented Jan 8, 2026

Collections.empty... better transfers the semantic that we expect an immutable list

Yes - also immutable - https://apidia.net/java/OpenJDK/25/?pck=java.util&cls=.Collections


As usual, I point to Eclipse Collections (https://eclipse.dev/collections/), which have the mutability and immutability explicitly.

@calixtus calixtus added this pull request to the merge queue Jan 8, 2026
Merged via the queue into main with commit 1ac8dee Jan 8, 2026
67 of 75 checks passed
@calixtus calixtus deleted the subhramit-misc branch January 8, 2026 14:49
@subhramit
Copy link
Copy Markdown
Member Author

subhramit commented Jan 8, 2026

As usual, I point to Eclipse Collections (https://eclipse.dev/collections/), which have the mutability and immutability explicitly.

This is beautiful.
If they had the same interface apis as vanilla collections, one could migrate a large codebase easily. Unfortunately they seem not to.

// Java Streams
list.stream()
    .filter(x -> x > 5)
    .map(x -> x * 2)
    .collect(Collectors.toList());

// Eclipse Collections
list.select(x -> x > 5)
    .collect(x -> x * 2);

@Siedlerchr
Copy link
Copy Markdown
Member

The main branch is failing now

@subhramit
Copy link
Copy Markdown
Member Author

The main branch is failing now

yeah it is unrelated:

Execution failed for task ':jablib:downloadLtwaFile'.
> A failure occurred while executing de.undercouch.gradle.tasks.download.internal.DefaultWorkerExecutorHelper$DefaultWorkAction
   > de.undercouch.gradle.tasks.download.org.apache.hc.client5.http.ConnectTimeoutException: Connect to [https://www.issn.org:443](https://www.issn.org/) [www.issn.org/46.105.152.159] failed: Connect timed out

@koppor
Copy link
Copy Markdown
Member

koppor commented Jan 24, 2026

For reference: An LTWA workaround was added at #14892

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

Labels

dev: code-quality Issues related to code or architecture decisions Review effort 2/5 status: changes-required Pull requests that are not yet complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants