#[MEGA THREAD] Get Your Code Reviewed or Review Others

1 messages · Page 13 of 1

sharp epoch
# 
public static boolean sendPlayerToServer(Player player, String server) {
    try (ByteArrayOutputStream b = new ByteArrayOutputStream();
         DataOutputStream out = new DataOutputStream(b)) {

        out.writeUTF("Connect");
        out.writeUTF(server);
        player.sendPluginMessage(instancia, "BungeeCord", b.toByteArray());

    } catch (Exception e) {
        instancia.getLogger().info(ChatColor.RED + "Excepción conectándose a " + server);
        return false;
    }
    return true;
}
#

you shouldn't catch blanket exceptions like that either

night knoll
#

oh damn that's true 😭

#

i forgot to put return false; too

sharp epoch
#

here, if you catch a blanket Exception, if anything else besides an I/O exception were to be thrown (though unlikely) then this would preemptively catch it even though it doesn't try to handle the error

#

it is a naive method, which only returns false when something went wrong and warns to console even though it could've been something manageable

#

exceptions are the way you tell your server a fallible operation went wrong, and a boolean barely provides context as to what could potentially happen here

night knoll
#

okok ty

sharp epoch
#

you may also use nullability annotations to prevent possible NPEs there, which would happen if either argument there is null, shouldn't be the case but mistakes happen and nullability annotations allow you to prevent that

#

finally, you may want to check if the player is online before sending the plugin message

#

ah, sendPluginMessage throws IllegalArgumentException when one of the arguments is null, or the plugin is in an inconsistent state, so you may want to handle the latter as well

#

it isn't necessary for these methods to be bullet-proof since usually one designs systems in such a way that one preemptively tackles these kind of issues, but just giving you an idea of how many unforeseen issues can pop up in seemingly naive methods like that

junior holly
steady shard
#

anyone want to code review my duel manager:

night knoll
# steady shard https://github.com/XinCraft/codereview/blob/main/match/duel/DuelManager.java 
   @Override
            public void run() {
                Player expiredPlayer = Bukkit.getPlayer(invite.getDuelReceiver());

                if (expiredPlayer != null) {
                    expiredPlayer.sendMessage(ChatColor.RED + "The duel request you have been sent has expired.");
                }

                Player expiredPlayerSender = Bukkit.getPlayer(player);
                if (expiredPlayerSender != null) {
                    expiredPlayerSender.sendMessage(ChatColor.RED + "The duel request you have sent has expired.");
                }

                invites.remove(player);
            }
        });
    }```
Shouldn't ` invite.getDuelReceiver()` have a null check?
steady shard
#

I dont think theres ever a case where duelReciever is null

steady shard
night knoll
steady shard
#

thats what this is for

#

if (expiredPlayer != null) {

night knoll
#

Ah

#

And aren't Runnables supposed to have #runTaskLater?

steady shard
#

timeoutRunnable.runTaskLater(XinCraftPlugin.get(), 60 * 20);

night knoll
#

We are referring to the DuelManager, right?

night knoll
steady shard
junior holly
#

@steady shard I didn’t look at code but use guard clauses

junior holly
#

IMO makes your code cleaner and easier to read

steady shard
#

I usually would but because it’s only one liner I feel like this is cleaner

sly jackal
#

why would you not return there earlier

#

oh wait

#

I see whats going on

#

yeah its fine the way it is now

#

how are DuelInvite objects even created

#

you need to probably cancel the old invite as you just overwrite it in the map

#

idk if the task still stays alive and just cancels after some time anyways

#

but that would be confusing

#

I think it makes sense to just call removeInvite(player) before you put in the new one

#

same goes for the end of the runnable

#

instead of removing the invites map directly, use removeInvite(player)

#

you have a function for it, might as well use it

junior holly
#

https://paste.md-5.net/muwaxivita.java

Speaking of invite services, this is mine... to be fair not really sure if a base interface is even needed though it used to be written like: https://paste.md-5.net/ralogodode.java. However since inviteImpl is already a record theres no need to enforce those getter methods, tbf the inviteService is probably the most helpful thing in the paste

#

Also keep in mind the set within the map is not thread safe (I forgot this was even a thing to watch out for lol)

round fossil
#

Just wna add to the invite impl design,

it can be nice to have inviter and invitee uuid joined as key (commutatively), then no need to verify by validating invariants every time you query the cache

steady shard
red bison
junior holly
#

Also it’s a bit hard to define a “generic service” such as that would fit all implementation requirements

clever crag
clever crag
# junior holly Also it’s a bit hard to define a “generic service” such as that would fit all im...

like this?

Nullable
    public <T extends Service> Service getRawService(Class<T> serviceClass) {
        if (serviceClass.equals(Service.class)) {
            throw new RuntimeException("Service.class");
        }
        
        for (Service service : services) {
            if (service.getClass().equals(serviceClass)) {
                return service;
            }
        }
        
        return null;
    }
    
    @Nullable
    public <T extends Service> T getService(Class<T> serviceClass) {
        Service rawService = getRawService(serviceClass);
        return rawService == null ? null : serviceClass.cast(rawService);
    }
junior holly
#

I’m not using your api dawg

hybrid osprey
#

why would you even do this

dense leaf
#

throw new RuntimeException("Service.class")

late smelt
#

Not even using a map

sleek shard
#

maps are there for a reason

random yacht
#

Class#cast() is usually nicer, some IDEs complain about an unchecked exception and the cast() method hides it. Also makes the intent a bit more clear

#

But yeah the null check is redundant because null can be cast to anything

late smelt
#

Can I cast null to null

alpine anvil
#

Cast me to null

sharp epoch
hasty lotus
#

Not really a code review, but I was wondering what features should I add that would help server owners better? My plugin is called Murder Run, and it's a spin-off of Dead by Daylight. These are the current features https://github.com/PulseBeat02/MurderRun?tab=readme-ov-file#features, and I want to add more, but I don't want to turn it into a bloatware plugin like CMI.

#

I was thinking of a design similar to PAPI expansions but for my plugin, where server owners could add custom "theme packs," containing custom gadgets, maps, and more. These expansion packs would be like a plugin that they would install

night knoll
night knoll
#

mana regenerating task

#

😭

#

i don't know if i did it right

#

i prob have a super wrong code or smth

dense leaf
#

cache the statsmap entry

night knoll
#

do you mean like create a variable before starting the task?

dense leaf
# 
            double maxMana = this.statsMap.get(uuid).get(SkillsStatsEnum.MAXMANA);
            double currentMana = this.statsMap.get(uuid).get(SkillsStatsEnum.CURRENTMANA).intValue();

->

var stats = this.statsMap.get(uuid);
double maxMana = stats.get(...);
night knoll
#

alr

terse spoke
#

Could just use time comparison with System timeInMillis

steady shard
#

I've never done this before so I need feedback on my socket used for multi-server communication

#

and my code to talk to matches server to request a match:

    private CompletableFuture<Integer> askMatchesServer(String mapName, int teamSize) {
        return CompletableFuture.supplyAsync(() -> {
            SocketClient client = new SocketClient();
            try {
                client.startConnection("127.0.0.1", 6667);
                RequestMatchPacket packet = new RequestMatchPacket(mapName, teamSize);
                int response = client.requestInt(packet);
                client.stopConnection();
                return response;
            } catch (IOException e) {
                e.printStackTrace();
            }
            return -1;
        });
    }```
round fossil
#

u prob wanna use a custom executor

#

fork common pool doesnt feel so good

steady shard
#

what’s the differences?

sleek shard
#

Sorry for non direct screenshots, took this while at college - Does anyone know if this is okay for API design? Just trying to make it all abstracted. Just not sure on it, as I can’t call methods the same name. Would it be better if I have “Internal” interfaces like InternalEventBus, InternalSchedulers etc etc

IMG_5289.jpg IMG_5290.jpg
random yacht
#

The generic on callEvent() doesn't do anything, but that aside, it's definitely a unique approach

sleek shard
#

In your humble opinion

random yacht
#

I don't know if I like the whole Instances thing, but if you have a separate API and implementation then you need a bridge somewhere, and I guess that could be it

sleek shard
#

Yeah I have this right now @random yacht

IMG_5291.jpg
#

ignore da ditty screen

sharp epoch
#

unrelated to your thing but why are you taking actual pictures of your screen instead of screenshots lol

sleek shard
#

I didn’t have dc installed on my laptop

sharp epoch
#

do you just not wanna bother sending them up to your phone or something

sleek shard
#

meh

sharp epoch
sleek shard
#

just ignore why I do that LMAO

sharp epoch
#

I used it there for the longest time, since installing it seemed unncessary since I always had it open beside a browser anyway

sleek shard
#

ah

sharp epoch
sleek shard
#

fun

sharp epoch
#

first off, nothing is bound so anything counts as an event, sounds counter-intuitive

#

the design looks fine for an event bus, however I don't see any advantages to it vs say the Guava EventBus

#

then again, this is just design-wise so there's not much one can really speak about. I don't love how it is bound to a singleton there since it may conflict with unit testing when times comes to shove but other than that it looks fine

sleek shard
#

This is how it works

image.png
#

The EventBus

sharp epoch
#

it looks like it'd struggle at high-volume situations since call event has to look up for the event system every time an event is called

#

it'd be better to just tie together the event system with its bus, or if you want to maintain the current relationship have a Map<EventClass, EventSystem> instead but I don't know how well that will scale, food for thought

sleek shard
#

Yeah I did think about that, not sure about it

pine grail
#

A context, such as the server context, may have children, such as a world. Each context is then responsible for firing events, i.e. world.events().register(PlayerJoinEvent.class, event -> {}); would run when a player joins the world, but server.events().register(PlayerJoinEvent.class, event -> {}); would only run when a player first connects

#

Events are not bubbled up

woeful pasture
#

(don't roast how I look roast the project)

muted sentinel
woeful pasture
#

the borders are for my visuals, and aren't actually going to be there in the end btw lol

muted sentinel
sleek shard
#

Honestly I like it

image.png
pine grail
pine grail
#

Should I have some minimal code in an interface? I.e.

public interface SomeUsefulMathStuff {
  static int doStuff(int x) {
    return x + 5;
  }
}
dense leaf
#

why would that be an interface

desert ore
#

Looks like a util class

dense leaf
#

yea ^

pine grail
dense leaf
#

Because that should just be a util class

pine grail
#

soo

# 
public final class FastMath {

    private FastMath() {
        
    }

    public static int floorToInt(double x) {
        return x >= 0 ? (int) x : (int) x - 1;
    }

}
desert ore
#

Yes

pine grail
#

my API now has one class noo

#

excluding exceptions*

desert ore
pine grail
#

you can't

desert ore
#

You can almost

# 
public interface SomethingException {
    Exception create();
}```
 Or something not so ass
sleek shard
pine grail
#

I like interfaces for utils

sleek shard
#

even though that isn’t their usecase, fairs

#

this is mine so far @pine grail

image.png
#

Working on the Mongo and Redis stuff rn

alpine anvil
#

Who the fuck is drift mc

sleek shard
#

api is all the interfaces,
common is all the implementations,
spigot is for spigot shit ofc, like the SpigotEventSystem implements EventSystem<Event>

image.png
sleek shard
alpine anvil
#

Is that ur server u had in ur bio ages ago

sleek shard
#

that was Plexverse, but driftmc is a server I own

#

and used to run

#

concurrent players of 180 at one point

#

was fun

untold jay
#

How much revenue

sleek shard
#

$15k+

untold jay
#

Yearly?

sleek shard
#

That was total

#

It was a prison server

untold jay
#

Oh

#

damn

#

I never understood the excitement to play grinding servers and I still don't lol

#

was there anything unique to your prison server

sleek shard
#

Yeah it didn't get many dono's, that's why it shut down in the end, because all the money that was earned, was put back into the server, and I invested like $2k into a new realm, for it to fully flop and outsources another developer to make it easier, but they took too long, and the plugin was finished 10 minutes BEFORE the new realm released

#

was a pisstake

sleek shard
#

It was kinda cool, but no-one liked it as it wasn't their typical prison server

untold jay
#

Ah

late smelt
#

If prison players aren’t stuck in a box 24/7 they won’t play

sleek shard
#

That's what I figured out

#

If you search "DriftMC" on youtube, you'll find my server

#

The thing is - with modern prison server's. The prison community isn't used to it, so it's killing the gamemode, with all these "new" features. I need to try and re-release my server with a new core, but the same typical prison gamemode as back in the day, when it was OP

late smelt
#

That’s because the community is like 8

#

And doesn’t like change

alpine anvil
#

You have to ease in the changes but make them scream difference to make people join

untold jay
#

What is the most popular game mode right now

sleek shard
sleek shard
#

Only really because of hypixel

#

But on minehut, it's still Tycoon/Gen servers

untold jay
#

That's so lame

late smelt
#

We don’t talk about minehut

untold jay
#

Minehut servers are something else

sleek shard
#

loll

#

real

untold jay
#

The ones on the top page are actually well made

sleek shard
#

Actually Survival is quite up there, but that's because of all the youtubers that own them p2w servers

untold jay
#

But some of them just look like a bunch of plugins with little to no configuration mashed up

sleek shard
#

they usually get premades

untold jay
#

HCF probably makes decent revenue if you have the right players

#

A lot of those players are just rich and spend so much

sleek shard
#

Oh ViperMC back in the day

sharp epoch
#

HCF is competitive, don't see how to gain revenue off that

sleek shard
#

Actually prisons was really really popular with CosmicPrisons, making.. $15M in revenue

sharp epoch
#

anything competitive will ultimately rely on cosmetics which is eh, hit or miss as it depends on the server's reputation

sleek shard
untold jay
sleek shard
#

back in the 1.7.10 days

#

when p2w wasn't a rule

sharp epoch
sleek shard
#

I mean if it was back in the day... it kinda does

sharp epoch
#

Skyblock boomed just because it's actually just MMORPG

sleek shard
#

WynnCraft is still sick asf

#

Just too overcomplicated for me

rancid fern
#

hm? overcomplicated?

sleek shard
#

Yeah I just don’t get it myself

late smelt
#

His brain too smol

#

<3

alpine anvil
sleek shard
random yacht
pine grail
random yacht
#

Then rip in pieces

terse spoke
#

I used to own one of the largest geopol earth servers

#

Not sure about market now but very popular 2 years ago

night knoll
#

Sir, this is "[MEGA THREAD] Get Your Code Reviewed or Review Others"

junior holly
placid gulch
#

but but you can find where your house is and build ittttt

terse spoke
junior holly
#

Def heard that before

sleek shard
#

I wont be exactly sharing the backend code of it publicly.

woeful pasture
random yacht
#

Yeah pretty much the same criticism tbh

#

and as a matter of preference, I would prefer EventBus#fire() to be either #call() or #push(). But again, preference. If you prefer #fire(), nothing wrong with that either

woeful pasture
sleek shard
#

ahaha

#

yeah fairs

#

I'll rename to call

#

thank you choco and Y2K

#

appreciate you's

late smelt
#

But who is it calling

#

Did you provide a phone number

random yacht
#

Yes. All event calls must contain phone numbers of the callbacks subscribing to that event >:((

sleek shard
#

TRUE

#

I'll redirect it to my phone number

#

1 sec

steady shard
# 
@Slf4j
public class PlayerConnectionChecker implements PluginMessageListener {
    private final ConcurrentMap<String, CompletableFuture<Boolean>> futures = new ConcurrentHashMap<>();

    public CompletableFuture<Boolean> checkPlayerConnected(Player player, String targetPlayerName) {
        CompletableFuture<Boolean> future = new CompletableFuture<>();
        futures.put(targetPlayerName.toLowerCase(), future);

        try (ByteArrayOutputStream data = new ByteArrayOutputStream(); DataOutputStream out = new DataOutputStream(data)) {
            out.writeUTF("PlayerList");
            out.writeUTF("ALL"); // Request the player list for all servers
            log.info("Requesting player list from BungeeCord");
            player.sendPluginMessage(XinCraftCore.get(), "BungeeCord", data.toByteArray());
        } catch (Exception ex) {
            future.completeExceptionally(new RuntimeException("Unexpected error while writing to byte array", ex));
        }

        return future;
    }
# 

    @Override
    public void onPluginMessageReceived(String s, Player player, byte[] bytes) {
        if (!s.equals("BungeeCord")) {
            return;
        }

        try (DataInputStream in = new DataInputStream(new ByteArrayInputStream(bytes))) {
            String subChannel = in.readUTF();
            if (subChannel.equals("PlayerList")) {
                in.readUTF(); // Read the server name
                String[] playerList = in.readUTF().split(", ");
                log.info("Received player list from BungeeCord: {}", Arrays.toString(playerList));

                for (String playerName : playerList) {
                    CompletableFuture<Boolean> future = futures.remove(playerName.toLowerCase());
                    if (future != null) {
                        log.info("Player {} is online", playerName.toLowerCase());
                        future.complete(true);
                    }
                }
                // Complete futures for players not found in the list
                futures.forEach((playerName, future) -> future.complete(false));
                futures.clear();
            }
        } catch (IOException ex) {
            futures.forEach((playerName, future) -> future.completeExceptionally(new RuntimeException("Unexpected error while reading from byte array", ex)));
            futures.clear();
        }
    }
}```
#

my class for checking player connection server wide

#

usage:

# 

            XinCraftCore.get().getConnectionChecker().checkPlayerConnected(fromPlayer, targetPlayerName).thenAccept(isConnected -> {
                log.info("Player {} is connected: {}", targetPlayerName, isConnected);
                if (isConnected) {
                    String message = String.join(" ", Arrays.copyOfRange(args, 1, args.length));
                    XinCraftCore.sendNetworkPlayerMessage(fromPlayer, targetPlayerName, message);
                } else {
                    fromPlayer.sendMessage(ChatColor.RED + "That player is not online throughout the network!");
                }
            });```
sharp epoch
#
  1. you want to check whether the player is null or isn't online in that checkConnected method and returning early if so
#
  1. probably add a cleanup function that gets called onDisable, so if there are any pending tasks, you will just cancel them
#

I wonder if there is any difference between using guava's byte array data I/O or just plain old byte array I/O streams wrapped over a data I/O stream for things like these

#

the size is known beforehand so ultimately using a stream would be more inefficient as it is one more initialization & indirection? Would have to benchmark that

sharp epoch
#

barely a difference

image.png
#

would probably been better to test single-shot for something like this instead of average time but eh

wintry fern
#

or after warm up

rancid fern
#

Though I'd like more iterations for both warmup and run

round fossil
#

like its more of a convenience and boilerplate-reduction thing afaik

sharp epoch
#

it indeed is pretty useless to measure performance of a convenience class, however I was curious of whether it performed better or not than the traditional way to do it

#

with more iterations that error bar will be less pronounced and the difference should be negligible, but a 1-shot time benchmark would be good to know perhaps

round fossil
#

cuz unless my memory is really washed ByteStreams wraps and delegate the thingy more or less

sharp epoch
#

they just got their own implementation of DataInput/Output for this, unrelated to ByteArrayInput/Output and DataInput/Output streams

#

in theory, the convenience is that it combines both a data I/O stream and a byte array I/O stream

#

you also don't have to worry about closing the thing since it only works with arrays of known-size, meaning it'll get GC'd along with whatever array you pass it

round fossil
#

yes but the implementation provided by ByteStreams#newDataInput/Output uses ByteStreams$ByteArrayDataInput/OutputStream which is just a wrapper around DataInput/OutputStream

#

this is how I remember it at least because I was interested in this 2 years ago

#

maybe they switched it up

#

however, if they are different, then a benchmark is interesting certainly ^^

sharp epoch
#

I was under the impression it was a different implementation because I saw the private byte array data I/O stream classes and thought they were doing something differently, didn't realize it was just a wrapper

#

so, yeah. There's really no difference implementation wise, the guava classes are just a convenient class for this

#

the code for this hasn't really been touched in 10-15 years, that's crazy

desert ore
#

Some people are running code that's older than their users lmao

pine grail
#

I mean yes

#

considering that most OS devs are literally shipping code older than most of their users

desert ore
pine grail
#

fr

desert ore
#

That one time I reboot my laptop and accidentally end up in DOS mode

round fossil
round fossil
steady shard
#

?paste

rich fogBOT
steady shard
#

NetworkPlayer

@Data
public class NetworkPlayer {
    @Nullable
    private UUID uuid;
    @Nullable
    private String name;
    private boolean online;
}```
# 
public enum PlayerRequestType {
    UUID,
    NAME
}```
#

I use it like this in my message manager:

# 
        XinCraftNetwork.getPlayer(recipient).thenAccept(networkPlayer -> {
            if (!networkPlayer.isOnline()) {
                fromPlayer.sendMessage(ChatColor.RED + "That player isn't online!");
                return;
            }

            Conversation conversation = new Conversation(fromPlayer.getUniqueId(), networkPlayer.getUuid());
            conversations.saveConversation(conversation);
        });```
woeful pasture
steady shard
#

what is wrong with lombok

steady shard
steady shard
#

edited for that

tawny stream
#

Hello! I am trying to make random teleport plugin and have a problem. I have made a /rtp command which starts 5 second cooldown before player teleports. After cooldown it searches for random location in world. I have a problem that sometimes when waiting time ends server lags for 1-3 seconds and MSPT rises to 300-1000. What could be here wrong? Can provide more code and would be grateful for help

    private static final int MAX_ATTEMPTS = 32;

    private void teleportPlayer(Player player, int radius) {
        new BukkitRunnable() {
            @Override
            public void run() {
                World world = Bukkit.getWorld("world");
                if (world == null) {
                    player.sendMessage("Мир не найден.");
                    return;
                }

                for (int i = 0; i < MAX_ATTEMPTS; i++) {
                    int x = random.nextInt(radius * 2) - radius;
                    int z = random.nextInt(radius * 2) - radius;
                    int y = world.getHighestBlockYAt(x, z) + 1;
                    Location location = new Location(world, x, y, z);
                    Material blockType = location.getBlock().getRelative(0, -1, 0).getType();

                    if (blockType != Material.WATER && blockType != Material.LAVA) {
                        Chunk chunk = world.getChunkAt(location);
                        if (!chunk.isLoaded()) {
                            world.loadChunk(chunk);
                        }

                        Bukkit.getScheduler().runTask(Plugin.getInstance(), () -> {
                                    player.teleport(location);
                                    player.sendMessage(color("&6[\uD83D\uDD25] &fВы были успешно телепортированы на координаты: &6" + x + ", " + y + ", " + z));
                                    player.addPotionEffect(new PotionEffect(PotionEffectType.BLINDNESS, 40, 0));
                                    player.playSound(player.getLocation(), BLOCK_NOTE_BLOCK_PLING, 1.0f, 1.0f);
                                    player.setFallDistance(0F);
                                }
                        );

                        return;
                    }
                }

                player.sendMessage("Не удалось найти безопасное место для телепортации. Попробуйте ещё раз.");
            }
        }.runTaskAsynchronously(Plugin.getInstance());
    }
novel meteor
#

if (!chunk.isLoaded()) {world.loadChunk(chunk);}
That is pointless I believe, just creating a Location or accessing a block there loads the chunk.

#

And I do not understand the need for the nested runTask.

tawny stream
tawny stream
novel meteor
#

?workdistro

rich fogBOT
novel meteor
#

this may help a bit with the lag

#

@tawny stream

tawny stream
muted sentinel
#

Hey yall been awhile, took a break but back at it.
How does this look so far?

 public void updatePlayerDaily(UUID uuid, Daily daily, int amount) {
        PlayerData playerData = playerDataManager.getPlayerData(uuid);
        Player p = Bukkit.getPlayer(uuid);

        String playerKey = String.format("%s.%s", PlayerData.defaultData.DAILIES, daily);
        String dailyKey = "";

        int dailyAmount = 0;
        int playerAmount = playerData.getInt(playerKey + ".Amount");

        for (String type : dailyFile.getConfig().getKeys(false)) {
            //Gets the identifiers
            for (String s : dailyFile.getConfigurationSection(type).getKeys(false)) {
                dailyAmount = dailyFile.getConfigurationSection(s).getInt("Amount");
                dailyKey = String.format("%s.%s", type, s);
            }
        }
        int currentAmount = playerData.getInt(playerKey);
        int newAmount = currentAmount + amount;
        if (dailyAmount >= playerAmount) {
            send(p, "DAILY COMPLETE");
            return;
        }
        dailyFile.getConfig().set(dailyKey, newAmount);
    }```
woeful pasture
sharp epoch
#

I don't think there's anything wrong with the class itself, but rather the name

#

if you named it PlayerConfigurationSection, there would've been no issues with regards to what data the class contains since it'd essentially just be a ConfigurationSection with some utility methods

#

but I do agree that generally, if you want to make a type, and your data is formally defined, as in, this PlayerData/PlayerConfigurationSection thingy doesn't just hold arbitrary data for the player, then there'd be better ways to express what it is holding instead of having to manually parse the data each step of the way

dense leaf
#

something me and raydan (and ebic) have been working on for a couple weeks now: a spigot plugin development toolchain - for the gradle plugin I'd like some feedback about the current buildscript structure: https://pastes.dev/wUkJa6uvmr

todo:

  • plugin.yml builder
  • runServer tasks and copying
pastes | wUkJa6uvmr

A paste containing 66 lines of kotlin.

desert ore
#

Another todo is ✨ configurations ✨ (please shoot me in the head this is unbearable)

rancid fern
dense leaf
#

Binary patches, bsdiffed between vanilla and BT-generated spigot jar

#

not remapped

desert ore
#

They allow for fast spigot jar generation

rancid fern
#

I see

#

Could you add an option to use a Spigot jar from maven local

#

or make it run BT

desert ore
#

Probably

dense leaf
desert ore
#

Custom forks and shit

#

Or dev/exp builds

dense leaf
desert ore
dense leaf
#

Gradle configurations support files in general

#

via Project.files(Object... files)

desert ore
woeful pasture
desert ore
dense leaf
#

It's nice for e.g. Plib, I really hate having to install it on my server separately

dense leaf
alpine anvil
dense leaf
#

yeah you helped us a bit

desert ore
# dense leaf ?

pluginImplementation's are expected to be remapped if they use nms so the user can easily navigate through the decompiled code and check what minecraft classes it is using

woeful pasture
dense leaf
#

Why?

woeful pasture
#

I don't think I should have to say that including it in regular distribution is really really dumb

desert ore
dense leaf
#

It doesn't shade the plugin, as in, shade the jar contents

desert ore
woeful pasture
# dense leaf Enlighten me
  1. You're distributing someone else's plugin without their permission while not always illegal, almost certainly disrespectful.
  2. Not sure how you handle clashes once deployed, but if you don't things will break
  3. Even if you do above you've now bloated your final jar by completely unnecessarily including other plugins.
dense leaf
#

for 2. I need to check versions, currently it just does not get loaded if said plugin is already installed in the server

desert ore
#

Also, I think rad, when making pluginImplementation, had no idea what it does kek

dense leaf
#

for 3. how does it break other plugins?

woeful pasture
# dense leaf for 3. how does it break other plugins?

If im the server owner I install your plugin than install other plugins. Let's say idk 3 or 4 of my plugins use protocol lib your plugin now just has a dormant protocol lib sitting inside it doing absolutely nothing, but taking disk space and slowing startup time because you need to do clash detection. The more of your plugins I install the worse this gets and the slower startup becomes. I'm not saying the idea is bad for testing, but when deployed it's just not sensible imho

desert ore
#

Most servers already have lp, pp and plib, so it makes no sense to jij it

woeful pasture
#

If they're meant to be done like that you can shade the library? This just seems like a worse version of shading that can let dumb stuff slide

#

If it's a library as plugin you still run into the issues I've said above.

desert ore
woeful pasture
#

At that point you should bootstrap then

desert ore
#

Fabric does it, so I don't see a reason for us not to, since we are trying to mimic fabric looms functionality

woeful pasture
#

In which case instead of doing what this is you should implement a proper bootstrap

desert ore
#

But in a spigot realm

woeful pasture
#

I mean "Fabric does it so it's fine" isn't really a good counter at all. I'm just gonna leave it here as I pointed out issues I see in such a system

desert ore
dense leaf
#

I find it to be really useful when e.g. you're the only plugin/mod on the server, i.e. fully developing the server yourself, and you don't need to worry about every environment having dependencies installed, but rather just bundle them all. Whether it should be used for distribution, that's up to the plugin developer I guess. With licensing, plugins have to be GPL because of Bukkit, which I believe (don't quote me on this) allows redistribution like JIJ. However I am in no needs of any knowledge on the topic of licensing

night knoll
#

👋 I have been working on a skyblock remake for a while which uses Minestom. Please beware with me I started on quite a few parts of gamemode but never finished them but if anyone has any opinions x)
https://github.com/unjoinable/Skyblock

#

*I accept all levels of criticism provided you elaborate your point. *

hybrid osprey
#

Your groupid name is misleading and not good; it should not include 'java.io'

#

it should be com.github.unjoinable.Skyblock

desert ore
night knoll
#

oh right I see.

desert ore
night knoll
#

I will fix it

desert ore
#

How did you even come up with that

night knoll
#

I will show (nvm I forgot I deleted all my screenshots) But, someone told me it is a good idea

dense leaf
#

using gradle is pretty based

hybrid osprey
#

no it is not

desert ore
hybrid osprey
#

also avoid a general 'Utils.java'

desert ore
night knoll
hybrid osprey
#

compared to your other utils classes, Utils.java is lacking in documentation

night knoll
#

oh yeah

#

it is just a dumpster of methods I hate.

desert ore
#

Documenting util classes is so fucking funny

hybrid osprey
#

also you should prefer List over []

night knoll
#

arrays?

hybrid osprey
#

yes

night knoll
#

Arrays can be quite efficient

dense leaf
night knoll
#

I am surprised no one commented on my auto version updater

jagged rock
#

These should be tossed on their separate methods

image.png
night knoll
#

good idea.

dense leaf
jagged rock
#

Lots of singletons.. hm

hybrid osprey
jagged rock
#

sero clue what this comment means but this should be a discrete class. Tasks like these don't belong on your "main"

image.png
hybrid osprey
#

sero clue

#

I love this new number

night knoll
#

🤦‍♂️ fuck I forgot to remove the filthy comments

jagged rock
#

You can omit the first and last impl

image.png
#

duplicate code = nasty

#

and varargs covers every scenario

night knoll
#

okay

desert ore
jagged rock
#

I don't think closing the buffered reader closes the input stream reader too

image.png
desert ore
#

(I forgor how generics worked for a min so had to look em up lol)

jagged rock
#

try-with-resources >>

#

peak naming

image.png
hybrid osprey
#

whats an Arto

night knoll
#

StringArrayIntoIntArray

jagged rock
#

probably some unique fish

desert ore
hybrid osprey
#

ooo

#

i like cheese

desert ore
hybrid osprey
#

also you should look into using Duration

#

instead of long cooldownInMs

night knoll
#

which class is that?

jagged rock
#

if-return-else smh

image.png
hybrid osprey
night knoll
night knoll
jagged rock
#

Keep your constants above everything else. This is messy

image.png
hybrid osprey
#

Ability

jagged rock
#

Also why are messages in the player class??

jagged rock
night knoll
#

because they are used there

jagged rock
#

poor excuse

hybrid osprey
#

they don't belong there

#

they may be used there but use != belonging

night knoll
#

I can shove all the ability related code into AbilityHandler

#

which I should have probably done b4.

jagged rock
#

This isn't scalable. If I want to introduce custom abilities I need to fork your project. Ability cost types should have their own handling for this

image.png
#

This looks sloppy and doesn't follow naming conventions

image.png
#

Can't you store these as constants? makes no sense

night knoll
#

oh sorry I forgot to rename those

jagged rock
#

Make sure to do defensive copies. Returning mutable maps is error-prone

image.png
hybrid osprey
# 
    public static Statistic byOrdinal(int ordinal) {
        return values()[ordinal];
    }``` ⚠️ ordinal detected
jagged rock
#

ordinals are fine if you're doing networking

#

because ✨ bandwidth ✨

#

zero excuse elsewhere

desert ore
#

I love saving bandwidth when doing networking

jagged rock
#

it's quite difficult to guarantee read-only map copies when using records

image.png
hybrid osprey
#

oh wait sorry i ma tired

night knoll
#

it is okay

hybrid osprey
#

🤦‍♂️ confused records

jagged rock
#

While this is fine, interfaces with a "constants" class are preferable over enums for future expandability (look at kyori's NamedTextColor)

image.png
#

I don't see a reason for rewards to be capped to a type. For example, at work we make rewards pretty arbitrary

image.png
hybrid osprey
#

kek

jagged rock
#

We have rewards that just run a block of code that's registered somewhere

#

So you can have a ExpReward, CurrencyReward, ItemReward, CommandReward, StatisticReward, RunnableReward etc

hybrid osprey
#

some things are final while others aren't, but they could be

jagged rock
#

the usage of records vs classes is also inconsistent

#

I'd rather just see classes everywhere

night knoll
jagged rock
#

Zero reason for ranks to be hardcoded

image.png
night knoll
#

why not?

hybrid osprey
#

because they don't need to be hardcoded

jagged rock
#

why would they?

night knoll
#

I never plan on adding more

hybrid osprey
#

well I do

#

so don't hardcode it

night knoll
#

okay

hybrid osprey
#

also close your minimessage tags

jagged rock
#

The idea is to build good habits, not to fight us or be ignorant to the principle of "things should be expandable without the need to change code"

night knoll
#

oh no, my intentions are not to fight you. I am just trying to explain my pov.

#

sorry if u got the wrong message

hybrid osprey
#

that's fine

jagged rock
#

You can very easily make this a data object and make it configurable

#

And maybe even have a list of permissions and just check for those

night knoll
#

yes

jagged rock
#

You can them have your consumer make custom ranks (like hypixel's "pig++" for example) that are just an amalgamation of certain features

night knoll
#

Valid reasons, I will change it.

night knoll
jagged rock
#

Your codebase shouldn't be constrained by "design", but rather by choice

hybrid osprey
#

also maybe avoid shoving huge chunks of code monolithically

#

you should look into splitting it up

jagged rock
#

I'd like to see a modular approach for this

image.png
night knoll
jagged rock
#

minestom ECS would be sexy to see

hybrid osprey
#

ActionSlot can be a record

#

also why does it store the player

jagged rock
#

peak code

image.png
hybrid osprey
#

based off the name I would not initially think a slot could contain a player

jagged rock
#

These can be converted to a List<String>

image.png
night knoll
#

I could ig

jagged rock
#

These can be converted to a class + constants

image.png
night knoll
#

yea, but problem is all my Stats now use ordinals

jagged rock
#

womp womp don't use ordinals then

night knoll
hybrid osprey
#

wow

#

foreshadowing

jagged rock
#

this is how we do statistics at work

image.png
dense leaf
#

registries could fix you

#

yea

night knoll
#

these are a little different stats

jagged rock
hasty lotus
jagged rock
#

Then they're not statistics but attributes instead

night knoll
#

yes

hybrid osprey
#

Item 28

jagged rock
#

then don't call them statistics

night knoll
#

I am not going to lie, naming in entire project is horrible

jagged rock
#

cool, you know where to improve

dense leaf
jagged rock
#

do it

jagged rock
#

I make them private in java because it's about as long as making them public

#

with kotlin it's just bloat imo

dense leaf
hybrid osprey
# 
    @Override
    public @NotNull List<Component> lines() {
        List<Component> lines = new ArrayList<>();
        lines.add(getDateLine());
        lines.add(Component.empty());
        lines.add(getSeasonDateLine());
        lines.add(getTimeLine());
        lines.add(getLocationLine());
        lines.add(Component.empty());
        lines.add(getPurseLine());
        lines.add(getBitsLine());
        lines.add(Component.empty());
        lines.add(getNetworkLine());
        return lines;
    }``` oof
night knoll
#

would u take a look at items package.

night knoll
jagged rock
#

placeholders + configurable list of strings

hybrid osprey
night knoll
hybrid osprey
#

choose your next words very carefully

night knoll
#

it is just we both were doing same thing

hybrid osprey
#

okay

night knoll
#

oh that

hybrid osprey
#

/**

  • @author Swofty
  • @since 1.0.0
    */
night knoll
#

he coded that for me

dense leaf
#

mhm

hybrid osprey
#

hmm okay

night knoll
#

if by chance u need proof

image.png
hybrid osprey
#

forgiven

#

ActionBarDisplayReplacement can use Duration

#

I feel

night knoll
#

No its fine, since it uses milliseconds mostly

hybrid osprey
#

is there a reason time starts at 1560275700000L and why this is final

night knoll
#

So the time in my server exactly matches the Hypixel Skyblock time

hybrid osprey
#

but why can't this be configurable

#

that's what I'm asking

#

why are you hardcoding this

#

is SkyblockStandardTime just a timestamp

hasty lotus
#

Unrelated, but I recommend coding your own scheduler manager that can relay on the server one instead of directly using the server one

hybrid osprey
#

just use a Date

#

you do all this math work for nothing

night knoll
#

skyblock time works a little different

hybrid osprey
#

the class still mirrors a timestamp

#

you can simplify this a lot to a Date wrapper

#

why is there both Skill and SkyblockSkill

night knoll
hybrid osprey
#

linguistically this distinction doesn't make sense

#

and the Skill enum seems useless

#

if you then have actual Skill classes

night knoll
#

I will convert it into fields

night knoll
hybrid osprey
#

and your skills classes are also missing a few companions

#

I'm working my way up

night knoll
#

there is soo much redundent code rn.

hybrid osprey
#

can you explain what Registry does

night knoll
#

just stores object in a Map with a few methods

hybrid osprey
#

private static final ClickableButtonRegistry INSTANCE = new ClickableButtonRegistry(); ew

night knoll
#

why ewe

hybrid osprey
#

why

#

why is this a singleton

night knoll
#

just storing all different types of GUI Buttons

hasty lotus
#

Registries can be singletons

#

You aren’t modifying it right?

night knoll
#

nope

hasty lotus
#

Then it’s fine

night knoll
# 
        //logic
        try {
            NamespacedId id = NamespacedId.fromString(item.getTag(SkyblockInventory.BUTTON_TAG));
            ClickableItem button = ClickableButtonRegistry.getInstance().get(id);

            button.onClick().accept(player, event);
        } catch (Exception e) {
            player.sendMessage(INVALID_ACTION);
        }```
it allows me to do stuff like this
#

need to rename the registry rq

muted sentinel
# woeful pasture My only cratique doesn't even involve this method. If you're going to wrap Playe...

Taking your advice head on

    public Daily getDaily(UUID uuid, Daily daily) {
        return this.playerData.get(uuid).getDailies().get(daily.getName());
    }

    public void updateDaily(UUID uuid, Daily daily, int amount) {
        Player p = Bukkit.getPlayer(uuid);
        Daily playerDaily = this.getDaily(uuid, daily);
        
        int currentAmount = playerDaily.getAmount();
        int newAmount = currentAmount + amount;
        
        if (daily.getAmount() >= playerDaily.getAmount()) {
            send(p, "TASK COMPLETE");
            return;
        }
        dailyFile.getConfig().set(daily.toString(), newAmount);
    }```
hasty lotus
#

Registries can definitely be singletons as long as you aren’t modifying them

#

Otherwise, you would need to add freezing and unfreezing and stuff like that

night knoll
#

nope they arent being modified

hybrid osprey
#

I feel you can better do a singleton as an enum

dense leaf
#

registries my beloved

hasty lotus
#

After all, that’s the point of a registry right?

hybrid osprey
#

if youre using a class singleton, have a private constructor

dense leaf
#

spark and LP do this a lot and I fucking hate it

jagged rock
#

istg I've seen enums extend javaplugin

night knoll
#

what

hasty lotus
jagged rock
#

ig it's foolproof but it's degen

hasty lotus
#

Actually it’s pretty common in most Java frameworks for the enum

hybrid osprey
#

also I just realized

dense leaf
#

kotlin object >>>>

hybrid osprey
jagged rock
# 
enum MyPlugin extends JavaPlugin {
  INSTANCE;

  @Override
  public void onEnable() {
      ...
  }
}
hybrid osprey
#

why can you not do String.formatted

hasty lotus
#

I personally don’t use this way, but there’s an argument for this

jagged rock
#

can't extend the class by design

#

always a singleton

hasty lotus
#

Yeah

dense leaf
jagged rock
#

zero way to bypass it

night knoll
jagged rock
#

yes

hybrid osprey
#

more weird than calling another method

hasty lotus
#

In my opinion it’s too far fetched

#

No need to go that far

#

It’s just a Minecraft plugin

#

Who gives a fuck

night knoll
#

but yeah MiniString is the new thing for that..I just need to replace all the existing code to use MiniString

hasty lotus
#

You think I’m going to have a class extending LuckPermsPlugin

#

lol

jagged rock
#

type shit I do when some idiot locks down a plugin and has an "unfixable bug"

#

I've extended CraftScheduler before

night knoll
#

y? Scheduler seems fine

jagged rock
#

reasons

night knoll
#

I am enjoying Minestom.

hybrid osprey
#

is SkyblockLevel even used

night knoll
#

nrn

hybrid osprey
#

it's very thin

hasty lotus
#

You should look at my project butter

#

I want you to roast me jk lol

night knoll
#

I kinda went on a package/class making spree, when I got really bored like a week ago

hybrid osprey
# 
  for (io.github.unjoinable.skyblock.item.component.Component component : components.values()) {
            if (component instanceof BasicComponent) {
                ((BasicComponent) component).applyEffect(this, builder);
            }
        }
``` this is minor but I see a lot of improvements possible
#

also why are you using the FQN here

night knoll
#

FQN?

hybrid osprey
#

oh sorry, I see

#

fully qualified name

hasty lotus
#

You can use a stream for that if you want to

night knoll
#

these is probably at max 20 objects

hasty lotus
#

Your current code is fine

hybrid osprey
#

Builder has a generic name but isn't broadly applicable and is a SkyblockItemBuilder

night knoll
hybrid osprey
#

I literally gave you a name

#

read the last word

night knoll
#

oh ok

hybrid osprey
#

public Builder() {} //default constructor for gson maybe I am deficient in GSON but why are you serializing your builder

#

or expecting to

#

a builder is not a thing you store and load

night knoll
hybrid osprey
#

well duh, I read the comment

night knoll
# 
    {
      "material": "INK_SACK",
      "name": "Toxic Arrow Poison",
      "category": "ARROW_POISON",
      "tier": "UNCOMMON",
      "npc_sell_price": 2000,
      "id": "TOXIC_ARROW_POISON",
      "description": [
        "§8Consumed on arrow shot",
        "§7Arrows deal an additional",
        "§7§210% §7of damage as poison",
        "§7and reduce healing by §224%",
        "§2§7over §b4 §7seconds."
      ]
    }```
hybrid osprey
#

okay wait hold up

night knoll
#

that builder only exists for gson

hybrid osprey
#

you need a TypeAdapter son

#

this is not a builder

#

'builder' means a specific design pattern

#

Rarity has ordinal and can be List

#

not hardcoded

night knoll
#

in json?

hasty lotus
hybrid osprey
#

no in the enum

#

why does LoreSystem store a player

#

theres no docs telling me what LoreSystem does

night knoll
#

some components take player as argument to make lore

hybrid osprey
#

why is that LoreSystem's problem?

#

LoreSystem does not use player

night knoll
#

it doesn't for now.

hybrid osprey
#

it shouldnt ever

night knoll
#

why?

#

some components need players

hybrid osprey
#

because then you're having a class do more than 1 thing

#

then you need to let those 'components' get the players another way

#

not by omnibussing it with LoreSystem

#

item category is hardcoded

#

does not need to be

#

blow that up and start over

night knoll
hybrid osprey
#

because you may want other categories?

#

you should not design for hardcoding

night knoll
#

alr sure I can do that

hybrid osprey
#

ItemRecipe is useless

night knoll
#

I will remove all enums.

hasty lotus
#

Unrelated, but I strongly recommend you use a proper locale system

hybrid osprey
#

enums have a purpose

night knoll
hybrid osprey
#

well maybe dont do that

night knoll
hybrid osprey
#

some

#

you can use the existing bukkit Recipe

night knoll
hybrid osprey
#

oh, sorry

#

I don't understand your 'components' at all though

#

could I have a crumb of documentation

night knoll
hybrid osprey
#

group your typeadapter with their subjects

hybrid osprey
night knoll
hybrid osprey
#

Component interface is useless

night knoll
hybrid osprey
#

what implements Component

night knoll
#

all of these (minus ComponentContainer and Component it self)

image.png
#

then the actual components used by items implement these

#

lets say name? pretty basic stuff

public record NameComponent(String name) implements BasicComponent {

    @Override
    public void applyEffect(@NotNull SkyblockItem item, ItemStack.@NotNull Builder builder) {
        Rarity rarity = item.container().getComponent(RarityComponent.class).rarity();
        Component colouredName = Component.text(name).decoration(TextDecoration.ITALIC, false);

        if (rarity == Rarity.UNOBTAINABLE) builder.customName(colouredName);
        builder.customName(colouredName.color(rarity.getColor()));
    }
}```
hybrid osprey
#

BasicComponent semantically is odd

#

Basic tells me nothing

#

yeah, redo your builder and dont use a builder like this

#

that is not what a builder is for

#

and it has no real need to be static inner class

#

split it

night knoll
#

the components that are only applied, but never to be retrieved since it is defined in a json file containing all the default items

hybrid osprey
#

so its a transformer?

#

AbilityCostType is no bueno

#

and that toString reeks regardless

#

ability.abilities is awkward

night knoll
#

where is that?

hybrid osprey
#

item.ability.abilities

#

just as a package

night knoll
#

oh right

#

well it sort of made sense

#

since abilities contains all different abilities

dense leaf
hybrid osprey
#

frankly I would rename things and raise it up

#

flatten that

dense leaf
#

(PackProvider#pack -> BlockBrawlPack#pack -> ResourcePack)

hybrid osprey
#

IslandImpl is a bad name imo

night knoll
#

thats 1 place I tried to avoid enums

hybrid osprey
#

Islands is hardcoded it seems

#

your Island class really lacks meat

#

its very thin

night knoll
#

idk what else to put rn

dense leaf
#

"very thin and boney"

#

nice way to describe classes

hybrid osprey
#

i wanna see some MEAT on these CLASSES

#

I want a THICK IMPLEMENTATION

dense leaf
#

medium rare

hybrid osprey
#

your handlers look useless

#

and I have no documentation to prove myself otherwise

night knoll
#

I know

#

they are to avoid Minestom from spamming console, telling me to register handlers

hybrid osprey
#

at the very least explain that

#

but I'm not sure you fixed anything.

#

more like slapped a silencer on it

night knoll
#

yes

hybrid osprey
#

Button sucks

#

the item is a function?

#

odd

#

task is weird

#

SkyblockMenu includes locale stuff

#

@SuppressWarnings("all") //experimental annotations giving false warnings. this reeks

night knoll
hybrid osprey
#

what experimental annotations

#

You only use @Override

night knoll
hybrid osprey
#

also even if, suppressing all?

#

show me where you use that Experimental

night knoll
#

um um sorry that is not finish

night knoll
#

is experimental

hybrid osprey
#

it's not annotated as such

night knoll
hybrid osprey
#

then suppress on that method

#

not the entire class

#

and not ALL warnings.

#

and you need to write out that justification.

#

never blindly or broadly suppress

night knoll
#

oh okay

hybrid osprey
#

ThrowableStack is an armor stand?

night knoll
#

oh its for like

hybrid osprey
#

save it for the docs

night knoll
#

oh okay

hybrid osprey
#

write it in the documentation

night knoll
#

alright

hybrid osprey
#

but do explain to me why it has a Player field

night knoll
#

to push it with respect to player

hybrid osprey
#

why does it store it

#

you can easily move it to parameters

night knoll
#

it is used in onCollide too

hybrid osprey
#

then you need to reexplain what a ThrowableStack does if it can hurt people

#

and rethink that

night knoll
#

okay

hybrid osprey
#

I don't like SkyblockMob being an enum

#

have a registry of your mobs

night knoll
hybrid osprey
#

NPC also contains locale

night knoll
#

npc class does nothing for now

hybrid osprey
#

loot.loots awkward

night knoll
hybrid osprey
#

private static final Component NPC_MSG = StringUtils.toComponent("<yellow>[NPC] ");

#

this is hardcoding language

#

no bueno

#

loot looks a bit useless

night knoll
#

I am working on it rn.

hybrid osprey
#

Hologram.java and holograms/ ?

#

sus

#

put that under 1 roof

night knoll
#

to store different type of Holograms

#

alr sure

hybrid osprey
#

DamageIndicator

night knoll
#

yes

hybrid osprey
#

getRainbowEffect

#

thinking

night knoll
#

this rainbow effect is a little different

hybrid osprey
#

it wraps around the COLORS

night knoll
hybrid osprey
#

amended

night knoll
#

yea

hybrid osprey
#

SkyblockCollection looks thin and useless

#

and that's all the classes

night knoll
#

it doesn't have any meat as of now

night knoll
hybrid osprey
#

for a first project like this it's not terrible

#

but there are cracks in the foundation

#

but it's a decent start for sure.

#

👍

night knoll
#

I spent alot of time on making that item's system, but at some point I gave up.

#

I have a feel it is too late to convert Statistics to fields instead of enums

#

Unless I add indexes as a param.

steady shard
#

i’m going to write an advent calendar that has 24 challenges for my minigame server and each challenge has its own set of criteria

#

how should I write it so it’s not some massive class or 24 different classes

#

just have like a few different classes for different criteria such as goals or kills and them have one class registering them all?

late smelt
#

Yeah

#

Basically how you would do a quest system

steady shard
#

yeah some guy wrote old quest system and legit did a class for each quest

#

including like

dense leaf
steady shard
#

quest name 1 quest name 2 quest name 3

#

all different classes

#

for the same quest with different levels

late smelt
#

Nah you just make a class for each criteria

steady shard
#

and then on goal and on kill do something like adventmanager.onkill

#

and then it’ll handle all the kill related quests

jagged rock
#

Make a class for each objective

#

and then make a quest class that just holds objectives, requirements and metadata

sly jackal
#

like advancements, there are a set amount of trigger types for an advancement, and then you can combine certain ones and stuff to make the actual advancement

junior holly
woeful pasture
#

looks fine to me Shrug

junior holly
#

Well shit

woeful pasture
#

minigames never really look beautiful tbh

junior holly
#

maybe I did decent job

woeful pasture
junior holly
#

kek

woeful pasture
#

I lied about you doing horrible you did fine

junior holly
#

KEK

late smelt
#

The pinkies are fighting :(

junior holly
#

YOU FUCK

#

How dare you

woeful pasture
#

idk what a RoundState is, but why can you set it 👀

junior holly
#

That was purps doing I had a bug with rounds not starting / ending and that was his fix

#

it works so

#

I aint gonna touch it

steady shard
#

ahaha

steady shard
lunar vale
#

bro

sharp epoch
# steady shard

you know you can put multiple classes in a single file right

#

why do different levels even have different classes, that sounds like bad design to me

lunar vale
#

exactly

sly jackal
#

I hope this is not yours cuz its the opposite of what we told you to do

junior holly
steady shard
#

finished my advent code if you guys would like to review it

#

im chucking it on github now

#

I would appreciate any and all feedback as I've never wrote a system like this before

wooden wraith
woeful pasture
# wooden wraith command: https://paste.md-5.net/menuyobelu.java listener: https://paste.md-5.ne...

if (label.equalsIgnoreCase("openinv")) { you don't need this if statement
you should use if negation more. instead of

if (sender instanceof Player player) {...}

// do
if (!(sender instanceof Player player) {
  sender.sendMessage("This command can only be run by a player.");
  return true;
}

this makes your code far more readable.
Use components avoid legacy text, CommandSender#spigot#sendMessage() You can use something like Minedown or Adventure + MiniMessage to make life easier, Legacy text is 11 years too old.
Don't hardcode your messages, otherwise people who don't speak english can't configure your plugin for their languages or customize it at all

        if (event.getView().getTitle().startsWith("Editing ")) {

DO NOT DO THIS AT ALL NEVER EVER EVER EVER DO THIS.
Otherwise i can defeat your plugin by renaming a chest "Editing ". See this thread
Your entire GUI system is just kinda bad in general the above thread will help with that as well.

woeful pasture
# steady shard https://github.com/XinCraft/advent-codereview/tree/main/advent

Not sure if I vibe with criteria being aware of your database. They probably shouldn't be, not to mention doing a database call that often is uhhh horrible at best.

    public EatCriteria withAmount(MongoClient client, int amount) {
        this.amount = amount;
        this.collection = client.getDatabase("advent-2024").getCollection(getName());
        return this;
    }

Database logic and in memory criteria should be completely separate

#

I see you mixing this logic all over your criteria

#

you should be batching database calls together instead of firing them off right as they happen

#

should probably have an onMatchComplete type deal that sends off all the stats

sharp epoch
#

also legacy text is officially supported on spigot still, so I wouldn't say it is bad to use it

dense leaf
#

that's because choco is slacking smh

woeful pasture
#

its updated

dense leaf
#

then he should force md to merge it

sharp epoch
#

what does Ctrl + Q do in discord, I accidentally pressed it and closed my legcord instance lol

woeful pasture
#

quits

#

I'd assume graceful vs alt-f4

dense leaf
#

press ctrl alt shift W

sharp epoch
#

I thought it just crashed because of a missing feature in legcord or something

dense leaf
#

what the hell is legcord

sharp epoch
dense leaf
#

LOL I remember the arm drama

#

sorry arm©™®

woeful pasture
#

lol

#

wtf

sharp epoch
woeful pasture
sharp epoch
#

it depends on the business, if it is tech-related then you're probably going to have a talk with apple lawyers

steady shard
#

my idea was that it’s called async so it should be fine

woeful pasture
#

Pull, cache, wait, bulk update

woeful pasture
junior holly
#

https://github.com/NormalManV2/NormalDiscGolf/tree/42b7f92a408ecd2de4b1a9b284640f6b8b8ac50c/src/main/java/normalmanv2/normalDiscGolf/common/queue

https://github.com/NormalManV2/NormalDiscGolf/blob/42b7f92a408ecd2de4b1a9b284640f6b8b8ac50c/src/main/java/normalmanv2/normalDiscGolf/impl/manager/QueueManager.java

https://github.com/NormalManV2/NormalDiscGolf/blob/42b7f92a408ecd2de4b1a9b284640f6b8b8ac50c/src/main/java/normalmanv2/normalDiscGolf/impl/service/QueueService.java

https://github.com/NormalManV2/NormalDiscGolf/blob/42b7f92a408ecd2de4b1a9b284640f6b8b8ac50c/src/main/java/normalmanv2/normalDiscGolf/impl/manager/RoundHandler.java

Any advice on this game round queueing system?

round fossil
#

Tbh doesnt look much like a queue to me

steady shard
junior holly
random yacht
#

I don't think your QueueService needs to exist. Pretty much all of that can live within QueueManager, no?

#

Also, maybe you should be introduced to a Queue object 👀 I feel that would be useful

junior holly
#

Probably however I was tired and just trying to use as many objects as possible kek

junior holly
random yacht
#

Yeah definitely not the same. A Queue is just a FIFO/FILO Collection

#

FIFO is more like a proper queue like... a real life line. First in, first out. FILO is more like a stack of objects. Useful in its own right, but FIFO is probably what you want

junior holly
#

Boil it down basically? Where should matchmaking logic be held do you think?

random yacht
#

Well it depends on how you're matchmaking I guess. By what metric? Or do you not care? Just anybody/everybody, if you're in line, you'll queue?

junior holly
#

I guess for now it doesn’t matter that much, but I’ve got a couple data classes that hold references to ratings and division, etc so that will come into play later

random yacht
#

Then yeah you need to throw people into a collection of some sort (maybe just a List is fine) and give everyone a weight by some metric. Maybe it's based on win-loss ratio, maybe it's based on average game score, whatever. Then when you're ready to queue, find the amount of players that you need and try to keep them as close together in weight as possible

#

e.g. a weight of 500 vs a weight of 550 is a lot better than a weight of 100 vs a weight of 1000. The weight difference is far too great. But you need some level of acceptable deviation

junior holly
#

MatchMakingService perhaps?

random yacht
#

Yeah whatever you want to call it. It just needs to have some sort of weight "matching" (for lack of a better term) system

junior holly
#

Hmm

sharp epoch
#

so a priority queue

junior holly
#

Technically I don't think I even need this system for non tournament rounds anyway

random yacht
junior holly
random yacht
#

e.g. you could still have someone queue with 1000 weight but the next entry might only have 100 weight, so then what do you do?

#

You can't just take the next two entrants in the queue. The 100 will get decimated by the one 10x its skill lol

junior holly
#

That's not an issue per se if it's not a tournament round

sharp epoch
#

you implement WRED on top of your priority queue

random yacht
#

I mean yeah if you don't give a shit about SBMM, just throw the next two players into a queue together lol

sharp epoch
#

you may not even have to implement it, there's most likely an implementation out there

jagged rock
junior holly
#

Well at least imo, irl I'm not that good at disc golf but my goal when playing is to beat previous scores on whatever course I'm playing, otherwise if it's a torunament or something then it's actually competative as it were so

#

I think non tournament rounds can go without a sophisticated matchmaking system

jagged rock
junior holly
#

Since when Amazon kek

junior holly
#

The scoring is based on how regular golf scoring works btw

night knoll
round fossil
#

If I’m a Java user, how do the examples look?

dense leaf
#

they look good

round fossil
#

Thinking non existent

#

I mean not sure how Java and Kotlin interoperates with coroutines for example

dense leaf
#

very badly

#

very very badly

rancid fern
#

The bytecode that Kotlin coroutines generate is horrible

#

Actual pain to work with

dense leaf
#

@Metadata usage when

round fossil
#

Or we rm rf Java maybe :D

dense leaf
#

^

late smelt
#

What if we just rm rf Kotlin

dense leaf
#

nah thumbsup

night knoll
dense leaf
#

pretty based

junior holly
round fossil
dense leaf
junior holly
#

Lame

sly jackal
#

packed is open source right?

round fossil
#

i think we need to punish rad

junior holly
#

^

#

No boy kissing for a whole week you’re grounded rad

dense leaf
dense leaf
junior holly
#

It’s harsh but fair

late smelt
#

Kotlin is Kancelled

dense leaf
#

damn

round fossil
#

Suspend fun

sharp epoch
#

just throw the whole packed source onto gemini and tell it to generate a java version

sly jackal
#

Codlin

junior holly
sly jackal
#

probably works out fine

sharp epoch
#

chances are it actually works and takes over packed

round fossil
dense leaf
#

no

dense leaf
junior holly
#

^ why not let rad go through the suffering Gemini will have to deal with in reading all that kotlin

night knoll
#

Do feedback: ❌
Make programming language debate: ✅✅✅✅

dense leaf
#

spigotmc in a nutshell

junior holly
#

Usually how it goes eh?

round fossil
#

Are we debating or just circlejerking

junior holly
#

Ladder

round fossil
#

Seems to be an oddly recurring theme

dense leaf
#

trueeee

#

gonna talk about my addiction to yarn there fr

round fossil
#

And you’re banned (/s)

dense leaf
sharp epoch
#

unless you're making some clear beginner mistakes in which everyone will jump to tell you where you went wrong, because that's easy to point out

#

when it ends up being about design choices, there won't be much of a way to provide sensible feedback there

round fossil
dense pond
#

.

dense pond
rancid fern
#

no

dense leaf
#

bye

rancid fern
#

Not downloading a random rar file from discord

night knoll
sly jackal
#

rawr

woeful pasture
dense pond
#

okay sorry hah

#

how to make it public in git hub

leaden scroll
#

on the bottom

dense pond
#

thanks

dense pond
#

i guess

rich fogBOT
dense leaf
#

why did you commit your build dir

#

your util classes should be final and have a private constructor

#

im not sure whether the j.u.c. scheduler is a good idea

woeful pasture
# dense pond https://github.com/Kmyk441/DailyCommandPlugin 
  •   scheduler.scheduleAtFixedRate(
              () -> Bukkit.getScheduler().runTask(plugin, () -> Bukkit.dispatchCommand(Bukkit.getConsoleSender(), command)),
              initialDelay,
              TimeUnit.DAYS.toMillis(1),
              TimeUnit.MILLISECONDS
      );```

No absolutely not. Please use math and use the Bukkit Scheduler you know you can delay tasks with it right? as Javier pointed out this is fine I didn't think about server lag causing this to be offset and execute at an incorrect time

Within this method is wildly inefficent to be writing to disk every time you add one command. You should do bulk edits and only save to disk on server shutdown or over some period of time like 10 or 15 minutes.
You do the same thing with your timezone

  •   public void setTimeZone(String timeZone) {
      reload();
      getConfig().set("time-zone", timeZone);
      save();
  }```
dense pond
#

do you mean i should do it so user isnt suposted to edit the file?

late smelt
#

Make a reload command

pine grail
dense pond
sly jackal
#

the config file gets read by your plugin, then the file contents are separate from whats in the program

#

and using the api you can either overwrite the file, or re read it in

sharp epoch
desert ore