Paying off technical debt: almost all utility classes have a private constructor now.#2649
Conversation
|
Hm, not sure if I like the addition of those constructors just to prevent the instantiation of those classes?! |
|
|
||
| private static final Log LOGGER = LogFactory.getLog(ExportAction.class); | ||
|
|
||
| private ExportAction() { |
There was a problem hiding this comment.
I doubt that this is useful here. I am sure Export Action is somewhere called
There was a problem hiding this comment.
I just had a look at that. ExportAction is indeed instantiated in org.jabref.gui.preftabs.PreferencesDialog, but that class has its own ExportAction class in it. PreferencesDialog also has no import for org.jabref.gui.exporter.ExportAction, so the instantiation is most likely an instantiation of the inner class.
There was a problem hiding this comment.
Ah okay, good to know, seems like they are different exportActions here.
Some time ago I was working with these action classes and therefore was just crurious.
|
I would have not cared about this, as it is normally pretty obvious that you should not create an object of a class which only has static methods, but it is actually recommended http://www.informit.com/articles/article.aspx?p=1216151&seqNum=4 But I think we agreed a while ago that we focus more on fixing/implementing issues (where you can resolve some code quality issues locally) than solely on internal code quality. PS: You could also take a look at the modernizer test output log and maybe fix some of the mentioned issues as well (I would suggest ignoring group related stuff for the moment) |
|
The technical debt metrics from Sonarqube (or rather SQALE) are a little fuzzy, I would not care too much about the absolute numbers they yield ;) Anyway, in my opinion the merge conflicts should be resolved and the PR should be merged. At least nothing breaks from adding private constructors and on the plus side @TRvanRossum fixed at least one case where one of the classes was instantiated to call a static method. Some bracketing improvements are in there as well. |
|
The merge conflicts should be solved now. |
* upstream/master: Localization: General: French: Translation of new entries Localization: Menu: French: Translation of an entry (#2685) Fix #2680 and fix #2667: Swing errors are catched properly and without freezing (#2681) Do not log AND throw Replace misleading error message for fetcher connection error Document CrossRef test Fix subtitle detection for CrossRef fetcher Revert "Invoke LogMessages.add in JavaFX thread" Use global user agent Update mockito from 2.7.17 to 2.7.18 Move GuiAppender to GUI package Invoke LogMessages.add in JavaFX thread [WIP] Put the PDFAnnotationImporter under Test, enhance FileAnnotationTab (#2640) Fix for "Paying off technical debt: almost all utility classes have a private constructor now." (#2672) Revert "Paying off technical debt: almost all utility classes have a private constructor now. (#2649)" (#2670) Paying off technical debt: almost all utility classes have a private constructor now. (#2649) Changed codeformatting for better fxml annotation (#2668) Disalbe Google Scholar tests on all CI environments (#2654) Fix JSONException in Crossref fetcher as mentioned in #2442
I also removed an unused private method. According to SonarQube scans, this removes at least five days of technical debt.
gradle localizationUpdate?