SonarCloud Code Quality

SonarCloud Code Quality (Sprint 4)

Overview

In Sprint 4, Team 4 cleaned up the SonarCloud issues in code written by Team 4. This adds no gameplay, but it removes a real crash path and makes the weapon, loot, player and cutscene code easier for every team to read.

• Feature: SonarCloud Code Quality (#409)
• Task #403 (Harshika): code smells in weapon, loot and cutscene code (15 issues). Delivered in PR #422, with one follow-up in #494.
• Task #410 (Pranshu): the reliability bug, plus the player and loot code smells (16 issues). Delivered in PRs #412 and #414.

Scope

Only issues on lines written by Team 4 members were in scope, which we checked with git blame on each flagged line. Starter template code and other teams' code were left alone, even in files we edited. Before changing a teammate's lines, we confirmed it with them.

Before and After

Team 4 code	Before (16 Oct)	After (19 Oct)
Bugs	1 (critical)	0
Code smells	30	0
Total	31	0

For context, the whole project went from Reliability D to C, from 4 to 2 open bugs, and from about 407 to 242 code smells over the same week. Those project-wide numbers include every team's cleanup, not only ours. The Team 4 bug fixed here (ConsumableGenerator) was one of the bugs holding the rating at D.

The Bug: ConsumableGenerator (S6416, critical)

Problem: The no-argument constructor passed FileLoader.readClass(...) straight into the other constructor. readClass returns null when configs/consumables.json is missing or cannot be parsed, so generation crashed with an unclear "ConsumableConfigs must not be null" error.

Fix: A new loadConfigs(path) method checks for null and throws an IllegalStateException that names the missing file.

Why fail instead of using default values? Falling back to defaults was tried first, but the default config values are not valid items (a heal amount of 0 is rejected by HealEffect), so the game would only crash later, when loot is generated. The config file ships with the game, so a missing file means a broken build, and failing straight away with a clear message is easier to debug.

Tests: shouldFailClearlyWhenConfigFileIsMissing and shouldLoadConfigsFromFileWhenItExists in ConsumableGeneratorTest.

Code Smells Fixed

Task #410: player and loot code

File	Rule	What was done	PR
LootPlacement.java	S9391 (use a stream)	Kept the loop, suppressed with a reason (see below)	#412
PlayerBuffComponent.java	S9391 (use a stream)	Kept the loop, suppressed with a reason	#412
ConsumableUseComponentTest.java	S1612 (lambda to method reference)	Replaced with a typed method reference	#412
WeaponUpgradeEffectTest.java	S1612 (lambda to method reference)	Replaced with a typed method reference	#412
ConsumableItem.java	S2160 (override equals)	Suppressed with a reason: items are equal only when their unique ids match, which is inherited from Item on purpose	#412
PlayerActions.java (5 lines)	S106 (System.out)	Moved to the class's logger: debug for normal messages, error for a missing physics body	#414
PlayerActions.java	S1192 (duplicated literal)	"bribeNoTarget" is now the constant BRIBE_NO_TARGET_EVENT	#414
PlayerActions.java	S116 (field naming)	SlideTimer renamed to slideTimer. CrouchSpeedRate had already been renamed on main	#414
PlayerRenderComponent.java	S1185 (useless override)	Removed create(), which only called super.create()	#414
LootPickupComponentTest.java	S5785 (use assertNotNull)	assertFalse(x == null) replaced with assertNotNull(x)	#414

The PlayerActions changes were kept in their own small PR (#414), because many branches edit that file.

Task #403: weapon, loot and cutscene code

File	Rule	What was done	PR
WeaponDisplay.java	S1186 (empty method)	Documented why draw is empty	#422
SpecialAttackCooldownDisplay.java	S1186 (empty method)	Documented why draw is empty	#422
Item.java	S6885 (use Math.clamp)	Nested Math.max / Math.min replaced with Math.clamp	#422
ArrowFactory.java	S1192 (duplicated literal)	Already fixed on main before #422	-
WeaponAttackComponentTest.java	S5778 (one call in assertThrows)	Lambda reduced to one call that can throw	#422
WeaponItemTest.java	S5778 (one call in assertThrows)	Lambda reduced to one call that can throw	#422
ArrowMovementComponent.java	S1450 (field to local)	body is now a local variable in create()	#422
WeaponItem.java	S2160 (override equals)	Overrides equals / hashCode, delegating to Item	#422
ArrowMovementComponentTest.java	S6068 (needless eq(...))	Removed eq(...)	#422
TextureRenderComponentTest.java	S6068 (needless eq(...))	Removed eq(...)	#422
ItemDropComponent.java	S1192 (duplicated literal)	"itemDropped" is now a constant	#422
WeaponRenderComponent.java	S1144 (unused method)	Removed the unused throwDagger method	#422
StoryCutsceneDisplay.java	S1450 (field to local)	panel is now a local variable	#422
StoryCutsceneScreen.java	S1450 (field to local)	darkOverlay is now a local variable	#422
BobbingTextureRenderComponent.java	S9395 (explicit cast)	maxDimension kept as an int, so there is no silent int-to-float conversion. This was listed in #422 but missing from its diff. A (float) cast was tried first, but SonarCloud flagged it as unnecessary (S1905), so #494 avoids the conversion instead	#494

Issues We Chose Not to "Fix"

Three issues were closed with @SuppressWarnings and a short explanation, instead of changing the code the way SonarCloud suggests:

• S9391 "use a stream instead of this loop" (LootPlacement.forSpawnPoints, PlayerBuffComponent.update). A plain for loop is easier to read and explain than a stream pipeline here, and the loops are short.
• S2160 "override equals" (ConsumableItem). Item.equals compares unique ids on purpose, so two potions with the same name are still different items in the inventory. Overriding it would change nothing.

The #410 ticket planned to mark the S9391 issues "Won't Fix" in the SonarCloud dashboard. We suppressed them in the code instead, so the reason sits next to the code where the next person will read it, and SonarCloud closes them automatically.

Remaining Issues (Out of Scope)

Some files we edited still have open SonarCloud issues, but on lines written by other teams, so we left them for their owners:

• PlayerActions.java: Speed and the sound-effect constants (WALKING_SE, JUMP_SE, ...) break the naming rule (S116)
• PlayerRenderComponent.java: unused playerAtlas field (S1068)
• WeaponDisplay.java: %n instead of \n in String.format (S3457)
• WeaponRenderComponent.java: missing default case (S131) and unused parameter (S1172)
• ArrowFactory.java: createProjectileBase has 11 parameters (S107)

Checking the Results

1. Open the SonarCloud dashboard and go to Issues.
2. Filter by File for any file in the tables above. Our issues show as Closed (Fixed).
3. Every PR also gets a SonarCloud comment with its own Quality Gate result.

All changes keep behaviour the same. ./gradlew test and spotlessCheck passed on every PR.