Skip to content

.pr_agent_auto_best_practices

qodo-merge-bot edited this page May 24, 2026 · 7 revisions

Pattern 1: When working with Optional values, avoid isPresent()/isEmpty() checks followed by get(); instead unwrap once or use map/flatMap/filter/ifPresentOrElse/orElseThrow to keep control flow safe and idiomatic.

Example code before:

Optional<Path> path = findPath();
if (path.isEmpty()) {
    return;
}
doWork(path.get());

Example code after:

findPath().ifPresent(this::doWork);
Relevant past accepted suggestions:
Suggestion 1: [maintainability] `CheckConsistency` uses Optional.get()
`CheckConsistency` uses Optional.get() The modified consistency- and integrity-check flows use `Optional.isEmpty()` followed by `Optional.get()`, which is discouraged by the Optional-control-flow compliance rule. This pattern is unnecessarily verbose and increases the risk of unsafe `get()` usage during future edits or refactors.

Issue description

New/modified code uses Optional.isEmpty() and then Optional.get(), violating the Optional-control-flow compliance rule.

Issue Context

PR Compliance ID 29 requires idiomatic Optional control flow (e.g., map, ifPresentOrElse, orElseThrow) instead of manual presence checks followed by get(), because the latter is verbose and can be accidentally made unsafe during later changes.

Fix Focus Areas

  • jabkit/src/main/java/org/jabref/toolkit/commands/CheckConsistency.java[53-83]
  • jabkit/src/main/java/org/jabref/toolkit/commands/CheckIntegrity.java[64-105]

Suggestion 2: [maintainability] `AGENTS.md` promotes `Optional.get()`
`AGENTS.md` promotes `Optional.get()` The updated guidance tells contributors to use `Optional.get()` when the value is present, which encourages unsafe and non-idiomatic Optional handling. This conflicts with the project’s requirement to use idiomatic Optional control flow (e.g., `ifPresent`, `ifPresentOrElse`, `map`, `orElseThrow`) rather than presence checks and `get()`.

Issue description

AGENTS.md currently advises using Optional.get() “if really present”, which is discouraged by the compliance rules favoring idiomatic Optional control flow.

Issue Context

This guidance is used to steer contributors/agents; recommending get() can lead to fragile code patterns and conflicts with the project’s Optional conventions.

Fix Focus Areas

  • AGENTS.md[164-165]

Suggestion 3: [maintainability] Optional `get()` after `isEmpty()`
Optional `get()` after `isEmpty()` `CSLCitationRanges` uses `if (range.isEmpty()) continue;` followed by multiple `range.get()` calls, which is non-idiomatic Optional control flow and risks future unsafe access if the code changes.

Issue description

New code uses Optional.isEmpty() and then calls Optional.get() multiple times. This is discouraged; prefer ifPresent, map, or unwrapping once to a local variable.

Issue Context

This occurs in the newly added CSLCitationRanges method while retrieving a reference mark anchor.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/openoffice/frontend/OOFrontend.java[302-312]

Suggestion 4: [maintainability] `pdfPath` uses `isPresent`+`get`
`pdfPath` uses `isPresent`+`get` The new OCR action uses `if (!pdfPath.isPresent())` followed by `pdfPath.get()`, which is the non-idiomatic and error-prone Optional pattern the project forbids. This reduces readability and can lead to unsafe refactors later.

Issue description

Optional is handled using isPresent() + get(), which violates the project's Optional usage rules.

Issue Context

In OcrLinkedFileAction.execute(), the code checks presence and then calls get() to pass the Path into the OCR task.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/linkedfile/OcrLinkedFileAction.java[51-56]

Suggestion 5: [maintainability] Optional `get()` after `isEmpty()`
Optional `get()` after `isEmpty()` The updated code checks `existingEntry.getCitationKey().isEmpty()` and then calls `existingEntry.getCitationKey().get()`, which is discouraged in favor of idiomatic Optional control flow. This pattern is less readable and easier to regress into unsafe access later.

Issue description

The code uses isEmpty() followed by get() on the same Optional, which violates the project's preferred Optional style.

Issue Context

Use idiomatic Optional control flow such as ifPresentOrElse, map, or orElseThrow (when appropriate) to keep the intent clear.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java[123-132]

Suggestion 6: [maintainability] `BibEntryMatchVisitor` uses `isPresent().get()`
`BibEntryMatchVisitor` uses `isPresent().get()` The new in-memory search visitor repeatedly uses `Optional.isPresent()` followed by `Optional.get()`, which is discouraged in favor of idiomatic Optional control flow. This reduces readability and increases the risk of future misuse.

Issue description

Optional values are handled via isPresent() + get() patterns, which the project compliance rules prohibit in new/modified code.

Issue Context

This occurs in the new BibEntryMatchVisitor in-memory matcher implementation.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/search/inmemory/BibEntryMatchVisitor.java[149-154]
  • jablib/src/main/java/org/jabref/logic/search/inmemory/BibEntryMatchVisitor.java[162-167] Suggested rewrite patterns:
  • value.map(v -> matchValue(v, term, matchKind, caseSensitive)).orElse(false)
  • or value.filter(v -> matchValue(v, term, matchKind, caseSensitive)).isPresent()

Suggestion 7: [maintainability] `Optional.get()` after `isEmpty()`
`Optional.get()` after `isEmpty()` New code uses `Optional.isEmpty()` checks followed by `Optional.get()`, which is discouraged and can lead to less readable and more error-prone control flow. The compliance rule requires using idiomatic Optional APIs (`map`/`filter`/`ifPresentOrElse`/`orElseThrow`) instead.

Issue description

BookCoverFetcher uses Optional.isEmpty() + Optional.get() in new code paths, which violates the project's Optional style guidelines.

Issue Context

The cooldown logic should use Optional idioms such as filter, map, ifPresentOrElse, or orElseThrow instead of presence checks + get().

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/importer/BookCoverFetcher.java[64-66]

Pattern 2: Ensure all user-facing strings (CLI output, UI/FXML labels, integrity/validation messages, notifications) are localized and use placeholder-based formatting for variable parts instead of string concatenation.

Example code before:

notification(Localization.lang("Could not call executable") + ": " + commandPath);

Example code after:

notification(Localization.lang("Could not call executable %0", commandPath));
Relevant past accepted suggestions:
Suggestion 1: [maintainability] Unlocalized `Check` console message
Unlocalized `Check` console message `Check.call()` prints a hardcoded user-facing message instead of using the project localization mechanism. This prevents translation and violates the localization standard for user-visible messages.

Issue description

A user-facing CLI message is printed as a hardcoded English string, bypassing JabRef localization.

Issue Context

Other jabkit commands print user-visible output via Localization.lang(...). The new check command should follow the same convention.

Fix Focus Areas

  • jabkit/src/main/java/org/jabref/toolkit/commands/Check.java[44-46]

Suggestion 2: [correctness] Missing OCR l10n keys
Missing OCR l10n keys New user-facing OCR UI strings are passed to `Localization.lang(...)` in `OcrLinkedFileAction`, but their translation keys are missing from `JabRef_en.properties`, so they cannot be translated. Because missing keys are silently accepted, this undermines localization completeness and consistency for the new OCR flow, including error paths.

Issue description

Two new user-facing OCR messages are looked up via Localization.lang(...) but are not present in JabRef_en.properties, preventing translation and allowing missing keys to slip through silently.

Issue Context

OcrLinkedFileAction shows an error dialog when no file is found and a notification on unexpected failure, both using new localization keys/strings that are not defined in the English resource bundle. JabRef’s localization bundle behavior returns the key itself for missing entries and does not reliably warn, making incomplete localization coverage for these OCR error paths easy to miss.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/linkedfile/OcrLinkedFileAction.java[50-88]
  • jablib/src/main/resources/l10n/JabRef_en.properties[797-808]
  • jablib/src/main/java/org/jabref/logic/l10n/Localization.java[130-168]

Suggestion 3: [maintainability] `Reset current template` unlocalized
`Reset current template` unlocalized The new/modified FXML introduces a hard-coded user-facing string (`Reset current template`) instead of using a localization key. This breaks localization and UI text consistency across languages.

Issue description

AiTab.fxml contains a hard-coded user-facing button label (Reset current template) instead of a %... localization key.

Issue Context

Project UI strings must be localized (FXML text values should be %key).

Fix Focus Areas

  • jabgui/src/main/resources/org/jabref/gui/preferences/ai/AiTab.fxml[345-347]

Suggestion 4: [maintainability] `commandPath` concatenated in message
`commandPath` concatenated in message The error notification message builds a localized string via concatenation around `commandPath` instead of using localization placeholders. This makes translations harder and violates the localization placeholder requirement.

Issue description

A localized message is concatenated with commandPath instead of using %0 placeholders.

Issue Context

Compliance requires placeholder-based localization for variable content.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/push/AbstractPushToApplication.java[108-111]

Suggestion 5: [correctness] Hardcoded FXML label `group`
Hardcoded FXML label `group` The new label text `group` is hardcoded in FXML and is not localized using the required `%...` resource key format. This breaks localization and violates the requirement that all user-facing UI text in FXML must be localized.

Issue description

A new user-facing label was added in FXML as hardcoded text (group) instead of using JabRef’s localization mechanism.

Issue Context

In JabRef FXML files, user-facing strings must be localized by using a resource key prefixed with %.

Fix Focus Areas

  • jabgui/src/main/resources/org/jabref/gui/importer/ImportEntriesDialog.fxml[47-47]

Suggestion 6: [maintainability] Brace errors not localized
Brace errors not localized `BracketChecker` returns raw English strings (from `FieldWriter.checkBalancedBraces`) that are shown to users during integrity checking instead of using `Localization.lang(...)`. This violates the requirement that all user-facing text be localized and can lead to inconsistent UX across locales.

Issue description

BracketChecker now surfaces non-localized, hardcoded English brace-balance errors to the UI.

Issue Context

Integrity-check messages are user-facing and must be localized via Localization.lang(...).

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/integrity/BracketChecker.java[11-15]
  • jablib/src/main/java/org/jabref/logic/bibtex/FieldWriter.java[56-75]

Suggestion 7: [correctness] Brace list in message
Brace list in message SourceTab.storeSource passes a List of brace errors directly to Localization.lang, which will be rendered via List.toString() (e.g., with brackets/commas) instead of a readable multiline message. This makes the validation message confusing when brace errors occur.

Issue description

SourceTab.storeSource constructs a List<String> of brace errors and passes that list directly into Localization.lang(...), causing the UI message to show Java list formatting (e.g., [err1, err2]) instead of a readable message.

Issue Context

This path is hit when FieldWriter.checkBalancedBraces(newValue) returns errors during source editing.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/entryeditor/SourceTab.java[339-350]

Expected fix

  • Convert the List<String> to a user-friendly string (e.g., String.join("\n", errors)) before passing to Localization.lang.
  • Keep the returned behavior (early return) unchanged, only fix message formatting.

Pattern 3: Under @NullMarked (or any explicit nullness regime), align runtime behavior, annotations, and documentation: if null is a valid value, mark it @Nullable in the type (including type-use for generics/properties); otherwise remove null-handling and enforce non-null preconditions.

Example code before:

@NullMarked
class Model {
    String parse(String raw) {
        if (raw == null || raw.isBlank()) { // null accepted, but signature says non-null
            return "";
        }
        return raw.trim();
    }
}

Example code after:

@NullMarked
class Model {
    String parse(@Nullable String raw) {
        if (raw == null || raw.isBlank()) {
            return "";
        }
        return raw.trim();
    }
}
Relevant past accepted suggestions:
Suggestion 1: [correctness] `render` bibDatabase nullability mismatch
`render` bibDatabase nullability mismatch `BstVM` is now `@NullMarked`, but `render(List, BibDatabase)` still documents `bibDatabase` as potentially null while its signature does not declare `@Nullable`. This misaligned null-safety contract can lead to incorrect usage (passing null) and runtime failures when null is assumed impossible downstream.

Issue description

BstVM is annotated @NullMarked, but render(List<BibEntry>, BibDatabase) still documents bibDatabase as “may be null” while the method signature does not declare it as @Nullable. This creates a mismatch between documented behavior and the nullness contract implied by @NullMarked.

Issue Context

The compliance rule requires overrides/public APIs to have consistent nullability contracts. If bibDatabase is truly optional, it must be declared @Nullable and handled safely (e.g., using a default). If it is required, the Javadoc must be updated to remove the “may be null” claim and call sites must not pass null.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/bst/BstVM.java[63-89]
  • jablib/src/main/java/org/jabref/logic/bst/BstFunctions.java[53-60]

Suggestion 2: [correctness] Preamble null database NPE
Preamble null database NPE BstFunctions now uses `Optional.of(bibDatabase)` for preamble extraction, which throws if `bibDatabase` is null, even though `BstVM.render(..., bibDatabase)` still documents that the database may be null. Calling `render(entries, null)` will now crash during VM setup.

Issue description

BstFunctions assumes bibDatabase is non-null (Optional.of(...)), but BstVM.render(..., bibDatabase) is documented as allowing null. This mismatch creates an easy-to-hit NPE for callers following the documentation.

Issue Context

Even if current in-repo call sites pass a non-null database, the public API contract still claims null is acceptable.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/bst/BstFunctions.java[53-60]
  • jablib/src/main/java/org/jabref/logic/bst/BstVM.java[63-75]

Suggested change (choose one)

  1. Preserve contract: switch back to Optional.ofNullable(bstVMContext.bibDatabase()) and annotate the render parameter/context field as @Nullable as needed.
  2. Tighten contract: update Javadoc to state bibDatabase must be non-null and add Objects.requireNonNull(bibDatabase) in render(...) (and keep Optional.of(...)).

Suggestion 3: [maintainability] NullMarked vs null containers
NullMarked vs null containers @NullMarked is applied to BST classes/records while core containers are still designed to store nulls (e.g., stack and entry field maps), but their generic types are not marked nullable. This makes the nullness contract misleading and can cause future changes to introduce NPEs by trusting non-null-by-default types.

Issue description

BST code is now annotated with @NullMarked, but multiple containers still intentionally store null values (stack elements, entry fields/local strings). The generic types currently do not express this, so nullness tooling (and future readers) will assume non-null where null is valid.

Issue Context

This PR’s goal is to reduce nullness warnings. To do that safely, nullability should be represented in the type system rather than relying on conventions.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/bst/BstVMContext.java[16-27]
  • jablib/src/main/java/org/jabref/logic/bst/BstVMVisitor.java[152-180]

Suggested fix

  • Update container types to reflect nullable elements/values where null is part of semantics, e.g.:
  • Deque<@Nullable Object> for the stack (if null stack entries are valid),
  • Map<String, @Nullable String> for maps that store null as “missing”.
  • Then adjust local variables accordingly (e.g., @Nullable String value = ...) to prevent future unsafe dereferences under @NullMarked.

Suggestion 4: [correctness] `lastImportedEntry` nullability mismatch
`lastImportedEntry` nullability mismatch `CitationsRelationsTabViewModel` is `@NullMarked` but `lastImportedEntry` is stored/observed as potentially null (`new SimpleObjectProperty<>()` and null checks), without marking the property type as nullable. This undermines null-safety guarantees and can mislead callers.

Issue description

In a @NullMarked class, lastImportedEntry is treated as nullable at runtime (default null, tests expect null, UI subscriber checks entry != null) but the property is typed as non-null (ObjectProperty<BibEntry> / ReadOnlyObjectProperty<BibEntry>). This is an inconsistent nullness contract.

Issue Context

JSpecify @NullMarked means types are non-null by default; nullable values should be expressed explicitly (e.g., @Nullable type use).

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java[35-36]
  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java[55-56]
  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java[208-210]
  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java[161-165]

Suggestion 5: [maintainability] Nullable override mismatch
Nullable override mismatch ValueChecker.checkValue is now declared with a @Nullable parameter, but implementors (e.g., BracketChecker) still declare a non-null parameter, violating the interface nullness contract and potentially generating NullAway override warnings across jablib. This undermines null-safety analysis quality and can drown real warnings in build output.

Issue description

ValueChecker#checkValue now explicitly allows @Nullable input, but implementing classes still declare checkValue(String value) (implicitly non-null under NullAway). This creates a nullness-contract mismatch and can trigger NullAway override warnings across all ValueChecker implementations.

Issue Context

Null can legitimately reach these validators via GUI bindings (field content may be absent), so the interface annotation is reasonable; the implementations should reflect the same nullability in their signatures.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/integrity/ValueChecker.java[5-13]
  • jablib/src/main/java/org/jabref/logic/integrity/BracketChecker.java[9-22]

What to change

  • Update BracketChecker (and all other ValueChecker implementors) to declare checkValue(@Nullable String value).
  • No logic changes are required where implementations already guard via StringUtil.isBlank/isNullOrEmpty; this is primarily to align the declared contract and keep NullAway output clean.

Suggestion 6: [correctness] `parse()` accepts null unannotated
`parse()` accepts null unannotated `MainTableColumnModel` is `@NullMarked`, but `parse` now explicitly handles `null` without marking the parameter as nullable, making the API nullability contract unclear/inconsistent. This can mislead callers and static analysis and hides null-safety issues behind a silent default.

Issue description

MainTableColumnModel is @NullMarked, but parse(String rawColumnName) now checks for null (rawColumnName == null || rawColumnName.isBlank()), which implicitly widens the API to accept null while keeping a non-null signature.

Issue Context

Under JSpecify @NullMarked, parameters are non-null by default. If null is a supported input, the parameter must be explicitly annotated @Nullable; otherwise the null-handling should be removed.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/maintable/MainTableColumnModel.java[233-236]

Pattern 4: Do not enforce required runtime preconditions via Java assert (often disabled in production); use explicit checks that either return a controlled failure (with notification/logging) or throw a deliberate, specific exception.

Example code before:

assert userMessage.isPresent();
handle(userMessage.get());

Example code after:

ChatMessage msg = userMessage.orElseThrow(
    () -> new IllegalStateException("No user message to answer"));
handle(msg);
Relevant past accepted suggestions:
Suggestion 1: [correctness] RAG assert can crash
RAG assert can crash GenerateRagResponseTask.call() relies on an `assert` to guarantee a last USER message exists, then unconditionally calls `userMessage.get()`, which throws when the history contains no USER message (asserts are typically disabled in production). This can crash AI reply generation.

Issue description

GenerateRagResponseTask.call() uses assert userMessage.isPresent() and then calls userMessage.get(). In production JVMs assertions are usually disabled, so an empty chat history (or a history without USER messages) will throw NoSuchElementException and fail the task.

Issue Context

ChatHistoryUtils.getLastUserMessage(...) explicitly returns Optional.empty() when no USER message exists.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/ai/chatting/tasks/GenerateRagResponseTask.java[47-57]
  • jablib/src/main/java/org/jabref/logic/ai/chatting/util/ChatHistoryUtils.java[74-85]

Suggested change

Replace the assert with explicit handling, e.g.:

  • ChatMessage user = ChatHistoryUtils.getLastUserMessage(...).orElseThrow(() -> new IllegalStateException("No user message to answer"));
  • or return a ChatMessage.errorMessage(...) / fail the task with a user-friendly error, depending on your task/UI contract.

Suggestion 2: [reliability] Preconditions enforced by assert
Preconditions enforced by assert `importEntries` uses Java `assert` statements for required runtime preconditions (active database present, non-empty import list), but assertions are typically disabled in production so these checks can be skipped. This can lead to silent misbehavior (e.g., using a new empty `BibDatabaseContext`) instead of a controlled failure path.

Issue description

Required preconditions are guarded with assert, which is commonly disabled at runtime. This can allow execution to continue in invalid states (notably orElse(new BibDatabaseContext())).

Issue Context

The import flow should either:

  • reliably no-op with a user-facing notification when prerequisites are missing, or
  • throw a clear exception if this state is truly impossible.

Fix Focus Areas

  • jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java[74-86]

Suggested fix

  • Replace assert stateManager.getActiveDatabase().isPresent() with an explicit check:
  • if absent: notify/log and return (or throw IllegalStateException).
  • Replace/keep the empty-list guard as a normal runtime check (not assert).
  • Remove orElse(new BibDatabaseContext()) fallback in favor of a guaranteed active context when importing.

Suggestion 3: [correctness] Cancelled task becomes success
Cancelled task becomes success TrackedBackgroundTask.call() unconditionally sets status SUCCESS after perform() returns and sets ERROR for any exception, so a cancelled task can end up reported as SUCCESS/ERROR instead of CANCELLED. Code that relies on CANCELLED (e.g., summarization regenerate path) can misbehave and the UI can display the wrong task state.

Issue description

TrackedBackgroundTask.cancel() sets status to CANCELLED, but call() later overwrites status to SUCCESS after perform() completes, and overwrites to ERROR for any exception (including interruption/cancellation). This breaks logic that checks for CANCELLED and can show incorrect status in UI.

Issue Context

SummarizationTaskAggregator.start() checks task.getStatus() == CANCELLED to decide whether it may start a new regeneration task.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/ai/util/TrackedBackgroundTask.java[38-80]
  • jablib/src/main/java/org/jabref/logic/ai/summarization/SummarizationTaskAggregator.java[53-56]

Suggested fix

In call(), before setting SUCCESS, check isCancelled() / Thread.currentThread().isInterrupted() and keep/set CANCELLED. In the catch block, treat interruption/cancellation distinctly (set CANCELLED rather than ERROR) and avoid overwriting an already-cancelled status.


Pattern 5: For regex usage in frequently-invoked code paths, avoid String.matches(...)/String.replaceAll(...) and repeated Pattern.compile(...); precompile and reuse Pattern (or avoid regex entirely when simple string operations suffice).

Example code before:

boolean ok = prefix.matches("[a-zA-Z*]+");
String cleaned = input.replaceAll("\\s+", "");

Example code after:

private static final Pattern PREFIX = Pattern.compile("[a-zA-Z*]+");
private static final Pattern WS = Pattern.compile("\\s+");
boolean ok = PREFIX.matcher(prefix).matches();
String cleaned = WS.matcher(input).replaceAll("");
Relevant past accepted suggestions:
Suggestion 1: [performance] `cleanKey()` uses `replaceAll`
`cleanKey()` uses `replaceAll` `cleanKey` calls `replaceAll("\\s", "")`, which recompiles the regex on every invocation and violates the compiled-regex requirement for non-trivial/repeated regex usage. This can add avoidable overhead in citation-key generation paths.

Issue description

CitationKeyGenerator.cleanKey currently removes whitespace via String.replaceAll("\\s", ""), which recompiles the pattern per call.

Issue Context

The compliance checklist requires using compiled Pattern instances for repeated/non-trivial regex operations.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/citationkeypattern/CitationKeyGenerator.java[151-152]

Suggestion 2: [performance] `prefix.matches()` used directly
`prefix.matches()` used directly The new `CitationCommandString` constructor validates `prefix` using `String.matches`, which recompiles the regex each call and violates the requirement to use a compiled `Pattern`. This can add avoidable overhead if citation commands are constructed frequently.

Issue description

CitationCommandString uses String.matches(...) for regex validation, which recompiles the pattern each time.

Issue Context

Compliance requires compiled Pattern reuse instead of repeated String.matches.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/push/CitationCommandString.java[12-16]

Suggestion 3: [performance] `command.matches()` used directly
`command.matches()` used directly The new CAYW request validation uses `String.matches`, which recompiles the regex for every request and violates the compiled-pattern requirement. This is a hot-path HTTP endpoint and should use a precompiled `Pattern`.

Issue description

CAYW command validation uses String.matches, recompiling the regex for each request.

Issue Context

Compliance requires using Pattern.compile(...) once and reusing it.

Fix Focus Areas

  • jabsrv/src/main/java/org/jabref/http/server/cayw/CAYWResource.java[74-80]

Suggestion 4: [performance] `matchRegex` recompiles Pattern
`matchRegex` recompiles Pattern Regex evaluation recompiles a `Pattern` on each match attempt (`Pattern.compile(...)` inside `matchRegex`), which can be expensive when scanning many entries. The rule requires compiling and reusing patterns where applicable.

Issue description

matchRegex compiles the regex on every call, which scales poorly when searching across many entries.

Issue Context

In-memory search walks every entry and can call regex matching many times per query.

Fix Focus Areas

  • jablib/src/main/java/org/jabref/logic/search/inmemory/BibEntryMatchVisitor.java[191-199] Suggested approaches:
  • Compile the pattern once per comparison term (e.g., in visitComparison) and pass a Pattern into matching functions.
  • Or add a small cache (e.g., Map<String, Pattern> keyed by (pattern, caseSensitive)) scoped to the visitor instance.

[Auto-generated best practices - 2026-05-24]

Clone this wiki locally