build(pmd): update PMD rules and fix issues (#687)

* update and customize PDM rules for the launcher * start with 'category/java/errorprone.xml' and exclude or configure individual rules to fit to the project. * fix reported PMD issues or suppress warnings * fix reported CheckStyle issues or suppress warnings
MovingBlocks · Dec 17, 2022 · cfa8ce0 · cfa8ce0
1 parent c17773a
commit cfa8ce0
Show file tree

Hide file tree

Showing 18 changed files with 113 additions and 67 deletions.
diff --git a/build.gradle b/build.gradle
@@ -190,13 +190,18 @@ test {
 }
 
 checkstyle {
+    toolVersion = "10.4"
     ignoreFailures = false
     configDirectory.set(metricsConfigDir.dir("checkstyle"))
 }
 
 pmd {
-    ignoreFailures = true
-    ruleSetFiles = files("$rootDir/config/metrics/pmd/pmd.xml")
+    toolVersion = "6.39.0"
+    ignoreFailures = false
+    consoleOutput = true
+    threads = 4
+    ruleSetFiles "${rootDir}/config/pmd.xml"
+    ruleSets = []
 }
 
 spotbugs {
@@ -240,12 +245,14 @@ task extractCodeMetricsConfig(type: Copy) {
     into metricsConfigDir
 }
 
+processResources.dependsOn(createVersionInfoFile, extractCodeMetricsConfig)
+
 clean {
     delete createVersionInfoFile.outputs.files
     delete extractCodeMetricsConfig.destinationDir
 }
 
-processResources.dependsOn(createVersionInfoFile, extractCodeMetricsConfig)
+processResources.dependsOn(createVersionInfoFile)
 
 jar {
     //TODO we only use this name because the `.exe` start scripts require the JAR to be named 'TerasologyLauncher.jar'

diff --git a/config/pmd.xml b/config/pmd.xml
@@ -0,0 +1,32 @@
+<?xml version="1.0"?>
+
+<ruleset name="TerasologyLauncher PMD ruleset"
+         xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd">
+
+    <description>
+        Custom rules for the Terasology Launcher project.
+    </description>
+
+    <!-- Your rules will come here -->
+
+    <rule ref="category/java/errorprone.xml">
+        <exclude name="AvoidFieldNameMatchingMethodName"/>
+        <exclude name="AvoidFieldNameMatchingTypeName"/>
+        <exclude name="AvoidLiteralsInIfCondition"/>
+        <exclude name="BeanMembersShouldSerialize"/>
+        <exclude name="DoNotTerminateVM"/>
+        <exclude name="MissingSerialVersionUID"/>
+        <exclude name="NullAssignment"/>
+        <exclude name="SimpleDateFormatNeedsLocale"/>
+        <exclude name="UseLocaleWithCaseConversions"/>
+    </rule>
+
+    <rule ref="category/java/codestyle.xml/FieldNamingConventions">
+        <properties>
+            <property name="exclusions" value="logger|gson|serialVersionUID"/>
+        </properties>
+    </rule>
+
+</ruleset>
diff --git a/src/main/java/org/terasology/launcher/TerasologyLauncher.java b/src/main/java/org/terasology/launcher/TerasologyLauncher.java
@@ -98,7 +98,7 @@ public void start(final Stage initialStage) {
         });
 
         launcherInitTask.setOnFailed(event -> {
-            logger.error("The TerasologyLauncher could not be started!", event.getSource().getException());
+            logger.error("The TerasologyLauncher could not be started!", event.getSource().getException()); //NOPMD
             System.exit(1);
         });
 
@@ -179,9 +179,12 @@ private void showSplashStage(final Stage initialStage, final Task<LauncherConfig
     private void logSystemInformation() {
         if (logger.isDebugEnabled()) {
             // Java
-            logger.debug("Java: {} {} {}", System.getProperty("java.version"), System.getProperty("java.vendor"), System.getProperty("java.home"));
-            logger.debug("Java VM: {} {} {}", System.getProperty("java.vm.name"), System.getProperty("java.vm.vendor"), System.getProperty("java.vm.version"));
-            logger.debug("Java classpath: {}", System.getProperty("java.class.path"));
+            logger.debug("Java: {} {} {}",
+                    System.getProperty("java.version"), System.getProperty("java.vendor"), System.getProperty("java.home"));
+            logger.debug("Java VM: {} {} {}",
+                    System.getProperty("java.vm.name"), System.getProperty("java.vm.vendor"), System.getProperty("java.vm.version"));
+            logger.debug("Java classpath: {}",
+                    System.getProperty("java.class.path"));
 
             // OS
             logger.debug("OS: {} {} {}", System.getProperty("os.name"), System.getProperty("os.arch"), System.getProperty("os.version"));

diff --git a/src/main/java/org/terasology/launcher/game/GameManager.java b/src/main/java/org/terasology/launcher/game/GameManager.java
@@ -87,7 +87,8 @@ public void install(GameRelease release, ProgressListener listener) throws IOExc
         }
     }
 
-    private void download(GameRelease release, Path targetLocation, ProgressListener listener) throws DownloadException, IOException, InterruptedException {
+    private void download(GameRelease release, Path targetLocation, ProgressListener listener)
+            throws DownloadException, IOException, InterruptedException {
         final URL downloadUrl = release.getUrl();
 
         final long contentLength = DownloadUtils.getContentLength(downloadUrl);

diff --git a/src/main/java/org/terasology/launcher/game/GameStarter.java b/src/main/java/org/terasology/launcher/game/GameStarter.java
@@ -22,7 +22,7 @@
  *
  * @see <a href="https://docs.oracle.com/en/java/javase/14/docs/specs/man/java.html#overview-of-java-options">java command manual</a>
  */
-class GameStarter implements Callable<Process> {
+final class GameStarter implements Callable<Process> {
     private static final Logger logger = LoggerFactory.getLogger(GameStarter.class);
 
     final ProcessBuilder processBuilder;

diff --git a/src/main/java/org/terasology/launcher/log/TempLogFilePropertyDefiner.java b/src/main/java/org/terasology/launcher/log/TempLogFilePropertyDefiner.java
@@ -31,7 +31,7 @@ public TempLogFilePropertyDefiner() {
             throw new IllegalStateException("This class must not be instantiated twice");
         }
 
-        instance = this;
+        instance = this;    //NOPMD(AssignmentToNonFinalStatic)
     }
 
     public static TempLogFilePropertyDefiner getInstance() {
@@ -55,6 +55,7 @@ public String getPrefix() {
 
     /**
      * Set the prefix string for generating the file's name.
+     *
      * @param prefix the prefix string to be used in generating the file's name; may be null
      */
     public void setPrefix(String prefix) {
@@ -67,6 +68,7 @@ public String getSuffix() {
 
     /**
      * Set the suffix string for generating the file's name.
+     *
      * @param suffix the suffix string to be used in generating the file's name; may be null, in which case ".tmp" is used
      */
     public void setSuffix(String suffix) {
@@ -75,6 +77,7 @@ public void setSuffix(String suffix) {
 
     /**
      * Returns the temporary log file.
+     *
      * @return the log file
      */
     public Path getLogFile() {

diff --git a/src/main/java/org/terasology/launcher/repositories/Jenkins.java b/src/main/java/org/terasology/launcher/repositories/Jenkins.java
@@ -5,12 +5,10 @@
 
 /**
  * Data model for parsing build information from Jenkins.
+ *
+ * Instances of this class will be created by JSON parsers (e.g., GSON) and are usually not instantiated by hand.
  */
 public final class Jenkins {
-
-    private Jenkins() {
-    }
-
     public static class ApiResult {
         public Build[] builds;
     }

diff --git a/src/main/java/org/terasology/launcher/repositories/JenkinsClient.java b/src/main/java/org/terasology/launcher/repositories/JenkinsClient.java
@@ -29,10 +29,10 @@ class JenkinsClient {
 
     private static final String ARTIFACT = "artifact/";
 
-    private final Gson gson;
-
     final OkHttpClient client;
 
+    private final Gson gson;
+
     JenkinsClient(OkHttpClient httpClient, Gson gson) {
         this.gson = gson;
 
@@ -83,6 +83,7 @@ Jenkins.ApiResult request(URL url) throws InterruptedException {
     // c) the 'Expires' header is removed from response for requests with PropertiesRequest
 
     @Nullable
+    @SuppressWarnings("PMD.ReturnEmptyCollectionRatherThanNull")
     Properties requestProperties(final URL artifactUrl) {
         Preconditions.checkNotNull(artifactUrl);
 

diff --git a/src/main/java/org/terasology/launcher/repositories/JenkinsRepositoryAdapter.java b/src/main/java/org/terasology/launcher/repositories/JenkinsRepositoryAdapter.java
@@ -138,7 +138,7 @@ private String computeChangelogFrom(Jenkins.ChangeSet changeSet) {
     private static URL unsafeToUrl(String url) {
         try {
             return new URL(url);
-        } catch (MalformedURLException e) {
+        } catch (MalformedURLException e) { //NOPMD
             //TODO: at least log something here?
         }
         return null;

diff --git a/src/main/java/org/terasology/launcher/settings/LauncherSettings.java b/src/main/java/org/terasology/launcher/settings/LauncherSettings.java
@@ -7,7 +7,6 @@
 import com.google.gson.Gson;
 import com.google.gson.JsonParseException;
 import javafx.beans.property.Property;
-import javafx.beans.property.ReadOnlyProperty;
 import javafx.beans.property.SimpleBooleanProperty;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -30,10 +29,9 @@
 /**
  * User settings for the launcher, backed by Java {@link Properties}.
  */
+@SuppressWarnings("checkstyle:DeclarationOrder")
 public class LauncherSettings {
 
-    private static final Logger logger = LoggerFactory.getLogger(LauncherSettings.class);
-
     public static final String USER_JAVA_PARAMETERS_DEFAULT = "-XX:MaxGCPauseMillis=20";
     public static final String USER_GAME_PARAMETERS_DEFAULT = "";
 
@@ -54,6 +52,11 @@ public class LauncherSettings {
     public static final String PROPERTY_LAST_INSTALLED_GAME_JOB = "lastInstalledGameJob";
     public static final String PROPERTY_LAST_INSTALLED_GAME_VERSION = "lastInstalledGameVersion";
 
+    private static final Logger logger = LoggerFactory.getLogger(LauncherSettings.class);
+
+    private static final String WARN_MSG_INVALID_VALUE = "Invalid value '{}' for the parameter '{}'!";
+    private static final Level LOG_LEVEL_DEFAULT = Level.INFO;
+
     static final JavaHeapSize MAX_HEAP_SIZE_DEFAULT = JavaHeapSize.NOT_USED;
     static final JavaHeapSize INITIAL_HEAP_SIZE_DEFAULT = JavaHeapSize.NOT_USED;
     static final boolean CLOSE_LAUNCHER_AFTER_GAME_START_DEFAULT = true;
@@ -64,9 +67,6 @@ public class LauncherSettings {
 
     static final String LAUNCHER_SETTINGS_FILE_NAME = "TerasologyLauncherSettings.properties";
 
-    private static final String WARN_MSG_INVALID_VALUE = "Invalid value '{}' for the parameter '{}'!";
-    private static final Level LOG_LEVEL_DEFAULT = Level.INFO;
-
     private final Properties properties;
 
     private final Property<Boolean> showPreReleases = new SimpleBooleanProperty(SHOW_PRE_RELEASES_DEFAULT);
@@ -105,7 +105,9 @@ public synchronized void init() {
     void initLocale() {
         final String localeStr = properties.getProperty(PROPERTY_LOCALE);
         if (localeStr != null) {
-            Locale l = I18N.getSupportedLocales().stream().filter(x -> x.toLanguageTag().equals(localeStr)).findFirst().orElse(I18N.getDefaultLocale());
+            Locale l = I18N.getSupportedLocales().stream()
+                    .filter(x -> x.toLanguageTag().equals(localeStr))
+                    .findFirst().orElse(I18N.getDefaultLocale());
             I18N.setLocale(l);
 
             if (!I18N.getCurrentLocale().toString().equals(localeStr)) {
@@ -246,14 +248,6 @@ void initLastInstalledGameVersion() {
         }
     }
 
-    // --------------------------------------------------------------------- //
-    // PROPERTIES
-    // --------------------------------------------------------------------- //
-
-    public ReadOnlyProperty<Boolean> showPreReleases() {
-        return showPreReleases;
-    }
-
     // --------------------------------------------------------------------- //
     // GETTERS
     // --------------------------------------------------------------------- //

diff --git a/src/main/java/org/terasology/launcher/ui/ApplicationController.java b/src/main/java/org/terasology/launcher/ui/ApplicationController.java
@@ -244,8 +244,10 @@ private void initComboBoxes() {
         }, config, showPreReleases);
 
         gameReleaseComboBox.itemsProperty().bind(releases);
-        gameReleaseComboBox.buttonCellProperty().bind(Bindings.createObjectBinding(() -> new GameReleaseCell(installedGames, true), installedGames));
-        gameReleaseComboBox.cellFactoryProperty().bind(Bindings.createObjectBinding(() -> list -> new GameReleaseCell(installedGames), installedGames));
+        gameReleaseComboBox.buttonCellProperty()
+                .bind(Bindings.createObjectBinding(() -> new GameReleaseCell(installedGames, true), installedGames));
+        gameReleaseComboBox.cellFactoryProperty()
+                .bind(Bindings.createObjectBinding(() -> list -> new GameReleaseCell(installedGames), installedGames));
 
         selectedRelease.bind(gameReleaseComboBox.getSelectionModel().selectedItemProperty());
         //TODO: instead of imperatively updating the changelog view its value should be bound via property, too

diff --git a/src/main/java/org/terasology/launcher/ui/FxTimer.java b/src/main/java/org/terasology/launcher/ui/FxTimer.java
@@ -29,7 +29,7 @@ private FxTimer(java.time.Duration actionTime, java.time.Duration period, Runnab
         this.action = action;
 
         timeline.getKeyFrames().add(new KeyFrame(this.actionTime)); // used as placeholder
-        if (period != actionTime) {
+        if (!period.equals(actionTime)) {
             timeline.getKeyFrames().add(new KeyFrame(Duration.millis(period.toMillis())));
         }
 

diff --git a/src/main/java/org/terasology/launcher/ui/SettingsController.java b/src/main/java/org/terasology/launcher/ui/SettingsController.java
@@ -31,7 +31,6 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Locale;
-import java.util.MissingResourceException;
 import java.util.stream.Collectors;
 
 public class SettingsController {
@@ -386,18 +385,16 @@ protected void updateItem(Locale item, boolean empty) {
                 String countryCode = item.toLanguageTag();
                 String id = "flag_" + countryCode;
 
-                try {
-                    // Get the appropriate flag icon via BundleUtils
-                    Image icon = I18N.getFxImage(id);
-
+                // Get the appropriate flag icon via BundleUtils
+                Image icon = I18N.getFxImage(id);
+                if (icon != null) {
                     ImageView iconImageView = new ImageView(icon);
                     iconImageView.setFitHeight(11);
                     iconImageView.setPreserveRatio(true);
                     this.setGraphic(iconImageView);
-                } catch (MissingResourceException e) {
-                    logger.warn("ImageBundle key {} not found", id);
-                } catch (NullPointerException e) {
-                    logger.warn("Flag icon in ImageBundle key {} missing or corrupt", id);
+                } else {
+                    logger.warn("Flag icon for key '{}' could not be loaded.", id);
+
                 }
             }
         }

diff --git a/src/main/java/org/terasology/launcher/util/DownloadUtils.java b/src/main/java/org/terasology/launcher/util/DownloadUtils.java
@@ -99,7 +99,7 @@ private static void downloadToFile(ProgressListener listener, long contentLength
         long writtenBytes = 0;
         int n;
         if (!listener.isCancelled()) {
-            while ((n = in.read(buffer)) != -1) {
+            while ((n = in.read(buffer)) != -1) { //NOPMD(AssignmentInOperand)
                 if (listener.isCancelled()) {
                     break;
                 }
@@ -110,7 +110,7 @@ private static void downloadToFile(ProgressListener listener, long contentLength
                 int percentage = (int) (sizeFactor * writtenBytes);
                 if (percentage < 1) {
                     percentage = 1;
-                } else if (percentage >= 100) {
+                } else if (percentage >= 100) { //NOPMD(AvoidLiteralsInIfCondition)
                     percentage = 99;
                 }
                 listener.update(percentage);