OO Maintenance: Better checks, clean up dead code, refactoring #11779

subhramit · 2024-09-16T22:12:39Z

Clean up some dead code (due to redundant conditions).
Merge pre-conditions more efficiently, use real condition checks instead of flags always being set to true/false.
Follow-up to the beginning of refactoring at Add setting: always add "Cited on pages" text to JStyles. #11732 (comment) - since OO preferences are now declared at class level, it makes sense to use it everywhere instead of calling the getter again and again. In other cases as well, this PR removes extra get calls for null checks.
Minimize usage of preferences by passing specific preferences via constructor.
Use sub-typed exception handling in certain cases.
Use more relevant names for variables.

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

koppor · 2024-09-16T22:39:12Z

src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

@@ -601,12 +587,12 @@ private boolean checkThatEntriesHaveKeys(List<BibEntry> entries) {
        Optional<BibDatabaseContext> databaseContext = stateManager.getActiveDatabase();
        if (citePressed && databaseContext.isPresent()) {
            // Generate keys
-            CitationKeyPatternPreferences prefs = preferences.getCitationKeyPatternPreferences();
+            CitationKeyPatternPreferences citationKeyPatternPreferences = preferences.getCitationKeyPatternPreferences();


Maybe, you can have these passed in the constructor - and get rid of preferences parameter alltogheter? ^^

That would be the ideal level of abstraction, but preferences would be needed here:

Reason for moving openOfficePreferences in the constructor initialization block was that it is the core preference used all over the class (22 usages), whereas the rest of the preferences (ExternalApplicationsPreferences, LayoutFormatterPreferences, CitationKeyPatternPreferences) are used just used once in within their respective scopes (three times all together in total), thus declared locally.

@koppor I tried to clean it up a bit further by passing all the preference subtypes via constructor. Although I couldn't get rid of preferences, I could minimize its usage to this one place (above comment). Check commit f759087. If this is not useful, we can revert the commit.

Advantage (imo) - more inversion of control (gets rid of all getters).
Ping: @calixtus

koppor · 2024-09-17T08:53:23Z

src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

@@ -511,12 +511,6 @@ private void pushEntries(CitationType citationType, boolean addPageInfo) {
        }

        final BibDatabaseContext bibDatabaseContext = stateManager.getActiveDatabase().get();
-        if (bibDatabaseContext == null) {


Future: It is ok to add assert statements.

Example:

assert bibDatabaseContext != null

OK, one needs to add -ea somehow: https://stackoverflow.com/a/18168305/873282 -- I would have bet that I saw assertion errors when developing...

The long discussion at https://stackoverflow.com/q/2758224/873282 says: do something else, but I like assertions:

More semantic meaning than removed code

More semantic meaning than code comments

Runs in debug mode only (in contrast to Objects.requireNonNull

Does not clutter the code with @NonNull annotations, which need a separate framework for checking - (https://github.com/kelloggm/checkerframework-gradle-plugin)

Okay, will follow that.
Reason for removal here was that bibDatabaseContext could never be null here.

Sure. At the time of the code. But what if there is a future refactoring. One could argue that the whole research of pre- and post conditions is useless, because the author was sure at when writing the code 🤣🤣

This kind of code (or JSpecify annitations are useful for working on the code at a later point of time. To avoid regressions.

Side track: Another tool for avoiding regressions is OpenFastTrace - see https://devdocs.jabref.org/requirements/ for details ^^.

If it is to ensure the precondition, it is better to revert this change and use the dedicated error dialog instead of using an internal assertion. Will be updated in latest commit.

Checks for empty database and databasecontext merged in (updated) lines 509-515.

koppor · 2024-09-17T08:54:51Z

src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java

-        boolean hasStyle = true; // (style != null);
-        boolean hasDatabase = true; // !getBaseList().isEmpty();


These two comments (non-null check / isEmpty) are typical assert statements.

Okay wait, don't merge yet, I'll see if I can put those conditions/assertions (replacing style->currentStyle) instead of complete removal. I don't know if there were reasons these were commented and true was hardcoded.

koppor

As discussed: use of assert

Siedlerchr · 2024-09-20T20:47:19Z

form my point of view okay now

subhramit · 2024-09-20T20:51:52Z

I'll try to finish this up by tomorrow. Not much left. Just checking if we can work without listeners (I did something similar when disabling buttons during the csl project)...or else just add a few assertions.

subhramit · 2024-09-22T00:45:21Z

Whenever available, I request one of you to check the diff since the last changes and give an approval for merge.

obsolete

subhramit added 3 commits September 17, 2024 03:29

Clean up dead code

150603c

Use typed exception handling

d21a455

Rename prefs variable

041bf02

subhramit added openoffice/libreoffice type: code-quality Issues related to code or architecture decisions labels Sep 16, 2024

Merge branch 'main' into oo-maintenance

3dd04a4

subhramit requested review from Siedlerchr, koppor and calixtus September 16, 2024 22:13

koppor reviewed Sep 16, 2024

View reviewed changes

subhramit added 2 commits September 17, 2024 04:55

Minimize usage of preferences

f759087

Merge branch 'main' into oo-maintenance

985c9a8

subhramit requested a review from koppor September 16, 2024 23:39

calixtus previously approved these changes Sep 16, 2024

View reviewed changes

koppor reviewed Sep 17, 2024

View reviewed changes

koppor approved these changes Sep 17, 2024

View reviewed changes

koppor reviewed Sep 17, 2024

View reviewed changes

koppor previously requested changes Sep 17, 2024

View reviewed changes

subhramit marked this pull request as draft September 18, 2024 08:12

Merge branch 'main' into oo-maintenance

cb1bcc8

Siedlerchr previously approved these changes Sep 20, 2024

View reviewed changes

subhramit added 2 commits September 22, 2024 05:29

Merge branch 'main' into oo-maintenance

073d550

Enhanced pre-condition checks

7958e7d

subhramit dismissed stale reviews from calixtus and Siedlerchr via 7958e7d September 22, 2024 00:37

subhramit marked this pull request as ready for review September 22, 2024 00:39

subhramit requested a review from koppor September 22, 2024 00:45

subhramit added 2 commits September 22, 2024 06:25

Merge condition

68233dd

Reorder flags

6594375

subhramit changed the title ~~OO Maintenance: Clean up dead code, refactoring~~ OO Maintenance: Clean up dead code, better checks, refactoring Sep 22, 2024

subhramit changed the title ~~OO Maintenance: Clean up dead code, better checks, refactoring~~ OO Maintenance: Better checks, clean up dead code, refactoring Sep 22, 2024

subhramit enabled auto-merge September 22, 2024 12:37

Merge branch 'main' into oo-maintenance

a913ebe

subhramit requested a review from Siedlerchr September 22, 2024 22:09

subhramit added 3 commits September 23, 2024 03:42

Revert check: entry selection presence

6131662

Fix indent

2e77921

Merge branch 'main' into oo-maintenance

9e76718

Siedlerchr approved these changes Sep 23, 2024

View reviewed changes

subhramit added this pull request to the merge queue Sep 23, 2024

Merged via the queue into JabRef:main with commit 0e10cef Sep 23, 2024
23 checks passed

subhramit deleted the oo-maintenance branch September 23, 2024 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OO Maintenance: Better checks, clean up dead code, refactoring #11779

OO Maintenance: Better checks, clean up dead code, refactoring #11779

subhramit commented Sep 16, 2024 •

edited

Loading

koppor Sep 16, 2024

subhramit Sep 16, 2024 •

edited

Loading

subhramit Sep 16, 2024 •

edited

Loading

koppor Sep 17, 2024

subhramit Sep 17, 2024

koppor Sep 17, 2024

subhramit Sep 22, 2024

subhramit Sep 22, 2024

koppor Sep 17, 2024

subhramit Sep 17, 2024 •

edited

Loading

koppor left a comment

Siedlerchr commented Sep 20, 2024

subhramit commented Sep 20, 2024

subhramit commented Sep 22, 2024 •

edited

Loading

		boolean hasStyle = true; // (style != null);
		boolean hasDatabase = true; // !getBaseList().isEmpty();

OO Maintenance: Better checks, clean up dead code, refactoring #11779

OO Maintenance: Better checks, clean up dead code, refactoring #11779

Conversation

subhramit commented Sep 16, 2024 • edited Loading

Mandatory checks

koppor Sep 16, 2024

Choose a reason for hiding this comment

subhramit Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

subhramit Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

koppor Sep 17, 2024

Choose a reason for hiding this comment

subhramit Sep 17, 2024

Choose a reason for hiding this comment

koppor Sep 17, 2024

Choose a reason for hiding this comment

subhramit Sep 22, 2024

Choose a reason for hiding this comment

subhramit Sep 22, 2024

Choose a reason for hiding this comment

koppor Sep 17, 2024

Choose a reason for hiding this comment

subhramit Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

Siedlerchr commented Sep 20, 2024

subhramit commented Sep 20, 2024

subhramit commented Sep 22, 2024 • edited Loading

subhramit commented Sep 16, 2024 •

edited

Loading

subhramit Sep 16, 2024 •

edited

Loading

subhramit Sep 16, 2024 •

edited

Loading

subhramit Sep 17, 2024 •

edited

Loading

subhramit commented Sep 22, 2024 •

edited

Loading