Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hub: Code quality, enhancments and better maintainability #23

Open
wants to merge 15 commits into
base: staging
Choose a base branch
from
Open

Hub: Code quality, enhancments and better maintainability #23

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
6c4f0df
version number prints at server startup instead of ${plugin.version} …
palsmo Dec 31, 2024
d28121f
Code quality, enhancments and better maintainability
palsmo Jan 1, 2025
cff0dfd
join message should not be delayed
palsmo Jan 1, 2025
9a23f5b
Extracted welcome sounds into its own class & wrote a friendly welcom…
palsmo Jan 1, 2025
f387bdd
Utility class should not be instantiated
palsmo Jan 1, 2025
841390b
Better exception message.
palsmo Jan 1, 2025
6388477
Fixed good integration with PremiumVanish, server will have uniform j…
palsmo Jan 2, 2025
f19ee38
Uncomment dependencies
palsmo Jan 2, 2025
b41d678
Using 'isConnected()' is most appropriate here, would correcly solve …
palsmo Jan 2, 2025
0c15e01
Added check for if the firework would have been despawned etc. before…
palsmo Jan 2, 2025
82eb9c6
Moved fields to top of class, as is standard.
palsmo Jan 2, 2025
f2649af
Small simplifications, added checks for functions that require their …
palsmo Jan 3, 2025
98183d0
Fixed proper integration 'config.yml' + spilled out to a few other fi…
palsmo Jan 5, 2025
0440e83
Removed debug code accidentally left behind
palsmo Jan 5, 2025
a07194f
Proper mirroring of resource files, new configs 'slot_hub_compass' & …
palsmo Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,10 @@
.idea
/target
/.idea
*.iml
*.iml

# jdtls generation
.classpath
.factorypath
.project
.settings/
12 changes: 10 additions & 2 deletions zander-hub/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,15 @@
<artifactId>zander-hub</artifactId>
<version>1.0</version>

<build>
<resources>
<resource>
<directory>src/main/resources</directory>
<filtering>true</filtering>
</resource>
</resources>
</build>

<repositories>
<!-- PaperMC/WaterFall -->
<repository>
Expand All @@ -29,5 +38,4 @@
<scope>provided</scope>
</dependency>
</dependencies>

</project>
</project>
9 changes: 5 additions & 4 deletions zander-hub/src/main/java/org/modularsoft/zander/hub/ZanderHubMain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ public void onEnable() {
plugin = this;

this.getServer().getMessenger().registerOutgoingPluginChannel(this, "BungeeCord");
// this.getServer().getMessenger().registerIncomingPluginChannel(this, "BungeeCord", new PluginMessageChannel(this));
// this.getServer().getMessenger().registerIncomingPluginChannel(this,
// "BungeeCord", new PluginMessageChannel(this));

// Init Message
TextComponent enabledMessage = Component.empty()
.color(NamedTextColor.GREEN)
.append(Component.text("\n\nZander Hub has been enabled.\n"))
// .append(Component.text("Running Version " + ZanderHubMain.class.getPackage().getImplementationVersion() + "\n"))
.append(Component.text("Running Version " + plugin.getDescription().getVersion() + "\n"))
.append(Component.text("Running Version " + plugin.getPluginMeta().getVersion() + "\n"))
.append(Component.text("GitHub Repository: https://github.com/ModularSoftAU/zander\n"))
.append(Component.text("Created by Modular Software\n\n", NamedTextColor.DARK_PURPLE));
getServer().sendMessage(enabledMessage);
Expand All @@ -56,5 +56,6 @@ public void onEnable() {
}

@Override
public void onDisable() {}
public void onDisable() {
}
}
231 changes: 177 additions & 54 deletions zander-hub/src/main/java/org/modularsoft/zander/hub/events/HubPlayerJoin.java
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could make the compass slot and such a configuration option in the config yml instead of setting it static.

Original file line number Diff line number Diff line change
Expand Up @@ -2,96 +2,219 @@

import net.kyori.adventure.text.Component;
import net.kyori.adventure.text.format.NamedTextColor;
import org.modularsoft.zander.hub.ConfigurationManager;
import org.modularsoft.zander.hub.ZanderHubMain;
import org.modularsoft.zander.hub.items.NavigationCompassItem;
import net.md_5.bungee.api.ChatColor;
import org.bukkit.Bukkit;
import org.bukkit.Color;
import org.bukkit.FireworkEffect;
import org.bukkit.Location;
import org.bukkit.Sound;
import org.bukkit.entity.Firework;
import org.bukkit.entity.Player;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;
import org.bukkit.event.player.PlayerJoinEvent;
import org.bukkit.event.player.PlayerLoginEvent;
import org.bukkit.inventory.meta.FireworkMeta;
import org.bukkit.metadata.MetadataValue;
import org.bukkit.scoreboard.Scoreboard;
import org.bukkit.scoreboard.Team;
import org.modularsoft.zander.hub.ConfigurationManager;
import org.modularsoft.zander.hub.ZanderHubMain;
import org.modularsoft.zander.hub.items.NavigationCompassItem;

import java.util.ArrayList;
import java.util.List;
import java.util.Random;

public class HubPlayerJoin implements Listener {
ZanderHubMain plugin;
// Misc settings
private static final boolean TEST_ALWAYS_FIRST_JOIN = false; // * default false
private static final int NAV_COMPASS_SLOT = 4;
private static final long ROUTINE_PLAYER_JOINED_DELAY = (long) (1.2f * 20);

// Firework settings
private static final double FIREWORK_GROUND_HEIGHT = 3; // blocks
private static final long FIREWORK_DETONATE_DELAY = (long) (0.3f * 20);
private static final Color[] FIREWORK_COLOR_PALETTE = {
Color.RED, Color.GREEN, Color.BLUE, Color.YELLOW, Color.PURPLE,
Color.ORANGE, Color.WHITE, Color.AQUA, Color.LIME,
};

// Sound settings
private static final float SOUND_PITCH = 1.0f;
private static final float SOUND_VOLUME = 1.0f;
private static final Sound[] WELCOME_SOUNDS = {
Sound.BLOCK_AMETHYST_BLOCK_FALL,
Sound.BLOCK_AMETHYST_CLUSTER_BREAK,
Sound.BLOCK_BEACON_ACTIVATE,
Sound.BLOCK_BELL_RESONATE,
Sound.BLOCK_CAMPFIRE_CRACKLE,
Sound.BLOCK_COMPOSTER_FILL_SUCCESS,
Sound.BLOCK_COPPER_FALL,
Sound.BLOCK_DECORATED_POT_HIT,
Sound.BLOCK_ENCHANTMENT_TABLE_USE,
Sound.BLOCK_ENDER_CHEST_OPEN,
Sound.BLOCK_END_PORTAL_FRAME_FILL,
Sound.BLOCK_NETHERITE_BLOCK_STEP,
Sound.BLOCK_RESPAWN_ANCHOR_CHARGE,
Sound.BLOCK_RESPAWN_ANCHOR_DEPLETE,
Sound.BLOCK_SCAFFOLDING_PLACE,
Sound.BLOCK_WATER_AMBIENT,
Sound.ENTITY_ALLAY_AMBIENT_WITHOUT_ITEM,
Sound.ENTITY_ARMOR_STAND_BREAK,
Sound.ENTITY_AXOLOTL_SPLASH,
Sound.ENTITY_CAT_PURREOW,
Sound.ENTITY_CHICKEN_DEATH,
Sound.ENTITY_CREEPER_PRIMED,
Sound.ENTITY_DOLPHIN_ATTACK,
Sound.ENTITY_ENDERMAN_TELEPORT,
Sound.ENTITY_ENDER_DRAGON_HURT,
Sound.ENTITY_EVOKER_DEATH,
Sound.ENTITY_FROG_DEATH,
Sound.ENTITY_GLOW_SQUID_AMBIENT,
Sound.ENTITY_GOAT_AMBIENT,
Sound.ENTITY_HOGLIN_AMBIENT,
Sound.ENTITY_PIGLIN_ADMIRING_ITEM,
Sound.ENTITY_PIG_AMBIENT,
Sound.ENTITY_PLAYER_BIG_FALL,
Sound.ENTITY_PLAYER_HURT_FREEZE,
Sound.ENTITY_SHULKER_SHOOT,
Sound.ENTITY_VILLAGER_HURT,
Sound.ENTITY_WANDERING_TRADER_AMBIENT,
Sound.ENTITY_WANDERING_TRADER_DISAPPEARED,
Sound.ENTITY_WANDERING_TRADER_REAPPEARED,
Sound.ENTITY_WARDEN_NEARBY_CLOSER,
Sound.ENTITY_WITCH_HURT,
Sound.ENTITY_WITHER_SKELETON_STEP,
Sound.ENTITY_ZOMBIE_INFECT,
};

private final ZanderHubMain plugin;

public HubPlayerJoin(ZanderHubMain plugin) {
this.plugin = plugin;
}

/// Triggers when player's client first connects.
/// Best used for validation and basic setup (permission, flags, etc).
@EventHandler
public void onPlayerLogin(PlayerLoginEvent event) {
Player player = event.getPlayer();
setPermissions(player);
}

/// Triggers when player's client has joined the world.
/// Best used for initial world interactions (player world state changes etc).
@EventHandler
public void onPlayerJoin(PlayerJoinEvent event) {
Player player = event.getPlayer();
event.joinMessage(null); // * suppress default
chatJoinMessage(player);
setInitialState(player);
Bukkit.getScheduler().runTaskLater(plugin, () -> {
// * bukkit uses 'world/playerdata' dir for tracking
if (!player.hasPlayedBefore() || TEST_ALWAYS_FIRST_JOIN) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

player.hasPlayedBefore should always return a Boolean, not sure why the secondary test is present?

chatWelcomeMessage(player);
spawnWelcomeFirework(player);
}
playWelcomeSound(player);
}, ROUTINE_PLAYER_JOINED_DELAY);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Add player online check before delayed task execution

The verification shows that the delayed task performs several player-dependent operations that could fail if the player disconnects:

  • spawnWelcomeFirework: Spawns entities and modifies world state at player's location
  • chatWelcomeMessage: Sends messages to the player
  • playWelcomeSound: Plays sound at player's location

Add a player.isOnline() check at the start of the delayed task to prevent potential exceptions:

Bukkit.getScheduler().runTaskLater(plugin, () -> {
    if (!player.isOnline()) return;
    
    if (!player.hasPlayedBefore() || TEST_ALWAYS_FIRST_JOIN) {
        chatWelcomeMessage(player);
        spawnWelcomeFirework(player);
    }
    playWelcomeSound(player);
}, ROUTINE_PLAYER_JOINED_DELAY);
🔗 Analysis chain

Potential scheduling check
When scheduling tasks with a delay, consider verifying that the player is still online before running logic that references player state (e.g., for spawnWelcomeFirework). This helps avoid possible exceptions or NPEs if the player quits early.

Let me search for more context about the player-related methods being called to better understand the potential impact.

Let me try a different approach to find these methods using ripgrep instead.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# This script searches for runTaskLater calls to see if there are any checks around player presence afterward.
rg "runTaskLater" -A 5

Length of output: 1445

Script:

#!/bin/bash
# Search for the implementation of methods that interact with the player
ast-grep --pattern 'void chatWelcomeMessage(Player $_) { $$$ }'
ast-grep --pattern 'void spawnWelcomeFirework(Player $_) { $$$ }'
ast-grep --pattern 'void playWelcomeSound(Player $_) { $$$ }'

Length of output: 197

Script:

#!/bin/bash
# Search for the method implementations using ripgrep
rg "chatWelcomeMessage|spawnWelcomeFirework|playWelcomeSound" -A 10

Length of output: 5507


// Clear the Players inventory for miscellaneous items
player.getInventory().clear();
/// Set special permission depending on the player.
private void setPermissions(Player player) {
if (player.hasPermission("zander.hub.fly")) {
player.setAllowFlight(true);
}
}

// Teleport Player to Hub spawn point
/// Set the initial state of the player in the world.
private void setInitialState(Player player) {
setupNoCollision(player);
player.teleport(ConfigurationManager.getHubLocation());
player.getInventory().clear();
player.getInventory().setHeldItemSlot(NAV_COMPASS_SLOT);
NavigationCompassItem.giveCompass(player);
}

NavigationCompassItem.giveCompass(player); // Give player navigation compass
player.getInventory().setHeldItemSlot(4); // Set the players current slot to the navigation compass

event.joinMessage(null);
// event.setJoinMessage(null); // disable default join message

if (!player.hasPlayedBefore()) {
// New user Joins for first time celebratory firework
Firework firework = event.getPlayer().getWorld().spawn(event.getPlayer().getLocation(), Firework.class);
FireworkMeta fireworkmeta = firework.getFireworkMeta();
fireworkmeta.addEffect(FireworkEffect.builder()
.flicker(false)
.trail(true)
.with(FireworkEffect.Type.CREEPER)
.withColor(Color.GREEN)
.withFade(Color.BLUE)
.build());
fireworkmeta.setPower(3);
firework.setFireworkMeta(fireworkmeta);

// Send new user a MOTD seperate to the main MOTD
List<String> newplayermotd = ConfigurationManager.getWelcome().getStringList("newplayerwelcome");
for (String s : newplayermotd) {
event.getPlayer().sendMessage(ChatColor.translateAlternateColorCodes('&', s));
}
event.getPlayer().sendMessage(" "); // Separate between messages
/// Disable entity collision for player.
/// Correct way as mentioned at:
/// https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/entity/LivingEntity.html#setCollidable(boolean)
private void setupNoCollision(Player player) {
Scoreboard scoreboard = Bukkit.getScoreboardManager().getMainScoreboard();
Team team = scoreboard.getTeam("nocollision");
if (team == null) {
team = scoreboard.registerNewTeam("nocollision");
team.setOption(Team.Option.COLLISION_RULE, Team.OptionStatus.NEVER);
}
team.addEntry(player.getName());
}

// Play random sound
int randomSoundIndex = (int) (Math.random() * Sound.values().length - 1);
Sound randomChosenSound = Sound.values()[randomSoundIndex];
player.playSound(player.getLocation(), randomChosenSound, 1f,1f);
/// Spawn a pretty firework where the player is.
private void spawnWelcomeFirework(Player player) {
Location spawnLoc = player.getLocation().add(0, FIREWORK_GROUND_HEIGHT, 0);
Firework firework = player.getWorld().spawn(spawnLoc, Firework.class);
FireworkMeta meta = firework.getFireworkMeta();

if (!isVanished(player)) {
Component broadcastMessage = Component.empty()
.color(NamedTextColor.GRAY)
.append(event.getPlayer().name())
.append(Component.text(" joined."));
Random random = new Random();
Color[] FCP = FIREWORK_COLOR_PALETTE;

for (Player otherPlayer : Bukkit.getOnlinePlayers()) {
otherPlayer.sendMessage(broadcastMessage);
}
// random primary-colors
List<Color> colors = new ArrayList<>();
int numColors = random.nextInt(3) + 2; // 2-4 colors
for (int i = 0; i < numColors; i++) {
colors.add(FCP[random.nextInt(FCP.length)]);
}
// random fade-color and firework type
Color fadeColor = FCP[random.nextInt(FCP.length)];
FireworkEffect.Type[] types = FireworkEffect.Type.values();
FireworkEffect.Type type = types[random.nextInt(types.length)];

FireworkEffect effect = FireworkEffect.builder()
.flicker(false)
.trail(true)
.with(type)
.withColor(colors)
.withFade(fadeColor)
.build();
meta.addEffect(effect);
meta.setPower(1);
firework.setFireworkMeta(meta);

Bukkit.getScheduler().runTaskLater(plugin, () -> {
firework.detonate();
}, FIREWORK_DETONATE_DELAY);
}

player.setCollidable(true); // Disable player collision.

// If user has the fly permission, enable flight
if (player.hasPermission("zander.hub.fly")) {
player.setAllowFlight(true);
/// Send a welcome message in player's chat.
private void chatWelcomeMessage(Player player) {
List<String> message = ConfigurationManager.getWelcome().getStringList("newplayerwelcome");
for (String row : message) {
player.sendMessage(ChatColor.translateAlternateColorCodes('&', row));
}
player.sendMessage("");
}

/// Send a join message in player's chat.
private void chatJoinMessage(Player player) {
if (isVanished(player))
return;
Component message = Component.empty()
.color(NamedTextColor.GRAY)
.append(player.name())
.append(Component.text(" joined."));
Bukkit.getOnlinePlayers().forEach(p -> p.sendMessage(message));
}

/// Play a random sound for the player.
private void playWelcomeSound(Player player) {
Sound randomSound = WELCOME_SOUNDS[(int) (Math.random() * WELCOME_SOUNDS.length)];
player.playSound(player.getLocation(), randomSound, SOUND_VOLUME, SOUND_PITCH);
}

/// Check if the player is currently vanished.
private boolean isVanished(Player player) {
for (MetadataValue meta : player.getMetadata("vanished")) {
if (meta.asBoolean()) return true;
}
return false;
return player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean);

Copy link

@coderabbitai coderabbitai bot Jan 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null check in isVanished method

The stream operation on metadata could throw NPE if the "vanished" metadata is not set.

 private boolean isVanished(Player player) {
-    return player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean);
+    return player.hasMetadata("vanished") && 
+           player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Check if the player is currently vanished.
private boolean isVanished(Player player) {
for (MetadataValue meta : player.getMetadata("vanished")) {
if (meta.asBoolean()) return true;
}
return false;
return player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean);
/// Check if the player is currently vanished.
private boolean isVanished(Player player) {
return player.hasMetadata("vanished") &&
player.getMetadata("vanished").stream().anyMatch(MetadataValue::asBoolean);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@palsmo you'll need a null check as stated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

}
}
2 changes: 1 addition & 1 deletion zander-hub/src/main/resources/plugin.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@ permissions:
zanderhub.administrator:
default: op
zanderhub.build:
default: op
default: op