Skip to content
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

Jump to bottom

Fix some IntelliJ findings #6204

Merged
merged 3 commits into from
Mar 30, 2020
Merged

Fix some IntelliJ findings #6204

merged 3 commits into from
Mar 30, 2020

Conversation

koppor
Copy link
Member

@koppor koppor commented Mar 29, 2020

  • Fix bug in TagBar (added itself instead of newTags)
  • Wrong number of LOGGER paramters
  • Inner class may be static
  • Fix comment typo
  • Bulk operation can be used instead of iteration
  • Arrays.asList with only one element
  • Optimized count by using numbers earlier
  • Chained append for StringBuilder
  • Initialize ArrayList by passing the initial contents in the constructor

@koppor
Fix some IntelliJ findings
167558d 
- Fix bug in TagBar (added itself instead of newTags)
- Wrong number of LOGGER paramters
- Inner class may be static
- Fix comment typo
- Bulk operation can be used instead of iteration
- Arrays.asList with only one element
- Optimized count by using numbers earlier
- Chained append for StringBuilder
- Initialize ArrayList by passing the initial contents in the constructor
@koppor koppor added the type: code-quality Issues related to code or architecture decisions label Mar 29, 2020
tobiasdiez
tobiasdiez requested changes Mar 30, 2020
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

It's ok, but I think we have more important construction sites then these minor code improvements. Especially if some of them are questionable (e.g. I found the StringBuilder.append version less readable then a normal string concatenation).

src/main/java/org/jabref/gui/fieldeditors/contextmenu/CaseChangeMenu.java Outdated
@@ -23,8 +23,7 @@ public CaseChangeMenu(final StringProperty text) {
Objects.requireNonNull(text);

// create menu items, one for each case changer
List<Formatter> caseChangers = new ArrayList<>();
caseChangers.addAll(Formatters.getCaseChangers());
List<Formatter> caseChangers = new ArrayList<>(Formatters.getCaseChangers());
Copy link
Member

Choose a reason for hiding this comment

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

I found the previous code easier to understand...

Copy link
Member Author

Choose a reason for hiding this comment

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

IntelliJ reported that as performance issue. I will revert it nevertheless, as I think it's not that crucial...

Copy link
Member

Choose a reason for hiding this comment

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

I actually find the intelij variant easy enough to understand. It's obvious that the constructor of ArrayList can take a collection

src/main/java/org/jabref/logic/cleanup/Cleanups.java Outdated Show resolved Hide resolved
@koppor
Copy link
Member Author

koppor commented Mar 30, 2020

Reverted the questioned improvements.

@tobiasdiez tobiasdiez merged commit d1a83cd into master Mar 30, 2020
@tobiasdiez tobiasdiez deleted the fix-intellij-findings branch March 30, 2020 12:54
koppor pushed a commit that referenced this pull request Nov 15, 2022
Squashed 'buildres/csl/csl-styles/' changes from 13fd98e..7a10e3f
951c108 
7a10e3f Bluebook: Remove small-caps for case short & legislation (#6305)
ca4a92b Merge pull request #6244 from POBrien333/patch-1107
12743ad Create social-science-history.csl (#6233)
f7c1d57 Update american-chemical-society.csl (#6231)
aca6b23 ready: Update oil-shale.csl (#6225)
aadae55 Create pallas.csl (#6204)
cc7d016 Merge pull request #6253 from nschneid/patch-1
77fab39 Merge pull request #6282 from POBrien333/patch-1119
e2668eb Merge pull request #6263 from POBrien333/patch-1113
7f3244d move style to dependent folder
8584993 Create development-growth-differentiation.csl (#6226)
dfe71ff Create biotechnology-and-bioprocess-engineering.csl (#6227)
0d91742 Create sn-computer-science.csl (#6228)
a0d09b4 Create mots.csl (#6205)
47665e5 Update review-of-international-studies.csl
d573b8b Create computer-supported-cooperative-work.csl
cebec0e ACL: check if edition is ordinal before printing the word "edition"
03a0a39 Re-indent CSL styles
3c64906 fix self link
479c061 fix small issues after visual inspection
a356e72 Create gnosis-journal-of-gnostic-studies.csl

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 7a10e3f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr left review comments

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Assignees
No one assigned
Labels
type: code-quality Issues related to code or architecture decisions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@koppor @Siedlerchr @tobiasdiez