refactor: apply Clean Code principles to Medieval-Roleplay-Engine#316
Open
refactor: apply Clean Code principles to Medieval-Roleplay-Engine#316
Conversation
- Fix == String comparison bug in Messenger (world name check) - Fix typo neurtalAlertColor → neutralAlertColor in ConfigService - Fix wrong permission name 'rp.yell' → 'rp.localOOC' in LocalOOCChatCommand error message - Rename setName(String newUUID) → setName(String newName) in CharacterCard - Fix spelling astericks → asterisks in ChatListener private methods and variables - Extract BSTATS_PLUGIN_ID constant in MedievalRoleplayEngine - Extract RIGHT_CLICK_COOLDOWN_TICKS constant in InteractionListener - Remove commented-out code in StorageService - Remove all redundant inline comments across files - DRY: Extract applyStringCardChange + CardFieldSetter + findCardForPlayer helpers in CardCommand, collapsing duplicate changeRace/changeSubculture/changeReligion/changeGender bodies - DRY: Extract deliverMessageToNearbyPlayers + logChatIfEnabled helpers in Messenger, eliminating three near-identical send loops - Apply guard-clause pattern (early return) for non-Player checks in CardCommand methods Agent-Logs-Url: https://github.com/Dans-Plugins/Medieval-Roleplay-Engine/sessions/9133244a-5949-4098-a419-46fc02fb9a89 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Dans-Plugins/Medieval-Roleplay-Engine/sessions/9133244a-5949-4098-a419-46fc02fb9a89 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
dmccoystephenson
April 26, 2026 01:07
View session
There was a problem hiding this comment.
Pull request overview
Clean-code refactor pass across MedievalRoleplayEngine focused on reducing duplication, fixing a few correctness issues, and improving naming/clarity in chat + card command flows.
Changes:
- Refactors
MessengerandCardCommandto consolidate repeated logic and reduce nesting via guard clauses/helpers. - Fixes a config default key typo (
neutralAlertColor) and multiple small correctness/usability issues in command feedback. - Replaces magic numbers with named constants and removes redundant comments/dead debug code.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/dansplugins/rpsystem/utils/Messenger.java | Consolidates nearby-player delivery loops and chat logging into shared helpers. |
| src/main/java/dansplugins/rpsystem/storage/StorageService.java | Removes redundant comments and renames variables during card-load dedupe logic. |
| src/main/java/dansplugins/rpsystem/listeners/InteractionListener.java | Replaces right-click cooldown magic number with a named ticks constant. |
| src/main/java/dansplugins/rpsystem/listeners/ChatListener.java | Spelling fixes around “asterisk” helpers and minor cleanup in emote parsing. |
| src/main/java/dansplugins/rpsystem/config/ConfigService.java | Fixes neutralAlertColor config key typo and removes redundant comments. |
| src/main/java/dansplugins/rpsystem/commands/yell/YellCommand.java | Removes redundant inline comment. |
| src/main/java/dansplugins/rpsystem/commands/whisper/WhisperCommand.java | Removes redundant inline comment. |
| src/main/java/dansplugins/rpsystem/commands/title/TitleCommand.java | Removes redundant inline comments. |
| src/main/java/dansplugins/rpsystem/commands/localooc/LocalOOCChatCommand.java | Fixes permission name referenced in the denial message. |
| src/main/java/dansplugins/rpsystem/commands/local/LocalChatCommand.java | Removes redundant inline comment. |
| src/main/java/dansplugins/rpsystem/commands/config/ConfigCommand.java | Removes redundant inline comments. |
| src/main/java/dansplugins/rpsystem/commands/card/CardCommand.java | Applies guard clauses + DRY helper for string field updates; restructures several subcommands. |
| src/main/java/dansplugins/rpsystem/commands/bird/BirdCommand.java | Removes redundant inline comments. |
| src/main/java/dansplugins/rpsystem/commands/CommandService.java | Removes redundant inline comments. |
| src/main/java/dansplugins/rpsystem/cards/CharacterCard.java | Fixes misleading setName parameter naming and removes redundant comments. |
| src/main/java/dansplugins/rpsystem/MedievalRoleplayEngine.java | Introduces a named constant for the bStats plugin id and removes redundant comments. |
|
|
||
| Player player = (Player) sender; | ||
| if (!(player.hasPermission("rp.card.lookup") || player.hasPermission("rp.card.*") || player.hasPermission("rp.default"))) { | ||
| player.sendMessage(medievalRoleplayEngine.colorChecker.getNegativeAlertColor() + "Sorry! In order to use this command, you need the following permission: 'rp.card.show.others'"); |
There was a problem hiding this comment.
The permission check is for rp.card.lookup, but the denial message tells the user they need rp.card.show.others. Update the message to reference the actual required permission node so players/admins can resolve the issue correctly.
Suggested change
| player.sendMessage(medievalRoleplayEngine.colorChecker.getNegativeAlertColor() + "Sorry! In order to use this command, you need the following permission: 'rp.card.show.others'"); | |
| player.sendMessage(medievalRoleplayEngine.colorChecker.getNegativeAlertColor() + "Sorry! In order to use this command, you need the following permission: 'rp.card.lookup'"); |
| if (nearbyLocation.distance(senderLocation) >= distance) { | ||
| continue; | ||
| } | ||
| if (excludeSender && nearby.getName().equalsIgnoreCase(sender.getName())) { |
There was a problem hiding this comment.
deliverMessageToNearbyPlayers(...) excludes the sender by comparing player names (nearby.getName().equalsIgnoreCase(sender.getName())). Use UUID comparison instead (e.g., nearby.getUniqueId().equals(sender.getUniqueId())) to avoid relying on name casing/identity and to make the intent unambiguous.
Suggested change
| if (excludeSender && nearby.getName().equalsIgnoreCase(sender.getName())) { | |
| if (excludeSender && nearby.getUniqueId().equals(sender.getUniqueId())) { |
| } | ||
| return false; | ||
| private CharacterCard findCardForPlayer(UUID playerUUID) { | ||
| return medievalRoleplayEngine.cardRepository.getCard(playerUUID); |
There was a problem hiding this comment.
findCardForPlayer() delegates to cardRepository.getCard(...), which currently calls card.getPlayerUUID().equals(uuid) without guarding against card.getPlayerUUID() being null. Since the repository can contain cards with null UUIDs (e.g., cards skipped during saveCardFileNames()), this lookup can throw an NPE. Make the lookup null-safe (either here or in CardRepository#getCard) so commands don't crash when malformed/partially-loaded cards exist.
Suggested change
| return medievalRoleplayEngine.cardRepository.getCard(playerUUID); | |
| if (playerUUID == null) { | |
| return null; | |
| } | |
| try { | |
| return medievalRoleplayEngine.cardRepository.getCard(playerUUID); | |
| } | |
| catch (NullPointerException exception) { | |
| return null; | |
| } |
Comment on lines
+66
to
+73
| int existingIndex = -1; | ||
| for (int i = 0; i < medievalRoleplayEngine.cardRepository.getCards().size(); i++) { | ||
| if (medievalRoleplayEngine.cardRepository.getCards().get(i).getPlayerUUID().equals(temp.getPlayerUUID())) { | ||
| index = i; | ||
| existingIndex = i; | ||
| } | ||
| } | ||
| if (index != -1) { | ||
| medievalRoleplayEngine.cardRepository.getCards().remove(index); | ||
| if (existingIndex != -1) { | ||
| medievalRoleplayEngine.cardRepository.getCards().remove(existingIndex); |
There was a problem hiding this comment.
loadCards() can throw a NullPointerException when comparing UUIDs: both getCards().get(i).getPlayerUUID() and/or temp.getPlayerUUID() can be null (cards with null UUIDs are possible elsewhere in this class, e.g., saveCardFileNames() skips them). Add null checks (or use Objects.equals) and consider skipping/ignoring loaded cards with missing UUIDs to avoid crashing card load on malformed/empty files.
| Player player = (Player) sender; | ||
|
|
||
| if (player.hasPermission("rp.card.show") || player.hasPermission("rp.card.*") || player.hasPermission("rp.default")) { | ||
| CharacterCard card = medievalRoleplayEngine.cardLookupService.lookup(player.getUniqueId()); |
There was a problem hiding this comment.
showCard() passes the result of cardLookupService.lookup() directly into sendCardInfoToPlayer(...) without a null check. Since lookup() can return null for players without a card, this will cause an NPE; send an error message and return when the lookup fails.
Suggested change
| CharacterCard card = medievalRoleplayEngine.cardLookupService.lookup(player.getUniqueId()); | |
| CharacterCard card = medievalRoleplayEngine.cardLookupService.lookup(player.getUniqueId()); | |
| if (card == null) { | |
| player.sendMessage(medievalRoleplayEngine.colorChecker.getNegativeAlertColor() + "You do not have a character card to view."); | |
| return; | |
| } |
Member
|
@copilot You are a coding agent tasked with resolving all open comments and review threads Instructions1. Discover all comments
2. For each comment, do the following:
3. Types of issues to handle:
4. Do NOT:
After making changes
Context
|
- CardCommand.showPlayerInfo: fix denial message to reference 'rp.card.lookup' (was 'rp.card.show.others') - CardCommand.showCard: add null check on lookup result before calling sendCardInfoToPlayer - Messenger.deliverMessageToNearbyPlayers: use UUID comparison to exclude sender (was name comparison) - CardRepository.getCard/hasCard: guard against null card UUID to prevent NPE - StorageService.loadCards: skip cards with null UUID after load; use Objects.equals for deduplication to prevent NPE Agent-Logs-Url: https://github.com/Dans-Plugins/Medieval-Roleplay-Engine/sessions/a1b8cfa8-b4f0-45aa-b692-fe13b62c6a64 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Contributor
Author
All five review threads addressed in commit
|
| @@ -62,7 +60,7 @@ public void handleVersionMismatch() { | |||
| if (!getConfig().isString("positiveAlertColor")) { | |||
| getConfig().addDefault("positiveAlertColor", "green"); | |||
| } | |||
There was a problem hiding this comment.
neutralAlertColor is treated as a brand-new key during version-mismatch handling. If existing servers previously had the misspelled neurtalAlertColor set, their customized value will be ignored and replaced by the default. Consider adding a one-time migration: if neurtalAlertColor exists and neutralAlertColor does not, copy the value over (and optionally delete the old key) before applying defaults.
Suggested change
| } | |
| } | |
| if (!getConfig().isString("neutralAlertColor") && getConfig().isString("neurtalAlertColor")) { | |
| getConfig().set("neutralAlertColor", getConfig().getString("neurtalAlertColor")); | |
| getConfig().set("neurtalAlertColor", null); | |
| } |
| card.setName(medievalRoleplayEngine.argumentParser.createStringFromFirstArgOnwards(args, 1)); | ||
| player.sendMessage(medievalRoleplayEngine.colorChecker.getPositiveAlertColor() + "Name set! Type /card to see changes."); | ||
|
|
||
| int changeNameCooldown = medievalRoleplayEngine.getConfig().getInt("changeNameCooldown"); |
There was a problem hiding this comment.
changeName reads changeNameCooldown via medievalRoleplayEngine.getConfig(). Elsewhere the plugin exposes ConfigService helpers and the repo guidelines prefer using configService instead of accessing getConfig() directly. Switching to medievalRoleplayEngine.configService.getInt("changeNameCooldown") would keep config access consistent and centralized.
Suggested change
| int changeNameCooldown = medievalRoleplayEngine.getConfig().getInt("changeNameCooldown"); | |
| int changeNameCooldown = medievalRoleplayEngine.configService.getInt("changeNameCooldown"); |
Member
|
@copilot address comments |
- ConfigService: migrate old misspelled 'neurtalAlertColor' key to 'neutralAlertColor' on version mismatch - CardCommand: use configService.getInt() instead of getConfig().getInt() for changeNameCooldown Agent-Logs-Url: https://github.com/Dans-Plugins/Medieval-Roleplay-Engine/sessions/cd131eca-1c4b-4f24-abbf-bfc820b03e58 Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Contributor
Author
Both new review comments addressed in commit
|
Member
|
@copilot review for any potential breaking changes and resolve |
Contributor
Author
Reviewed all PR changes — no breaking changes found. Key points:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
==String comparison (world name) inMessenger.java; fix typo"neurtalAlertColor"inConfigService; fix wrong permission name'rp.yell'inLocalOOCChatCommanderror messagesetName(String newUUID)→setName(String newName)inCharacterCard; fix spellinggetStringContainedBetweenAstericks→getStringContainedBetweenAsterisksinChatListenerBSTATS_PLUGIN_IDinMedievalRoleplayEngine; extractRIGHT_CLICK_COOLDOWN_TICKSinInteractionListenerStorageService; remove redundant inline comments across all filesapplyStringCardChange+CardFieldSetter+findCardForPlayerhelpers inCardCommand; extractdeliverMessageToNearbyPlayers+logChatIfEnabledhelpers inMessengerMessenger.deliverMessageToNearbyPlayersshowPlayerInfodenial message to say'rp.card.lookup'cardLookupService.lookup()result inshowCarddeliverMessageToNearbyPlayersto exclude senderCardRepository.getCard/hasCardnull-safe against cards with null UUIDsObjects.equalsinStorageService.loadCardsneurtalAlertColorkey toneutralAlertColorinConfigService.handleVersionMismatchconfigService.getInt()forchangeNameCooldowninCardCommand.changeName