refactor: address IntelliJ QAPlug plugin findings #5149

jdrueckert · 2023-10-22T15:20:05Z

make final fields static
remove unnecessary local before return
remove instanceof checks in catch clause
avoid throwing java.lang.Exception
remove null checks covered by instanceof check
remove toString on String objects
remove overrides that only call their super
remove unused private methods
remove unnecessary if statement nesting
remove unused local variables
use try-with-resources to close resources after use
use equals() to compare object references
fix broken javadoc references
fix disallowed self-closing and empty javadoc elements
fix bad use of symbols in javadoc
fix disallowed list item tag in javadoc without surrounding list
fix erroneous javadoc tags
fix emphasize tags in javadoc
remove illegal throws
remove unused imports

Related:
Terasology/CoreRendering#75
Terasology/Furnishings#17
Terasology/Health#105
Terasology/Inventory#51
Terasology/Behaviors#114
Terasology/CoreWorlds#45
Terasology/FlexiblePathfinding#32

Adds to #3859

soloturn

walked through the single commits to understand reasoning better. looks all ok to me. as the PR is big, it should be merged immediate as rebase etc is not so easy, in my opinion.

- add pmd suppression for UselessOverridingMethod rule

soloturn · 2023-10-28T05:31:01Z

engine/src/main/java/org/terasology/engine/rendering/nui/CoreScreenLayer.java

@@ -80,6 +80,12 @@ public void setDepthAuto() {
        }
    }

+    @Override
+    @SuppressWarnings("PMD.UselessOverridingMethod") // override elevates access modifier


what does "elevates access modifier" mean in this context, resp why one want to have it here?

The method is only calling its super and thus wouldn't be required, but the super method is protected while the overriding method is public, thereby elevating the accessibility of the method. Without the override, package-external entities would not be able to call the (super) method.

If we look at the jenkins build error in https://jenkins.terasology.io/job/Terasology/job/engine/job/PR-5149/3/console that's exactly the problem we see:

/home/jenkins/agent/workspace/Terasology_engine_PR-5149/engine/src/main/java/org/terasology/engine/rendering/nui/internal/NUIManagerInternal.java:421: error: setId(String) has protected access in AbstractWidget screen.setId(uri.toString()); ^

NUIManagerInternal needs to call this method but is not able to without the override that allows the method to be called by package-external entities. Hence we need the override as it elevates the access modifier.

Note: Whether NUIManagerInternal should call this method or whether the super method should be public in the first place is a valid question but it goes beyond this refactoring here and I consciously decide to ignore this potential rabbit hole for now.

I think it can make sense to have some methods protected on an interface/super class, and exposing them in sub-classes. For instance, if implementations are not in the same package but are supposed to handle internals.

Whether or not it makes sense here in particular is something we could double check again.

skaldarnar

Went through all the commits now and left some comments (please split PRs like this into smaller chunks, maybe 5 commits at max, to allow for faster reviews).

skaldarnar · 2023-10-29T21:52:55Z

engine/src/main/java/org/terasology/engine/network/internal/ClientConnectionHandler.java

@@ -34,6 +34,7 @@
 public class ClientConnectionHandler extends ChannelInboundHandlerAdapter {

    private static final Logger logger = LoggerFactory.getLogger(ClientConnectionHandler.class);
+    private static final long timeoutThreshold = 120000;


Should these be names with all-caps then (TIMEOUT_THRESHOLD)?

for the logger we don't really apply the all-caps constant practice anywhere. if we want to do it consistently, then we should probably do it for the logger as well, right?

I'll not do this within this PR, but we can start refactoring accordingly on everything we actively work on if we think it makes sense

I think the all caps notation is specifically used for constant values like this threshold.

I personally think that just being static and final does not make something a constant as in mathematics, so I'm in favor of not being too strict about consistency for this. I prefer logger.info(...) over LOGGER.info(...), but on the other hand would prefer wait(TIMEOUT) over wait(timeout).

skaldarnar · 2023-10-29T21:56:27Z

engine/src/main/java/org/terasology/engine/logic/characters/CharacterSystem.java

-            DisplayNameComponent displayNameComponent = instigatorClientInfo.getComponent(DisplayNameComponent.class);
-            return displayNameComponent.name;
+            return instigatorClientInfo.getComponent(DisplayNameComponent.class).name;


Hm, I'm not sure about this one. Technically it is possible, yes, but I don't like that there are so many .s in that one line.

On the other hand, it is in line with the two statements above, where we also do

someObject.getComponent(MyComponent.calss).someField;

I'll leave it as is for now.

skaldarnar · 2023-10-29T22:06:30Z

engine/src/main/java/org/terasology/engine/logic/chat/ChatSystem.java

-            if (message.getType() == CoreMessageType.CHAT || message.getType() == CoreMessageType.NOTIFICATION) {
-
+            if (message.getType() == CoreMessageType.CHAT || message.getType() == CoreMessageType.NOTIFICATION
+                    && !nuiManager.isOpen(CHAT_UI) && !nuiManager.isOpen(CONSOLE_UI)) {
                // show overlay only if chat and console are hidden
-                if (!nuiManager.isOpen(CHAT_UI) && !nuiManager.isOpen(CONSOLE_UI)) {
-                    overlay.setVisible(true);
-                }


I know that I sometimes use constructs like this to put emphasis on the logical control flow, trusting in the compiler to be smart about the details.

I find long if conditions with many parts hard to read and to understand, and one has to put quite a lot thought into breaking it up when changing the control flow.

Therefore, I'm not a huge fan of some these changes in particular, but I'm not going to argue about this. Let's see how it plays out.

I'll leave it as is for now

skaldarnar · 2023-10-29T22:08:12Z

engine/src/main/java/org/terasology/engine/input/lwjgl/LwjglControllerDevice.java

@@ -92,6 +92,7 @@ private void updateGamepadMappings() {
            if (!GLFW.glfwUpdateGamepadMappings(gamecontrolleDB)) {
                logger.error("Cannot update GLFW's gamepad mapping, gamepads may not work correctly");
            }
+            inputStream.close();


Use try with resources instead of manually closing. This hopefully applies to most of the occurrences of this warning.

skaldarnar · 2023-10-29T22:12:45Z

engine/src/main/java/org/terasology/engine/rendering/nui/CoreScreenLayer.java

@@ -80,6 +80,12 @@ public void setDepthAuto() {
        }
    }

+    @Override
+    @SuppressWarnings("PMD.UselessOverridingMethod") // override elevates access modifier


I think it can make sense to have some methods protected on an interface/super class, and exposing them in sub-classes. For instance, if implementations are not in the same package but are supposed to handle internals.

Whether or not it makes sense here in particular is something we could double check again.

skaldarnar · 2023-10-29T22:14:14Z

engine/src/main/java/org/terasology/engine/persistence/typeHandling/RegisterTypeHandler.java

@@ -11,8 +11,8 @@
 import java.lang.annotation.Target;

 /**
- * Marks a {@link TypeHandler} to be automatically registered at a
- * {@link TypeHandlerLibrary TypeHandlerLibrary} on environment change.
+ * Marks a {@link org.terasology.persistence.typeHandling.TypeHandler} to be automatically registered at a


Is this really necessary? I thought that JavaDoc would be smart enough to reuse imports and allow for shorthand links within the same package 🤔 How does this render?

javadoc complained about an unknown reference and auto-generated the long version when I re-added it 🤷‍♀️

how do you "auto-generate" something with javadoc?

Generally in an IDE like IntelliJ if you start by entering /** atop a method then IntelliJ will generate the whole Javadoc block (if you shift-enter or something?) including defined parameters etc. There are probably similar tricks when you have other interactions with Javadoc.

Plus the whole copilot madness which can just be eerily close to generating a pile of stuff correctly or oh-so-far from it at times :-)

skaldarnar · 2023-10-29T22:15:09Z

engine/src/main/java/org/terasology/engine/world/block/family/AbstractBlockFamily.java

-    }
-
-    protected AbstractBlockFamily(BlockFamilyDefinition definition, BlockBuilderHelper blockBuilder) {
+    protected AbstractBlockFamily(BlockFamilyDefinition definition) {


change unrelated to the commit message - is that on purpose?

yes and no - I saw that blockBuilder isn't used and wanted to change something about it but then I thought I'd ask soundwave about the intent of the changes first - still waiting to hear back... wanted to put the change in a separate PR, but seems they snuck their way in here unfortunately...

skaldarnar · 2023-10-29T22:17:53Z

...c/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/savedGames/GameProvider.java

-     *     1. "Game"    -> "Game"
-     *     2. "Game"    -> "Game 1"
-     *     3. "Game 1"  -> "Game 2"


Not even here (in <pre>)? 😔 Maybe needs another {@code ... or so, never understood how that really works...

skaldarnar · 2023-10-29T22:20:25Z

engine/src/main/java/org/terasology/engine/world/chunks/blockdata/ExtraBlockDataManager.java

@@ -33,9 +33,9 @@
 * whether the field is applicable for that block. For example:
 *
 * <pre>
- * @ExtraDataSystem
+ * {@code @ExtraDataSystem}


I think you can put the {@code right next to the <pre> and close it only in the end.

nope, {@code ...} (same as other @... tags) unfortunately doesn't work across multiple lines.

- fails storage mananager tests -> needs to be investigated in more detail

jdrueckert added 12 commits October 22, 2023 15:38

qa: make final fields static

aa6ca30

qa: remove unnecessary local before return

1a53322

qa: avoid instanceof checks in catch clause

db4f626

qa: method/constructor should not throw java.lang.Exception

e3d80fb

qa: instanceof check covers null check

f22c1fa

qa: avoid toString on String objects

9d7204b

qa: remove overrides that only call their super

993b370

qa: remove unused private methods

315ef35

qa: if statement nesting not necessary

3c5d507

qa: remove unused local variable

df3539a

qa: close resources after use

b0ec929

qa: use equals() to compare object references

06c5fcf

jdrueckert added this to the 2023 Revive - Milestone 2 milestone Oct 22, 2023

soloturn approved these changes Oct 27, 2023

View reviewed changes

jdrueckert added 4 commits October 27, 2023 10:10

Merge branch 'develop' into chore/qaplug-findings

3428369

fix: reintroduce override, needed to elevate access modifier

a3f8fbc

- add pmd suppression for UselessOverridingMethod rule

fix: exception type in override

760a174

chore: replace invalid javadoc character with unicode representation

f067ba2

soloturn reviewed Oct 28, 2023

View reviewed changes

javadoc: fix broken references

077d488

jdrueckert added 9 commits October 28, 2023 12:53

javadoc: fix disallowed self-closing and empty elements

6a816ed

javadoc: fix invalid uses of @param and @return

a27bd8e

javadoc: fix bad use of symbols

c93ad63

javadoc: fix disallowed list item tag without surrounding list

ad27ad0

javadoc: fix erroneous tags

cce1a8b

javadoc: fix more references

45e6571

javadoc: fix emphasize tags

35c2bc3

chore: fix missing javadoc errors

60a41a5

javadoc: fix @see

3d13e1f

skaldarnar reviewed Oct 29, 2023

View reviewed changes

jdrueckert added 8 commits October 30, 2023 11:23

checkstyle: remove illegal throws

848947e

checkstyle: remove unused imports

f585450

refactor: replace explicit close with try-with-resources

01e8edd

revert: removal of unused param from AbstractFamily constructor

b841271

revert: close zip Filesystem

2aad50f

- fails storage mananager tests -> needs to be investigated in more detail

revert: removing required zip.close

49725fa

Merge branch 'develop' into chore/qaplug-findings

bf26781

Merge branch 'develop' into chore/qaplug-findings

971c853

skaldarnar approved these changes Nov 8, 2023

View reviewed changes

Merge branch 'develop' into chore/qaplug-findings

08bd7ea

jdrueckert merged commit 1cef7c1 into develop Nov 11, 2023
9 checks passed

jdrueckert deleted the chore/qaplug-findings branch November 11, 2023 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: address IntelliJ QAPlug plugin findings #5149

refactor: address IntelliJ QAPlug plugin findings #5149

jdrueckert commented Oct 22, 2023 •

edited

Loading

soloturn left a comment •

edited

Loading

soloturn Oct 28, 2023

jdrueckert Oct 28, 2023

skaldarnar Oct 29, 2023

skaldarnar left a comment

skaldarnar Oct 29, 2023

jdrueckert Oct 30, 2023

jdrueckert Oct 30, 2023

skaldarnar Nov 8, 2023

skaldarnar Oct 29, 2023

jdrueckert Oct 30, 2023

skaldarnar Oct 29, 2023

jdrueckert Oct 30, 2023

skaldarnar Oct 29, 2023

jdrueckert Oct 30, 2023

skaldarnar Oct 29, 2023

skaldarnar Oct 29, 2023

jdrueckert Oct 30, 2023

soloturn Nov 3, 2023

Cervator Nov 3, 2023

skaldarnar Oct 29, 2023

jdrueckert Oct 30, 2023

jdrueckert Oct 30, 2023

skaldarnar Oct 29, 2023

skaldarnar Oct 29, 2023

jdrueckert Oct 30, 2023

refactor: address IntelliJ QAPlug plugin findings #5149

refactor: address IntelliJ QAPlug plugin findings #5149

Conversation

jdrueckert commented Oct 22, 2023 • edited Loading

soloturn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert commented Oct 22, 2023 •

edited

Loading

soloturn left a comment •

edited

Loading