qa: address pmd findings (#5274)

* qa: fix PMD CollapsibleIfStatements finding * qa: fix PMD SimplifiedTernary findings * fix: formatting mistake * qa: address PMD SystemPrintln findings - suppress for LoggingContext.java and Terasology.java because logging not necessarily available yet there - refactor for PathManager.java as logger is already in use in that context, also see previous discussion in #5036 (comment) * qa: address PMD InvalidLogMessageFormat finding - if I understand https://stackoverflow.com/questions/7102339/is-there-a-correct-way-to-pass-arguments-in-slf4j correctly, this should not be less performant but valid SLF4j format * qa: address PMD AvoidBranchingStatementAsLastInLoop findings * qa: address PMD ForLoopCanBeForeach findings * chore: remove unused import * qa: address PMD AvoidPrintStackTrace findings * chore: remove unused imports * chore: fix checkstyle DeclarationOrderCheck warning
MovingBlocks · Oct 4, 2024 · ba4d716 · ba4d716
1 parent d2810e6
commit ba4d716
Show file tree

Hide file tree

Showing 19 changed files with 71 additions and 72 deletions.
diff --git a/engine/src/main/java/org/terasology/engine/core/LoggingContext.java b/engine/src/main/java/org/terasology/engine/core/LoggingContext.java
@@ -25,6 +25,7 @@
 /**
  * Configures the underlying logback logging framework.
  */
+@SuppressWarnings("PMD.SystemPrintln")
 public final class LoggingContext {
 
     /**
@@ -74,7 +75,7 @@ public static void initialize(Path logFileFolder) {
         try {
             deleteLogFiles(logFileFolder, Duration.ofDays(5).getSeconds());
         } catch (IOException e) {
-            e.printStackTrace();
+            e.printStackTrace(); //NOPMD
         }
 
         // Unfortunately, setting context-based variables works only after initialization

diff --git a/engine/src/main/java/org/terasology/engine/core/PathManager.java b/engine/src/main/java/org/terasology/engine/core/PathManager.java
@@ -99,11 +99,11 @@ private static Path findInstallPath() {
             }
         }
 
-        System.err.printf(
-                "Native library installation directory not found. %n" +
-                "Things will almost certainly crash as a result, %n" +
-                "unless something else installed everything to java.library.path. %n" +
-                "Searched: %s%n", installationSearchPaths
+        LOGGER.error(
+                "Native library installation directory not found. /n" +
+                "Things will almost certainly crash as a result, /n" +
+                "unless something else installed everything to java.library.path. /n" +
+                "Searched: {}/n", installationSearchPaths
         );
         return currentDirectory;
     }

diff --git a/engine/src/main/java/org/terasology/engine/core/module/ModuleInstaller.java b/engine/src/main/java/org/terasology/engine/core/module/ModuleInstaller.java
@@ -65,7 +65,7 @@ private Map<URI, Path> getDownloadUrls(Iterable<Module> modules) {
             try {
                 uri = RemoteModuleExtension.getDownloadUrl(metadata).toURI();
             } catch (URISyntaxException e) {
-                e.printStackTrace();
+                logger.error("Couldn't get download URL: ", e);
             }
             String fileName = String.format("%s-%s.jar", id, version);
             Path folder = PathManager.getInstance().getHomeModPath().normalize();

diff --git a/...ne/src/main/java/org/terasology/engine/core/module/rendering/RenderingModuleRegistry.java b/...ne/src/main/java/org/terasology/engine/core/module/rendering/RenderingModuleRegistry.java
@@ -2,10 +2,12 @@
 // SPDX-License-Identifier: Apache-2.0
 package org.terasology.engine.core.module.rendering;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.terasology.context.annotation.API;
 import org.terasology.engine.context.Context;
 import org.terasology.engine.rendering.dag.ModuleRendering;
 import org.terasology.gestalt.module.ModuleEnvironment;
-import org.terasology.context.annotation.API;
 import org.terasology.gestalt.naming.Name;
 
 import javax.annotation.Nullable;
@@ -20,6 +22,7 @@
 
 @API
 public class RenderingModuleRegistry {
+    private static final Logger LOGGER = LoggerFactory.getLogger(RenderingModuleRegistry.class);
 
     private List<ModuleRendering> orderedModuleRenderingInstances = new ArrayList<>();
 
@@ -126,7 +129,7 @@ private Set<ModuleRendering> fetchRenderingModules(ModuleEnvironment moduleEnvir
                     Constructor<?> constructor = renderingClass.getConstructor(Context.class);
                     moduleRenderingInstance = (ModuleRendering) constructor.newInstance(context);
                 } catch (InstantiationException | IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
-                    e.printStackTrace();
+                    LOGGER.error("Couldn't get constructor: ", e);
                 }
             }
             if (moduleRenderingInstance != null) {

diff --git a/engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/DebugCallback.java b/engine/src/main/java/org/terasology/engine/core/subsystem/lwjgl/DebugCallback.java
@@ -38,26 +38,24 @@ public class DebugCallback implements org.lwjgl.opengl.GLDebugMessageCallbackI {
     @Override
     public void invoke(int source, int type, int id, int severity, int length, long message, long userParam) {
         String logFormat = "[{}] [{}] [{}] {}";
-        Object[] args = new Object[]{
-                "0x" + Integer.toHexString(id).toUpperCase(Locale.ROOT),
-                getSourceString(source),
-                getTypeString(type),
-                MemoryUtil.memASCII(message).trim()
-        };
+        String idString = "0x" + Integer.toHexString(id).toUpperCase(Locale.ROOT);
+        String sourceString = getSourceString(source);
+        String typeString = getTypeString(type);
+        String messageString = MemoryUtil.memASCII(message).trim();
 
         switch (severity) {
             case GL_DEBUG_SEVERITY_HIGH:
-                logger.error(logFormat, args);
+                logger.error(logFormat, idString, sourceString, typeString, messageString);
                 break;
             case GL_DEBUG_SEVERITY_MEDIUM:
-                logger.warn(logFormat, args);
+                logger.warn(logFormat, idString, sourceString, typeString, messageString);
                 break;
             case GL_DEBUG_SEVERITY_LOW:
-                logger.debug(logFormat, args);
+                logger.debug(logFormat, idString, sourceString, typeString, messageString);
                 break;
             default:
             case GL_DEBUG_SEVERITY_NOTIFICATION:
-                logger.trace(logFormat, args);
+                logger.trace(logFormat, idString, sourceString, typeString, messageString);
                 break;
         }
     }

diff --git a/...n/java/org/terasology/engine/entitySystem/entity/lifecycleEvents/BeforeEntityCreated.java b/...n/java/org/terasology/engine/entitySystem/entity/lifecycleEvents/BeforeEntityCreated.java
@@ -80,17 +80,15 @@ private Component getNext() {
                 if (componentsToAdd.containsKey(result.getClass())) {
                     throw new IllegalStateException("Requested to add component that was already defined for this entity");
                 }
-                if (componentsToRemove.contains(result.getClass())) {
-                    continue;
+                if (!componentsToRemove.contains(result.getClass())) {
+                    return result;
                 }
-                return result;
             }
             while (addedIterator.hasNext()) {
                 final Component result = addedIterator.next();
-                if (componentsToRemove.contains(result.getClass())) {
-                    continue;
+                if (!componentsToRemove.contains(result.getClass())) {
+                    return result;
                 }
-                return result;
             }
             return null;
         }

diff --git a/engine/src/main/java/org/terasology/engine/logic/behavior/asset/GroupBuilder.java b/engine/src/main/java/org/terasology/engine/logic/behavior/asset/GroupBuilder.java
@@ -19,6 +19,7 @@
  * @see Group
  */
 @API
+@SuppressWarnings("PMD.AvoidPrintStackTrace")
 public class GroupBuilder {
 
     private GroupData groupData;

diff --git a/engine/src/main/java/org/terasology/engine/logic/debug/MovementDebugCommands.java b/engine/src/main/java/org/terasology/engine/logic/debug/MovementDebugCommands.java
@@ -263,7 +263,7 @@ public String playerEyeHeight(@Sender EntityRef client, @CommandParam("eye-heigh
             }
             return "";
         } catch (NullPointerException e) {
-            e.printStackTrace();
+            logger.error("Couldn't set player eye height: e");
             return "";
         }
     }

diff --git a/engine/src/main/java/org/terasology/engine/logic/spawner/AbstractSpawner.java b/engine/src/main/java/org/terasology/engine/logic/spawner/AbstractSpawner.java
@@ -88,10 +88,9 @@ protected Vector3f findSpawnPosition(World world, Vector2i pos, int searchRadius
         // nothing above sea level found
         for (Vector2ic test : spiral) {
             Optional<Float> val = getWorld.apply(test);
-            if (!val.isPresent()) {
-                continue;
+            if (val.isPresent()) {
+                return new Vector3f(test.x(), TeraMath.floorToInt(val.get()), test.y());
             }
-            return new Vector3f(test.x(), TeraMath.floorToInt(val.get()), test.y());
         }
 
         throw new IllegalStateException("No spawn location found");

diff --git a/engine/src/main/java/org/terasology/engine/network/internal/ServerConnectListManager.java b/engine/src/main/java/org/terasology/engine/network/internal/ServerConnectListManager.java
@@ -69,7 +69,7 @@ private void saveLists() {
                 whitelistWriter.close();
             }
         } catch (IOException e) {
-            e.printStackTrace();
+            logger.error("Couldn't save lists: ", e);
         }
     }
 

diff --git a/engine/src/main/java/org/terasology/engine/persistence/serializers/PrefabSerializer.java b/engine/src/main/java/org/terasology/engine/persistence/serializers/PrefabSerializer.java
@@ -182,8 +182,8 @@ private void applyCommonDataDelta(EntityData.Prefab delta, PrefabData result) {
     }
 
     private void deserializeCommonData(EntityData.Prefab prefabData, PrefabData result) {
-        result.setPersisted((prefabData.hasPersisted()) ? prefabData.getPersisted() : true);
-        result.setAlwaysRelevant(prefabData.hasAlwaysRelevant() ? prefabData.getAlwaysRelevant() : false);
+        result.setPersisted(!prefabData.hasPersisted() || prefabData.getPersisted());
+        result.setAlwaysRelevant(prefabData.hasAlwaysRelevant() && prefabData.getAlwaysRelevant());
         if (prefabData.hasParentName()) {
             Prefab parent = Assets.get(prefabData.getParentName(), Prefab.class).orElse(null);
             result.setParent(parent);

diff --git a/engine/src/main/java/org/terasology/engine/physics/bullet/shapes/BulletConvexHullShape.java b/engine/src/main/java/org/terasology/engine/physics/bullet/shapes/BulletConvexHullShape.java
@@ -20,8 +20,7 @@ public class BulletConvexHullShape extends BulletCollisionShape implements Conve
 
     public BulletConvexHullShape(List<Vector3f> vertices) {
         FloatBuffer buffer = BufferUtils.createFloatBuffer(vertices.size() * 3);
-        for (int i = 0; i < vertices.size(); i++) {
-            Vector3f vertex = vertices.get(i);
+        for (Vector3f vertex : vertices) {
             buffer.put(vertex.x);
             buffer.put(vertex.y);
             buffer.put(vertex.z);

diff --git a/engine/src/main/java/org/terasology/engine/rendering/assets/mesh/ObjMeshFormat.java b/engine/src/main/java/org/terasology/engine/rendering/assets/mesh/ObjMeshFormat.java
@@ -56,9 +56,9 @@ private StandardMeshData processData(List<Vector3f> rawVertices, List<Vector3f>
                                          List<Vector2f> rawTexCoords, List<Vector3i[]> rawIndices) throws IOException {
         int numIndices = 0;
         int numVerts = 0;
-        for (int x = 0; x < rawIndices.size(); x++) {
-            numIndices += (rawIndices.get(x).length - 2) * 3;
-            numVerts += rawIndices.get(x).length;
+        for (Vector3i[] rawIndex : rawIndices) {
+            numIndices += (rawIndex.length - 2) * 3;
+            numVerts += rawIndex.length;
         }
 
         StandardMeshData result = new StandardMeshData();

diff --git a/...e/src/main/java/org/terasology/engine/rendering/assets/skeletalmesh/SkeletalMeshData.java b/...e/src/main/java/org/terasology/engine/rendering/assets/skeletalmesh/SkeletalMeshData.java
@@ -17,6 +17,8 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 
 
 public class SkeletalMeshData implements AssetData {
@@ -172,10 +174,8 @@ public AABBf getStaticAABB() {
     private void calculateNormals() {
         // TODO: Better algorithm (take into account triangle size and angles
         List<Vector3f> vertices = getBindPoseVertexPositions();
-        List<Vector3f> normals = Lists.newArrayListWithCapacity(vertices.size());
-        for (int i = 0; i < vertices.size(); ++i) {
-            normals.add(new Vector3f());
-        }
+        List<Vector3f> normals = IntStream.range(0, vertices.size()).mapToObj(i -> new Vector3f()).collect(Collectors.toCollection(() ->
+                Lists.newArrayListWithCapacity(vertices.size())));
         Vector3f v1 = new Vector3f();
         Vector3f v2 = new Vector3f();
         Vector3f norm = new Vector3f();

diff --git a/engine/src/main/java/org/terasology/engine/rendering/assets/texture/AWTTextureFormat.java b/engine/src/main/java/org/terasology/engine/rendering/assets/texture/AWTTextureFormat.java
@@ -19,6 +19,7 @@
 import java.nio.ByteBuffer;
 import java.util.List;
 import java.util.function.Predicate;
+import java.util.stream.IntStream;
 
 import static org.terasology.gestalt.assets.module.ModuleAssetScanner.OVERRIDE_FOLDER;
 
@@ -79,34 +80,34 @@ public static TextureData convertToTextureData(final BufferedImage image, Textur
 
         // Convert AWT image to proper internal format
         if (image.getType() == BufferedImage.TYPE_3BYTE_BGR) {
-            for (int i = 0; i < data.length; i += stride) {
+            IntStream.iterate(0, i -> i < data.length, i -> i + stride).forEach(i -> {
                 buf.put(data, i + 2, 1); // R
                 buf.put(data, i + 1, 1); // G
                 buf.put(data, i + 0, 1); // B
                 buf.put((byte) 255); // A
-            }
+            });
         } else if (image.getType() == BufferedImage.TYPE_4BYTE_ABGR) {
-            for (int i = 0; i < data.length; i += stride) {
+            IntStream.iterate(0, i -> i < data.length, i -> i + stride).forEach(i -> {
                 buf.put(data, i + 3, 1); // R
                 buf.put(data, i + 2, 1); // G
                 buf.put(data, i + 1, 1); // B
                 buf.put(data, i + 0, 1); // A
-            }
+            });
         } else if (image.getType() == BufferedImage.TYPE_INT_ARGB) {
-            for (int i = 0; i < data.length; i += stride) {
+            IntStream.iterate(0, i -> i < data.length, i -> i + stride).forEach(i -> {
                 buf.put(data, i + 1, 1); // R
                 buf.put(data, i + 2, 1); // G
                 buf.put(data, i + 3, 1); // B
                 buf.put(data, i + 0, 1); // A
-            }
+            });
         } else if (image.getType() == BufferedImage.TYPE_BYTE_INDEXED) {
             final ColorModel cm = image.getColorModel();
-            for (int i = 0; i < data.length; i += stride) {
+            IntStream.iterate(0, i -> i < data.length, i -> i + stride).forEach(i -> {
                 buf.put((byte) cm.getRed(data[i])); // R
                 buf.put((byte) cm.getGreen(data[i])); // G
                 buf.put((byte) cm.getBlue(data[i])); // B
                 buf.put((byte) cm.getAlpha(data[i])); // A
-            }
+            });
         } else {
             throw new IOException("Unsupported AWT format: " + image.getType());
         }

diff --git a/...rc/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/UniverseSetupScreen.java b/...rc/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/UniverseSetupScreen.java
@@ -4,6 +4,8 @@
 
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.terasology.engine.config.Config;
 import org.terasology.engine.context.Context;
 import org.terasology.engine.core.GameEngine;
@@ -51,10 +53,10 @@
 import org.terasology.gestalt.entitysystem.component.Component;
 import org.terasology.gestalt.module.Module;
 import org.terasology.gestalt.module.ModuleEnvironment;
-import org.terasology.gestalt.module.exceptions.UnresolvedDependencyException;
 import org.terasology.gestalt.module.dependencyresolution.DependencyInfo;
 import org.terasology.gestalt.module.dependencyresolution.DependencyResolver;
 import org.terasology.gestalt.module.dependencyresolution.ResolutionResult;
+import org.terasology.gestalt.module.exceptions.UnresolvedDependencyException;
 import org.terasology.gestalt.naming.Name;
 import org.terasology.math.TeraMath;
 import org.terasology.nui.WidgetUtil;
@@ -96,6 +98,8 @@
 public class UniverseSetupScreen extends CoreScreenLayer implements UISliderOnChangeTriggeredListener, PropertyChangeListener {
     public static final ResourceUrn ASSET_URI = new ResourceUrn("engine:universeSetupScreen");
 
+    private static final Logger LOGGER = LoggerFactory.getLogger(UniverseSetupScreen.class);
+
     @In
     private WorldGeneratorManager worldGeneratorManager;
 
@@ -203,12 +207,10 @@ public WorldGeneratorInfo get() {
 
             @Override
             public void set(WorldGeneratorInfo worldGeneratorInfo) {
-                if (worldGeneratorInfo != null) {
-                    if (context.get(UniverseWrapper.class).getWorldGenerator() == null
-                            || !worldGeneratorInfo.getUri().equals(context.get(UniverseWrapper.class).getWorldGenerator().getUri())) {
-                        config.getWorldGeneration().setDefaultGenerator(worldGeneratorInfo.getUri());
-                        addNewWorld(worldGeneratorInfo);
-                    }
+                if (worldGeneratorInfo != null && (context.get(UniverseWrapper.class).getWorldGenerator() == null
+                        || !worldGeneratorInfo.getUri().equals(context.get(UniverseWrapper.class).getWorldGenerator().getUri()))) {
+                    config.getWorldGeneration().setDefaultGenerator(worldGeneratorInfo.getUri());
+                    addNewWorld(worldGeneratorInfo);
                 }
             }
         });
@@ -356,10 +358,10 @@ private void addNewWorld(WorldGeneratorInfo worldGeneratorInfo) {
         } catch (UnresolvedWorldGeneratorException e) {
             getManager().pushScreen(MessagePopup.ASSET_URI, MessagePopup.class).setMessage("Selected world generator cannot be resolved!",
                 "Please report this issue on Discord/GitHub and select a different world generator!");
-            e.printStackTrace();
+            LOGGER.error("Selected world generator cannot be resolved: ", e);
         } catch (UnresolvedDependencyException e) {
             //TODO: this will likely fail at game creation time later-on due to lack of world generator - don't just ignore this
-            e.printStackTrace();
+            LOGGER.error("Selected world generator cannot be resolved: ", e);
         }
 
         configureProperties();

diff --git a/engine/src/main/java/org/terasology/engine/rendering/opengl/OpenGLMeshBase.java b/engine/src/main/java/org/terasology/engine/rendering/opengl/OpenGLMeshBase.java
@@ -11,13 +11,13 @@ public interface OpenGLMeshBase {
 
     default boolean updateState(VBOContext state) {
         GL30.glBindBuffer(GL30.GL_ARRAY_BUFFER, state.vbo);
-        for (int x = 0; x < state.entries.length; x++) {
-            if (state.entries[x].version != state.entries[x].resource.getVersion()) {
-                if (state.entries[x].size != state.entries[x].resource.inSize()) {
+        for (VBOContext.VBOSubBuffer subBuffer : state.entries) {
+            if (subBuffer.version != subBuffer.resource.getVersion()) {
+                if (subBuffer.size != subBuffer.resource.inSize()) {
                     return false;
                 }
-                VertexResource resource = state.entries[x].resource;
-                int offset = state.entries[x].offset;
+                VertexResource resource = subBuffer.resource;
+                int offset = subBuffer.offset;
                 resource.writeBuffer((buffer) -> {
                     GL30.glBufferSubData(GL30.GL_ARRAY_BUFFER, offset, buffer);
                 });

diff --git a/engine/src/main/java/org/terasology/engine/world/time/WorldTimeImpl.java b/engine/src/main/java/org/terasology/engine/world/time/WorldTimeImpl.java
@@ -13,7 +13,6 @@
 import java.util.concurrent.atomic.AtomicLong;
 
 public class WorldTimeImpl extends BaseComponentSystem implements WorldTime, UpdateSubscriberSystem {
-
     private static final float WORLD_TIME_MULTIPLIER = 48f;
 
     private AtomicLong worldTime = new AtomicLong(0);
@@ -68,17 +67,14 @@ public void update(float delta) {
 
             if (startTick != endTick) {
                 long tick = endTime - endTime % TICK_EVENT_RATE;
-                getWorldEntity().send(new WorldTimeEvent(tick));
+                for (EntityRef e : entityManager.getEntitiesWith(WorldComponent.class)) {
+                    // this may not be called if there's no entity with World Component (yet)
+                    // TODO: consider catching / logging this case (someplace better than here)
+                    e.send(new WorldTimeEvent(tick));
+                }
             }
 
             // TODO: consider sending a DailyTick (independent from solar events such as midnight)
         }
     }
-
-    private EntityRef getWorldEntity() {
-        for (EntityRef entity : entityManager.getEntitiesWith(WorldComponent.class)) {
-            return entity;
-        }
-        return EntityRef.NULL;
-    }
 }
diff --git a/facades/PC/src/main/java/org/terasology/engine/Terasology.java b/facades/PC/src/main/java/org/terasology/engine/Terasology.java
@@ -318,6 +318,7 @@ private void populateSubsystems(TerasologyEngineBuilder builder) {
         builder.add(new HibernationSubsystem());
     }
 
+    @SuppressWarnings({"PMD.SystemPrintln", "PMD.AvoidPrintStackTrace"})
     private void reportException(Throwable throwable) {
         Path logPath = LoggingContext.getLoggingPath();