[MEGA THREAD] Get Your Code Reviewed or Review Others

[random yacht]

Not to say you can't and shouldn't support it, just that it's unconventional is all

[floral inlet]

yeah makes sense

my goal here is just to get used to multi module projects

and redis is pretty easy to implement so

[surreal peak]

so you optimistically send a message that setting a home succeeded and in the case of a db error, you send the error message after you just said it succeeded? \👀

just a design decision, i believe i also send the message after the db call cuz i assume a local db which is fairly fast

[random yacht]

No. You use a CompletableFuture like a normal person lol

[dense leaf]

this is why i love clouds async commands

[surreal peak]

kinda odd to the end user that - in the case of a very slow db - the confirmation message only appears a few seconds later

just imagine

[dense leaf]

just block the main thread

[random yacht]

If you have a very slow DB, you have other problems to concern yourself with

[surreal peak]

facts

[dense leaf]

is 1s considered slow

[surreal peak]

if it doesnt include a network call yes

[dense leaf]

it includes a network call

[surreal peak]

atleast in minecraft land id say

[random yacht]

Even with a network call, that's still relatively slow

[dense leaf]

need to start moving to selfhosted dbs then

[random yacht]

Ideally a database call takes no more than like 200ms

[dense leaf]

i mean i have a selfhosted supabase instance running i'm just not using it

[dense leaf]

tbf it's using postgrest

[junior holly]

I mean doesn’t really make sense to me why they would seeing as there is no brain behind them

It just seems kind of goofy

[sly jackal]

blah blah ancient code blah blah

its probably harder to have them not tick then to have tehm tick a little bit

[junior holly]

Idk y2 said theres 2 ifs behind the ticking

[sly jackal]

exactly

[junior holly]

So then it sounds quite easy to make them not tick lol

[dense leaf]

gonna get banned for sending mojmapped code

[sly jackal]

no

they tick

but its only doing some ifs inside the tick

so its comparitvely like nothing compared to other entities ig

[dense leaf]

this is Display#tick

[junior holly]

Right which again, means it'd be quite easy to make them not tick?

[sly jackal]

idk

yeah exactly, just 2 ifs

most stuff is done on the client

[junior holly]

I mean what is the point of them ticking anyway

[sly jackal]

to stop riding eachother apparantly

[junior holly]

hmm

I guess it makes sense for some entity methods such as passenger ones for example

Beyond that I can't really think of a reason

[sly jackal]

right

I mean rad showed the code

so yeah

you are p much correct

[junior holly]

ya know

I thought that was his code and didn't even bother to look cuz I assumed it was kotlin

guess I should read the message kek

[muted sentinel]

Is this bad practice?

    public boolean homeExists(UUID uuid, String name) {
        for (Home h : getHomesMap(uuid)) {
            return h.getName().contains(name);
        }
        return false;
    }```

[untold jay]

Why .contains and does getHomesMap return a map?

[muted sentinel]

The homes are stored into a map with a home record with the name and location.

[untold jay]

What does the method getHomesMap return?

Also you should not be using String#contains

[muted sentinel]

A set

public HashSet<Home> getHomesMap(UUID uuid) {
        return homesMap.get(uuid);
    }

which that gets the map with the uuid

    private final HashMap<UUID, HashSet<Home>> homesMap = new HashMap<>();```

The name should be getHomes() i guess but i have that already returning a configurationSection

Did i go about this wrong?

also why shouldnt i use String#contains on this?

[jagged rock]

Any reason why it's a set and not something like a Map<String, HomeData>?

[muted sentinel]

It's already in the playerData. I thought it would be kinda redundant to make another manager

[remote kiln]

well, it's unconventional because redis isn't ACID unlike traditional databases (mongo, postgres, etc...), so it's unsafe to use it as a persistent storage

[surreal peak]

dont make another manager, make a Map<string, home> in the playerdata

[round fossil]

I mean it has partial ACID compliance, but even so u can deal with it using replication, sharding, 2phase commits and multiver conc control

ofc redis is still often considered unideal for persistent storage, but yea just saying ^^

(there's also other cool transaction models like BASE or CAP, one more I forgot the name of)

[muted sentinel]

Whats wrong with the way im doing it though?

Also that doesn't store multiple homes

[woeful pasture]

it does?

nested manager within the PlayerData doesn't make sense

Map<String, Home> Would cover anything you need

[muted sentinel]

Why would it be <string, home>?

[woeful pasture]

Home name home data

[muted sentinel]

So <UUID, New HashMap<String, Home>>

[hybrid osprey]

is each player supposed to have a bunch of homes

indexed by some String id

or UUID or something

[muted sentinel]

Yes, A player can have multiple homes. storing it in per uuid ymls.

[hybrid osprey]

okay so why have a UUID to map of string to home

is the UUID a players UUID?

[muted sentinel]

Yes

Thats why im kinda confused here

[hybrid osprey]

a player's data should hold your homes then for that player

[woeful pasture]

PlayerManagwr is okay

They're saying sont add another manager

Just have your PlayerData object

[muted sentinel]

This is within the PlayerData

[woeful pasture]

Why do you need UUID hashmap inside PlayedData

[muted sentinel]

When i tried without it for some reason when i set a new home the other players home changed aswell

[hybrid osprey]

then you have a bug

[woeful pasture]

Sounds like a saving issue

[hybrid osprey]

and you need to figure out why

[woeful pasture]

^

[muted sentinel]

dammit. lol okay.

thanks people

[muted sentinel]

It's because i put the map in PlayerDataManager instead of PlayerData🤦‍♂️

I dont think i tell you guys enough, Thank you for the help ❤️

[muted sentinel]

Updated code java public boolean homeExists(UUID uuid, String name) { return getHomes(uuid).containsKey(name); }

[hybrid osprey]

use this more

[sly jackal]

this is for the weak

[surreal peak]

correct

[muted sentinel]

why use this more?

[round fossil]

its to qualify between static method calls and instance method calls

[sly jackal]

I usually do it the other way around and call static stuff with the class prefixed

since my majority is not that

which also means it still works if I move my code out of the class for any reason

[surreal peak]

my grandma says only incompetent people do that

she doesnt like code bloat

[pine grail]

I use this becuase I like the look of it

[surreal peak]

get yourself a better theme

[pine grail]

I am infatuated with random strings of text

I have gone truly insane 💀

hell I'd hug ok

because it looks so huggable

[dense leaf]

what the fuck

[pine grail]

EXACTLY

[covert marsh]

I also use it to differentiate fields from variables inside deeper scopes (like methods)

fields always have this in my code

[round fossil]

I mean the concept extends to fields ofc

but yea sometimes its even mandatory when inner variables shadow outer variables

Also @covert marsh can u check the staff channel? I need ur opinion on it

[untold jay]

Should my service handle CompletableFutures, or should I give that responsibility to another class?

[round fossil]

CFs can be good api

so maybe

esp if u dont bother to implement ur own futures etc w/e

[untold jay]

I meant should my service return CompletableFutures for the consumer to be used

Or should I wrap my service and make the wrapper return CompletableFutures

[muted sentinel]

What do yall think about string interfaces?

    public interface messages {
        //Homes
        String HOME_SET = "&7Home: &a%s &7set to your location!";
        String HOME_MODIFIED = "&7Home: &a%s &7modified to your location!";
        String INVALID_HOME = "&cThat home is invalid!";
        String NO_HOMES = "&cYou don't have any homes set!";
        String HOME_LIMIT = "&cYour home limit has been used!";
        String TELEPORT_HOME = "&7Welcome to home: &a%s";
        String HOME_DELETED = "&7Home: &a%s &7was deleted successfully!";
    }```

[hybrid osprey]

no

use a languages file

do not hardcode this stuff

[cobalt trellis]

Even better for maximum agony: Make the messages depend on the player's locale

[surreal peak]

might be even more annoying, then that player needs to find a way to change it

[cobalt trellis]

Well, you'll usually not have a player play with non-english minecraft while still insisting that plugins be english

[junior holly]

What if I just want my plugin to have all its messages in differing languages

[surreal peak]

i forgor

[cobalt trellis]

Plus you kinda need an excuse as to why your server is collecting personal data

[desert ore]

ooo selling player data?

[junior holly]

this is not to farm and sell to the Chinese I swear

[cobalt trellis]

Gotta maximise profits somehow, you know

[junior holly]

Encrypted logs? Psh nah those go straight to china

[surreal peak]

now you somehow gotta comply with gdpr bonk

[junior holly]

No one has to know

[dense leaf]

https://docs.papermc.io/paper/dev/component-api/i18n paper has this

[desert ore]

If I work with the CCP I will get a safe place in china during ww3

And that's why we all should sell data to china!

[junior holly]

Idk that id call china safe in a ww3 scenario

[cobalt trellis]

Yeah, I18N is best done in conjunction with adventure

But it's not required

[desert ore]

well, they are very important for the worlds economy and whatever

[junior holly]

That’s why we steal their kids and leave the rest to rot

[round fossil]

I mean there’s something called humble object principle that just says it’s generally a good idea to separate logic itself from asynchronous and concurrent code, but that is not always possible, there’s also a design pattern to return an async interface and a sync interface (idr the name of it), however remember the sync implementation may still need to be synchronized

[muted sentinel]

Now im trying to think of a good way to do lang files -.-

make a new package/class named files/LangFiles and make it instanced.. then a whole lot of getters

[untold jay]

Yeah, I first saw this when I was inspecting the LuckPerms codebase, here's what I have now:

public interface PunishmentService {
    void init() throws DatabaseException;

    void shutdown() throws DatabaseException;

    @NotNull PlayerPunishmentHistory getPlayerHistory(@NotNull UUID target) throws DatabaseException;

    @NotNull PlayerMutePunishmentHistory getPlayerMuteHistory(@NotNull UUID target) throws DatabaseException;

    @NotNull Saved<PlayerMutePunishment> savePlayerMute(@NotNull PlayerMutePunishment mute) throws DatabaseException;

    void savePlayerMute(@NotNull Saved<PlayerMutePunishment> savedMute) throws DatabaseException;
}

public final class AsyncPunishmentService {
    private final PunishmentService service;
    private final ExecutorService executor;

    public AsyncPunishmentService(PunishmentService service) {
        Preconditions.checkArgument(service != null, "service");

        this.service = service;
        this.executor = Executors.newVirtualThreadPerTaskExecutor();
    }

    public void init() {
        this.service.init();
    }

    public void shutdown() {
        this.executor.shutdown();
        this.service.shutdown();
    }

    public CompletableFuture<PlayerPunishmentHistory> getPlayerHistory(@NotNull UUID target) {
        return CompletableFuture.supplyAsync(() -> this.service.getPlayerHistory(target), this.executor);
    }

    public CompletableFuture<PlayerMutePunishmentHistory> getPlayerMuteHistory(@NotNull UUID target) {
        return CompletableFuture.supplyAsync(() -> this.service.getPlayerMuteHistory(target), this.executor);
    }

    public CompletableFuture<Saved<PlayerMutePunishment>> savePlayerMute(@NotNull PlayerMutePunishment mute) {
        return CompletableFuture.supplyAsync(() -> this.service.savePlayerMute(mute), this.executor);
    }

    public CompletableFuture<Void> savePlayerMute(@NotNull Saved<PlayerMutePunishment> savedMute) {
        return CompletableFuture.runAsync(() -> this.service.savePlayerMute(savedMute), this.executor);
    }
}

[round fossil]

this assumes PunishmentService impl is thread safe

[untold jay]

Yes it will be

[glacial stream]

Is there any bad practice in this code especially for performance? Thanks in advance

[jagged rock]

I wonder if your run method could be converted into a removeIf

[junior holly]

https://github.com/NukeCaps/NormalSkyblock

Throwing myself into the gauntlet here, most of the implementation is in NormalSkyblock under impl (obviously), uh I mean theres a lot so I suppose go over whatever you guys feel like ❤️

would also like to point out at that I'm still figuring out my design as I go lol, some things might be in a questionable place

[untold jay]

Why are there so many classes that extend JavaPlugin

Does Bukkit load those classes?

[late smelt]

(No)

Bukkit will freak out if you do that

[junior holly]

I just created new modules and that was what was produced 🤷 , is it wrong?

[untold jay]

Probably

There should only be one entry point

[junior holly]

I was thinking that too, wasnt sure if they needed to be implemented or not or whatever, it's like it created a plugin within NormalSkyblock

[lunar vale]

do you really need a seperate class for the visitor log?

[glacial stream]

Hmm, I could probably remove the player on disconnect but then I still need to have the task to fire some code 2 ticks after the switch item event because the attributeinstance isn't updated yet with the correct item in their hand.

Edit: fixed it by predicting the future so I completely removed the runnable thanks

[midnight spade]

Teach me how to predict the future

[glacial stream]

https://tenor.com/view/delorean-time-travel-back-to-the-future-marty-mc-fly-gif-11295533

[lunar vale]

3

[junior holly]

SRP principal

[junior holly]

Also some advice on staying motivated for a project this large would be incredibly helpful

[random yacht]

Break things up into smaller chunks. Don't see it as "I want to make a skyblock plugin", try and see it as "I want to make a skyblock-style island generator", "I want to make a /spawn and /island command", "I want to make a simple invite system"

If your final goal is "skyblock plugin", that finish line is going to seem farther away for longer. If you move the goal posts to be closer and achieving smaller tasks, you'll feel more accomplished as you finish each little feature.

I did this for a mod I was doing. Piece by piece, I wanted to tackle the project as smaller tasks and was really happy when I saw it working. Then I was like "Oh I can't wait to move on and do this with it!"

[junior holly]

That is very good advice thank you choco!

Also apparently it seems I also have no idea how to setup a multi module maven project so some tips there would help out if anyone would be so kind

[muted sentinel]

this is how ive been doing

    private static LangHandler instance;
    private final FileHandler fileHandler;
    private final String no_homes;
    private final String invalid_home;
    private final String home_set;
    private final String modified_home;
    private final String teleport_home;
    private final String home_deleted;
    private final String home_limit;

    private LangHandler() {
        instance = this;
        this.fileHandler = new FileHandler("", "lang.yml");
        this.no_homes = this.fileHandler.getConfig().getString("NO_HOMES");
        this.invalid_home = this.fileHandler.getConfig().getString("INVALID_HOME");
        this.home_set = this.fileHandler.getConfig().getString("HOME_SET");
        this.modified_home = this.fileHandler.getConfig().getString("HOME_MODIFIED");
        this.teleport_home = this.fileHandler.getConfig().getString("TELEPORT_HOME");
        this.home_deleted = this.fileHandler.getConfig().getString("DELETED_HOME");
        this.home_limit = this.fileHandler.getConfig().getString("HOME_LIMIT");
    }

    public void create() {
        if (!fileHandler.getFile().exists()) {
            Core.getInstance().saveResource("lang.yml", false);
        }
    }
    public String getNoHomes() {
        return no_homes;
    }

    public String getInvalidHome() {
        return invalid_home;
    }

    public String getHomeSet() {
        return home_set;
    }

    public String getModifiedHome() {
        return modified_home;
    }

    public String getTeleportHome() {
        return teleport_home;
    }

    public String getHomeDeleted() {
        return home_deleted;
    }

    public String getHomeLimit() {
        return home_limit;
    }


    public static LangHandler getInstance() {
        return new LangHandler();
    }

}

Is this a good way to go about a lang file?

[hasty lotus]

i think you are better off using a library to handle localizations

and then creating some class to store all your message constants you can then use because you are going to have to make a ton of messages

and i dont think you want one class with like 500 getters in it

[muted sentinel]

do you have a suggestion on a lib?

or recommendation would be a better word to use here

[hasty lotus]

If I'm going to be honest with you, I think adventure is great

they have a very good localization system api

but that'd require you to of course use their components and stuff (which i do strongly recommend)

[dense leaf]

https://docs.papermc.io/paper/dev/component-api/i18n they have this

i am not sure if that is paper specific tho

[hasty lotus]

I dont think so

[muted sentinel]

I was just looking into this lol

[hasty lotus]

you would have to use the bukkit platform module too tho if you want to support spigot

[muted sentinel]

https://docs.advntr.dev/index.html

this what you were talking about?

[hasty lotus]

Yes

[muted sentinel]

I'll have to give this a try, I've never done this stuff before

I've always just hard coded most messages

[hasty lotus]

I use a properties file to store my locale messages which are written in minimessage

i define a custom <arg> tag to substitute arguments

For ex https://github.com/PulseBeat02/MurderRun/blob/main/src/main/resources/locale/murder_run_en.properties

Set the arena's first corner to (<red><arg:0></red>, <green><arg:1></green>, <blue><arg:2></blue>)</gold>

Then I have my own custom minimessage translator here https://github.com/PulseBeat02/MurderRun/blob/main/src/main/java/io/github/pulsebeat02/murderrun/locale/minimessage/MiniMessageTranslator.java and it reads it into a Component

(ignore the checkIfSpecialString method cuz thats for smthing different)

Using that, I store all messages in components here https://github.com/PulseBeat02/MurderRun/blob/main/src/main/java/io/github/pulsebeat02/murderrun/locale/Message.java where I can get them from my classes

it includes the properties key and then i can just call component.build(args...)

You don't have to use a properties file btw, you could use json, yaml or anything cause the translator method just accepts a key and you try to read it from your locale file

[muted sentinel]

Whats better to use though? do you have to reload/restart the server on all of them?

[hasty lotus]

depends on your use case

Are you thinking of using translatable lore on items?

[muted sentinel]

I know txt files you dont. but if im just updating a simple message no need to restart you know?

and probably not do anything with lores

[hasty lotus]

No need to restart it just reads from the Locale

if you just need like a single message then use properties

[late smelt]

I use named placeholders instead of arg

Because users loved named placeholders

[dense leaf]

%s

[hasty lotus]

I've been using the one from the minimessage locale pr

cause its not in adventure yet

they are gonna add it soon

[hasty lotus]

Kk yeah

You would use json for lores cause multiple lines and stuff

i use properties for lores cuz i have text wrapping but you should avoid it

[late smelt]

Minimessage has newline support iirc

[hasty lotus]

it's weird tho like i dont think it carries color components to the next line

[jagged rock]

newline dont work on lores

[late smelt]

I swear I used it for that

Maybe it was chat and I’m insane, idk

[random yacht]

It's iffy in chat, but may work for item lore with plugins

A little split("\n") never hurt nobody

[late smelt]

You can’t split a component like that silly

[hasty lotus]

i think the main issue is that the lore would accept a component[] for the lines, but the properties file would only give you a singular component to work with even if you used \n

maybe the new lines would work if it was the single element in an array, but idk

[junior holly]

https://github.com/NukeCaps/NormalSkyblock

Back again (made a new repo for this project), I believe I have correctly setup the modules this time let me know if you guys find anything questionable or any tips or anything of the sort

[jagged rock]

I'm not a fan of limiting "stats" to a hardcoded set of values

Experience, score etc should all be reworked into a generic "Statistic" that basically acts as a fancy named number formatter

Same with settings. In most cases they're just a fancy boolean wrapper. I remember writing a fully-generic settings system with fancy codecs

Your Settings class not being specific to a player is concerning. If you want server-wide settings consider making feature flags

[floral inlet]

how'd that work?

[jagged rock]

It converts every statistic to a common format (double in my case) and that's how it's tracked internally

It's just a very fancy converter between Double <-> T extends Number

and cached as a Map<String, Double>

[floral inlet]

isn't that basically what he's doing? aside from not using doubles

[jagged rock]

The idea isn't to just have a bunch of getKills getWhatever getThis getThat

But instead a getValue(Statistic<T>), modify, update

[floral inlet]

oh

[jagged rock]

We use kotlin at work so we end up having access to fancier methods

like player.updateStatistic

[floral inlet]

so you'd just have an empty class for each statistic?

[jagged rock]

You have like 5 or 6 statistic classes

DoubleStatistic, IntStatistic

And a class / object or whatever that just holds them

Which I usually call BuiltinStatistics

it's very much a static registry

(ignore the very inconsistent code this is old)

[floral inlet]

oh so you'd just have an extension fn on player that takes Statistic<T> and T to update it?

[jagged rock]

Pretty much

[floral inlet]

why have 6 statistic clases instead of just new Statistic<Long> or Integer

[jagged rock]

The class impl is responsible for the casting and formatting

TimeStatistic uses Longs but renders it in a time-like format

[floral inlet]

o

[floral inlet]

what does that mean btw

(not me using this post as my own btw 😭)

[jagged rock]

In short there's a settings class that's just a Map<String, Object>

It doesn't seem to be attached to a player so I'll assume it's server-wide. My point will still stand under both scenarios

There are 2 paths this can go:

• Settings are loaded on startup: Make it an immutable map, nuke any set methods
• Settings are updated on runtime: Either attach them to a player or use feature flags

The idea of feature flags is that you have a hardcoded set of flags that you can modify on runtime

They basically act as "mutable constants" in the sense where they're never updated in production unless something catastrophic occurs

The benefit they bring is that you can update behavior with no downtime

Now, you can be minimalistic with it, or you can go all out and have them synchronized to some server / use a third-party service that does it

At work I've been introducing this philosophy of "no hardcoded variables, flag everything" just in case something goes wrong and we need to tweak spawn rates / weapon damage / anything tbh, and not only does it make playtesting a breeze because we can tweak numbers in real time, but it also makes development a bit less cluttered

[floral inlet]

but what are feature flags in the first place?

[jagged rock]

fancy T wrappers with a name

[floral inlet]

oh I get it

it's similar to the statistic thing

but then, how do you modify them at runtime?

[jagged rock]

They're mutable

[floral inlet]

they're mutable so you'd just have a set / get method?

[jagged rock]

Yep

[floral inlet]

ah alr ty

[jagged rock]

You modify them with some fancy command and spam get on them

[floral inlet]

and they're constants in the sense that they're predefined

[covert marsh]

List as a registry for that juicy O(n) retrieval. Without having random access to the List (Which is the only purpose for a List)

[jagged rock]

shut up nerd

[covert marsh]

[floral inlet]

would he use a set instead?

or?

[jagged rock]

I get that it should be a map because O(1) which is actually O(N / bucketSize) or whatever

[floral inlet]

o

why not a set tho

[covert marsh]

A Set would be just as bad. Even worse since the iteration on HashSets is slightly slower than those of ArrayLists since you have to concatenate the hash buckets for iteration.

[jagged rock]

a Set is just a HashMap but every value is constant

There'd be no point given we want K-V lookup

[floral inlet]

oh

[late smelt]

Wait it’s all doubles?

Always has been

[junior holly]

Thank you for the feedback illusion, I just woke up so I’m not in a place to respond intelligently lol so I’ll come back later with some more questions probably

Do you think the same applies to settings by the way? I was just trying to boil down these really common things as to make them easier to reuse

[junior holly]

Wait I am just dumb and waking up you were literally talking about settings, I told you I wouldn’t be intelligent yet kek

[surreal peak]

Dont use stream.findfirst, use iterator.next

[willow bone]

singletons vs passing state

[junior holly]

Fair, also these sorta things are subject to change as I'll be implementing (at least for invites) /invite accept (some param to define a specific invite)

Though it works in a couple of my systems for sure, could still be better

Also I assume stream methods are a lot slower than iterators?

[junior holly]

Was this a question btw?

[willow bone]

yes

[junior holly]

Passing class instances for example through di is a more modular approach, it also helps when trying isolate some state in a more effective way. Singletons I suppose you could say is a form of global state management which a lot will argue is static abuse, though most cases you're probably only going to have a single instance of this class anyway, I feel it's still a valid approach depending on your design. In the case of spigot plugins however, it's perfectly acceptable for your main class to be a singleton considering Bukkit only instantiates the main once.

Someone could probably explain this better than I though

I personally use di more however considering it helps me keep organized and what I feel to be cleaner code. I also feel like it helps with tracking down issues and their sources which is again incredibly helpful (especially for someone who's disorganized like I am)

All in all, in the context of spigot, your main class is something a lot will argue should just be passed in the sense of di though if it's a singleton that's perfectly fine as I mentioned above. It more so relies on how you feel your system should work and what you've already got implemented.

@willow bone sorry that took me a while lol

[willow bone]

Not for a spigot plugin, in general

[junior holly]

I'd argue that di is better but I'm probably biased simply because I'm not organized

It helps imo

[surreal peak]

Wydm Itemmeta of air Cant be null

Alzo why cloning item

[junior holly]

Item builder?

[junior holly]

Oh actually you're right I forgot air has no meta right?

[junior holly]

Also I just think an immutable item stack is better

[junior holly]

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

I feel like this may be something to do down the line, but as it stands in the current impl my plan was to have different implementations of settings per things like Player, Island, Rpg stuff, etc, though I feel like feature flags is something a bit more complex. That being said I'm really just trying to get a foundation of which some aspect is playable before making optimizations

[glacial stream]

Hey, so I want to create an enum for my ability keys so I can't make typos when applying them to items, but I also need to make sure every enum constant has an registered ability and every registered ability has an enum constant. The problem is I can't just do everything in the enum because most AbstractAbilities need other classes as parameters.

I came up with these two solutions, I'm not sure if they are SOLID or if I will regret one (or both) of them in the future. Feedback is appreciated.

A: lazy initialization or something like that:
https://pastes.dev/jgC5Ay96p0

B: Requiring every AbstractAbility to take an enum constant as parameter and then checking if every enum constant has an registered ability: https://pastes.dev/vFYaUYGQql

[jagged rock]

The way I usually do it is with a factory pattern

Map<AbilityType, AbilityProvider> where AbilityProvider creates an Ability instance with whatever params you need

But overall method B seems to work better

[glacial stream]

Alright thank you 🙏

[charred forge]

Would the Enum Interface trick work for you?

enum Example implements Interface {

}

Then for each enum constant, you have to implement the methods of the Interface

https://github.com/TheNewEconomy/TheNewItemLibrary/tree/feat/cleanup mainly looking for feedback on the recode(feat/cleanup branch) of my internal Item Library I use for plugins. Pretty much just lets me serialize/deserialize ItemStacks for JSON across multiple platforms(Paper/Spigot/Sponge/Fabric soon), and build item stacks using a builder pattern without worrying about the platform-specific stuff so write the build once and use it for any of the supported platforms. The rewrite mainly was to clean it up and allow for additions without needing to include them into the project itself with the goal being to do something like ItemPlatform.addCheck(new PersistentCheck()) in my plugins. Please note: not completed in the current state, but fairly useable for the bukkit/paper part.

Example usage just to get an idea(from the old code base, but will be similar to the new) provides an itemstack for any of the platforms without changing the code itself

PluginCore.server().stackBuilder().of(denomination.getMaterial(), amount)
            .enchant(denomination.getEnchantments())
            .lore(denomination.getLore())
            .flags(denomination.getFlags())
            .damage(denomination.getDamage())
            .display(MiniMessage.miniMessage().deserialize(denomination.getName()))
            .modelData(denomination.getCustomModel());

[surreal peak]

what do you need those since comments for, git exists

[dense leaf]

for stuff like the spigot api it's very useful tbh, there's also the 1.8 people who want support lmao

and it's inconvenient to check the git for when the things got added

[charred forge]

It lets developers know that they should only expect that method to exist at that version

Especially when dealing with something such as ItemMeta that is volatile(see how they changed pattern type between interface/class)

Not really needed when dealing with sponge since the item data is stored with keys, but for spigot it's a great indicator on the Javadocs without having to read through Github commits.

"Hey I made this documentation you can use without digging through everything, but you should dig through everything anyways because you may not have access to a method on a certain version"

[cobalt trellis]

Yeah @since is one of the hallmarks of good API design

[charred forge]

I wouldn't go that far xD

[cobalt trellis]

Good APIs are APIs that can last.
If the API changes all the time to the point that @since is not useful, you got a serious issue with API maintenance

[charred forge]

true

mostly why I overhauled it to support components which seems to be where Mojang is taking all meta data

[cobalt trellis]

@since in itself is not /that/ important, but the implications of it are.

[charred forge]

heck, the food component is already outdated because mojang change its usage in the snapshots

[muted sentinel]

I think im getting better at this... i mean this was first try and compiled. worked no errors

There will be a reward system but i wanna see if im doing this better

[jagged rock]

Problem with the currenct structure is that you're going to end up with a lot of duplicate checks

I like to structure my quest systems around a "Quest" that's broken into "Metadata" (name, description etc), "Objectives" and "Rewards"

Might add requirements if you're really fancy

An objective has an ID and type, but also tracks per-player progress

And rewards can be commands, items etc

[muted sentinel]

A MetaData? What does this mean?

[muted sentinel]

What do you mean by a lot of dupe checks?

[tall trout]

?paste

[rich fog]

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

[tall trout]

what could i improve + tips: https://paste.md-5.net/nasovezaqa.java

[tall trout]

Meta data is what makes something unqiue such as the meta data for an item could be the name, the count, the lore, ect

[muted sentinel]

To me this all seems really redundent

private Ability1Handler(JavaPlugin plugin) {
        this.plugin = plugin;
        this.wardensAPI = WardenSMP.getInstance().getWardensAPI();
    }

    public static void initialize(JavaPlugin plugin) {
        if (instance == null) {
            instance = new Ability1Handler(plugin);
        }
    }

    public static Ability1Handler getInstance() {
        if (instance == null) {
            throw new IllegalStateException("Ability1Handler not initialized");
        }
        return instance;
    }```

But i'm still new to programming

 public void start(Player p) {
}```
Pass UUID instead of player

This can cause memory leaks

[muted sentinel]

Same with getCooldown() & startCooldown()

WardensAPI can be final

[woeful pasture]

passing Player itself can't cause memory leaks

but storing it past the time where the player does, indeed causes memory leaks

[jagged rock]

Regardless if you use a UUID or a player it'll still leak

But if you use a UUID you contain the leak and it kinda fixes itself if the player ends up rejoining

[untold jay]

Is it fine to abstract an unit test so I can test multiple implementations of an interface without a lot of duplicate code?

[surreal peak]

wdym

implementations must correspond to contracted behaviour

[untold jay]

I have a service interface, and its implementations all use different technologies, so to test it, I want to know if I should make an abstract unit test, then in a concrete unit test I can test the service implementation

[woeful pasture]

whatever is easier for you

tests are for you anyways as long as they cover what you need to be covered I see no harm

[surreal peak]

just provide a parameterized test for all four impls

[tall trout]

so like this:

        Player p = Bukkit.getPlayer(playerUUID);
        PlayerData pd = wardensAPI.getPlayerData(p);
        List<String> ability = pd.getAbilities();

        if (ability == null || ability.isEmpty()) {
            p.sendMessage("No abilities found.");
            return;
        }

        String element = ability.get(0).toString();
        COOLDOWN_TIME_MILLIS = getacooldown(element);
        long cooldownEndTime = getCooldown(p);

        if (!isOnCooldown(cooldownEndTime)) {
            startCooldown(p);
            switch (element) {
                case "dash":
                    doDash(p);
                    break;
                case "windblow":
                    doWindBlow(p);
                    break;
                default:
                    p.sendMessage("Unknown ability: " + element);
                    break;
            }
        } else {
            p.sendMessage(getCooldownMessage(cooldownEndTime));
        }
    }```

[muted sentinel]

Yeah, Also looks like WardensAPI is passing player aswell. I'm still learning but this is what i've been told. Use UUID for everything you can

If you have access to WardensAPI i would redo all that aswell.

[tall trout]

I don't have access to the warden API so is it still worth it or should i just pass the player in this case

[muted sentinel]

nope, uuid

Don't forget to pass it in your other methods too! 🙂

[tall trout]

K thanks for the tips!

[jagged rock]

This would greatly benefit from SOLID concepts

Check the pinned message on #help-development

[glacial stream]

So I have a runnable that does stuff every tick for every online player. https://pastes.dev/uLpaIHZoHs
Like sending and actionbar. But now I want to do more stuff every tick for every online player, like regenerating mana.

Should I put this and future tasks that require the same players and timing in the same runnable and break SRP, or create a separate runnable, listener and collection for every different task and have duplicate code, runnables, listeners and collections?

[jagged rock]

Ideally you should attach some sort of "entity data system" that tracks data and ticks a given entity

For example at work I made a system that we call "entity components" where you can add components to an entity that let's say, run a method every tick for that given entity

[surreal peak]

designed an ecs by mistake...

[glacial stream]

Do you mean something like this? https://pastes.dev/oBZ9F7SQX1
Or would this store the tick method in memory for every player even though it's the same for every extension of AbstractData apart from the parameters?

I'm quite lost about what would be a SOLID implementation for something like this.

[jagged rock]

Depends if the data needs to be ticked or not

Different component types do different things

[surreal peak]

removeIf please

[glacial stream]

Alright thank you both

[muted sentinel]

How much hate can i get for this? 😂

   public void rewardPlayer(UUID uuid, Task task) {
        Player p = Bukkit.getPlayer(uuid);
        for (String s : task.getRewards()) {
           if (s.contains("CMD:")) {
               Bukkit.dispatchCommand(Bukkit.getConsoleSender(), s.replace("CMD: ", "").replace("[player]", p.getName()));

           } else if (s.contains("ITEM:")) {
               String item = s.replace(" ", "").replace("ITEM:", "").replaceAll("\\d","");
               int quantity = Integer.parseInt(s.replaceAll("[^0-9]", ""));

               ItemStack itemStack = new ItemStack(Material.matchMaterial(item), quantity);
               if (Material.matchMaterial(item) != null) {
                   p.getInventory().addItem(itemStack);
                   return;
               }
               System.out.println(item + " is invalid!");
           }
        }
    }```

[dense leaf]

yes

[muted sentinel]

I got drunk last night and tried to code. 😂

I didn't just had an idea in my head and tried to make it work

This is what its getting 😂

    Rewards:
    - 'ITEM: diamond 5'```

Now thinking about it instead of a stringlist i should be using a ConfigurationSection like

Rewards:
  Item: diamond
    Quantity: 5```

Redoing this

[round fossil]

Wont work

[muted sentinel]

what wont?

[random yacht]

That YAML snippet isn’t valid

You can make a list of objects but you can’t make an object that also has a value

e.g.

Rewards:
- Item: ‘minecraft:diamond’
  Count: 5
- Item: ‘minecraft:emerald’
  Count: 8

[muted sentinel]

Yeah i was just writing it on a fly, i getcha

[random yacht]

Only downside of object lists is that they’re syntactically awkward

[muted sentinel]

Yeah, im actually learning that now and its annoying 😂

adding it into a hashmap i should say

[random yacht]

Well just for the end users

YAML sucks lol

[late smelt]

It’s also a bit weird to handle via the api

Since all you really have is getMapList

[surreal peak]

yaml cringe

[dense leaf]

toml best

[random yacht]

Someone needs to add getSerializableList()

[muted sentinel]

God dammit, i posted it in the wrong area

sorry

[sly jackal]

nbt best

[woeful pasture]

I stg these end up looking so much better in JSON

[random yacht]

Well, yeah, they do. It's just a list of objects

"Rewards": [
    {
        "Item": "minecraft:diamond",
        "Count": 5
    },
    {
        "Item": "minecraft:emerald",
        "Count": 3
    }
]

[surreal peak]

wondering if theres a "cleaner" way :/

[random yacht]

You're checking opt != "" twice. No reason that can't be inverted and continued, right?

[sly jackal]

where would you set opt_set = i then

[hybrid osprey]

what language is this

[dense leaf]

odin

[remote kiln]

english

[willow bone]

한국어

[surreal peak]

what

[willow bone]

its korean

[tall trout]

        shooter.getWorld().playSound(shooter.getLocation(), Sound.ENTITY_ENDER_DRAGON_SHOOT, 2f, 1f);

        Location startLocation = shooter.getEyeLocation();  // Start the beam from the player's eye level
        Vector direction = startLocation.getDirection();    // Get the direction the player is looking

        double maxBeamLength = 50.0;

        new BukkitRunnable() {
            double currentLength = 0;

            @Override
            public void run() {
                if (currentLength > maxBeamLength) {
                    this.cancel();
                    return;
                }

                Location currentLocation = startLocation.clone().add(direction.clone().multiply(currentLength));

                shooter.getWorld().spawnParticle(Particle.END_ROD, currentLocation, 10, 0.2, 0.2, 0.2, 0.01);

                for (Entity entity : shooter.getWorld().getNearbyEntities(currentLocation, 1.5, 1.5, 1.5)) {
                    if (entity instanceof LivingEntity) {
                        LivingEntity livingEntity = (LivingEntity) entity;
                        if (!livingEntity.equals(shooter)) {
                            livingEntity.damage(3);
                        }
                    }
                }

                currentLength += 1.0; // Increase beam length
            }
        }.runTaskTimer(WardenSMP.getInstance(), 0L, 3L); // Run the task every 2 ticks (0.1 seconds)
    }```

Nothing partially wrong just looking for tips and things to improve

[surreal peak]

dont create new bukkitrunnables, use the scheduler

or use an inner class

[random yacht]

I mean tbf that's a suitable way to schedule tasks, but yeah, ideally you use the scheduler. Less nesting involved

Is there any reason you're cloning startLocation and direction though? Doesn't seem you're using them anywhere aside from in that loop. You could save yourself lots of object allocations if you just do something like this:

public static void doLightBeam(Player shooter) {
        shooter.getWorld().playSound(shooter.getLocation(), Sound.ENTITY_ENDER_DRAGON_SHOOT, 2f, 1f);

        World world = shooter.getWorld(); // <----- Save this in a variable so you don't have to keep fetching it
        Location location = shooter.getEyeLocation();  // Start the beam from the player's eye level
        Vector direction = location.getDirection();    // Get the direction the player is looking

        final int iterations = 50; // <------ this can just be a simple int now

        new BukkitRunnable() {
            int iteration = 0;

            @Override
            public void run() {
                if (iteration > iterations) {
                    this.cancel();
                    return;
                }

                iteration++;
                location.add(direction); // Mutable, and direction is normalized so it adds by 1

                world.spawnParticle(Particle.END_ROD, location, 10, 0.2, 0.2, 0.2, 0.01);

                for (Entity entity : world.getNearbyEntities(location, 1.5, 1.5, 1.5)) {
                    if (entity instanceof LivingEntity) {
                        LivingEntity livingEntity = (LivingEntity) entity;
                        if (!livingEntity.equals(shooter)) {
                            livingEntity.damage(3);
                        }
                    }
                }
            }
        }.runTaskTimer(WardenSMP.getInstance(), 0L, 3L); // Run the task every 2 ticks (0.1 seconds)
    }
}

Everything else honestly looks fine

[surreal peak]

di 👉👈

[frank lotus]

How would you simplify this without losing clarity? Or what would you change?

colors:
  default:
    need_permission: false
    permission: "holotools.holocrafter.color.default"
    slots:
      default_slot: light_blue
      slot_0: blue
      slot_2: blue
      slot_4: blue
      slot_6: blue


colors_lists:
  blue:
    unselected:
      hex: "#19a7d2"
      alpha: 150
    selected:
      hex: "#28bae6"
      alpha: 200

  light_blue:
    unselected:
      hex: "#24dce5"
      alpha: 150
    selected:
      hex: "#5be4ec"
      alpha: 200

[glacial stream]

If need_permission is false you shouldn't have to configure the permission right? (You can check if the permission object is null and if it is assume need_permission is false).

And unless colors will have more keys in the future you can remove it and make default the highest one

[hybrid osprey]

what exactly is this for

[jagged rock]

Alpha can be part of hex

[sly jackal]

making default the highest one is sure to cause issues, I think this format is fine

[frank lotus]

It's so you can configure colors for ranks, and basically, each panel is a slot
https://cdn.discordapp.com/attachments/849481473496580116/1282816769937117288/2024-09-09_02-40-05.mp4?ex=66e0bbb7&is=66df6a37&hm=6c221134436af23a75c5f046406cc9c9ee880a271ed9c788d2673a857814dffe&

[sly jackal]

why does the slot number have increments of 2

[dense leaf]

are you just using item displays for the background

[frank lotus]

In the config I sent, there are only 2 colors, and since slot_1, slot_3 is not defined, it uses the default color, making it look like this:

[frank lotus]

TextDisplay

[sly jackal]

oh okay so its an optional thing

i see

[frank lotus]

Yes, it's the only thing I could think of to simplify the config and avoid having to specify 8 slots all the time 😅

[sly jackal]

yes, its a good idea

yeah other than the suggestions already made by people i think its fine

not much more you can do without removing information

[dense leaf]

why

[frank lotus]

Because it can be set to transparent

[dense leaf]

okay but why not use an item display

[frank lotus]

Because I want it to be see-through. Do ItemDisplays allow you to set transparency?

[dense leaf]

yes

item models also allow for transparency

[frank lotus]

But with a resource pack, right?

[dense leaf]

yea

[frank lotus]

I don't like that >.<

[dense leaf]

why exactly

you can make your plugin generate a resource pack

which then works with other ones on the server

[sly jackal]

i think there is a case to make for no resourcepack

like if its possible without resourcepack with seemingly the exact same look, why not

[dense leaf]

well you can achieve a lot more tho

visually speaking

[sly jackal]

yes

but if you dont have to

like if you are already using one then ofc but if you dont use one yet and dont plan to, why go through the trouble

[frank lotus]

Exactly. I prefer to do it this way and add optional features with resource packs if the end user wants them

[sly jackal]

yeah same for my plugin

[glacial stream]

How can text turn into a colored background without a resourcepack?

[sly jackal]

#help-development

nvm

its related to the convo

I guess its by using the text display background

and then just haveing some arbitrary text with invisible color

[late smelt]

You can set the text display background (including alpha)

Then I think you can just fill the text display with spaces

[sly jackal]

oh right spaces

that makes more sense haha

[glacial stream]

Oh cool, thanks

[frank lotus]

Thanks for the help @sharp epoch. This is how it looks in the HoloWardrobe 🐱

[junior holly]

That is very cool

[hybrid osprey]

That is amazing

[sharp epoch]

that is actually really cool, I like it

I love the amount of stuff display entities allow us to do nowadays

[dense leaf]

Not a big fan of the empty slots that say "Air" on hover tbh, otherwise really awesome!

[late smelt]

How did you do the tooltips

[rancid fern]

Text Displays

[late smelt]

Well yeah

But how does one get all the data from the item to the display

Is it just a bunch of checks for stuff like attributes

[rancid fern]

You can get the tooltip from nms

Which is what they're doing

[late smelt]

I see

API when?

[junior holly]

https://tenor.com/view/mr-krabs-back-to-work-spongebob-get-back-to-work-krusty-krab-gif-16474125980856176144

[sharp epoch]

||ItemStack#calculateTooltipLines in Paper||

the tooltip is gotten from the component list, so it should be relatively easy to make an API for it (each component with a tooltip info has an appendHoverText method, I think there's a method in ItemStack that collects them for you)

[late smelt]

Maybe once we have component api merged

[random yacht]

That feels like it should be a client-only method, no?

Why would the server have any use for that whatsoever?

Yeah. Its only uses are in the client. Weird that that's not stipped from the server and even weirder that Paper added API for it

[late smelt]

The server has to use it at some point, right?

[random yacht]

Nope

3 method invocations, all on the client

Either for searching or for rendering

[late smelt]

Maybe Mojang is just being nice

[random yacht]

They probably just don't strip out code that isn't under the .client package

[late smelt]

They don't strip a lot of things these days

Like GameTest

[random yacht]

I think that's intentional. Pretty sure they mentioned it in a snapshot at one point

[late smelt]

yeah

I assume they don't strip any methods that are on server classes

[dense leaf]

YOU USE ECLIPSE??

[random yacht]

Yes?

This isn't news

[dense leaf]

WOW

[late smelt]

Hypixel loves him for it

Real

[random yacht]

They actually finally added a better, in-lined search widget which is great

[late smelt]

I don't see Mojang removing it any time soon, so I'd say it's fine for an API

[random yacht]

Death to search window

[random yacht]

Hesitant. Last time I added API for methods that were on the server but were used exclusively by the client, we ended up having a dead API that doesn't work anymore

RIP to CreativeCategory. Lived for like 2 versions

[late smelt]

lol

[muted sentinel]

Is this too much for one object? Should i split it up into more objects?

    public Task(String name, String description, String icon, TaskType type, int amount, @Nullable String material, @Nullable String word, boolean repeatable) {
        this.name = name;
        this.description = description;
        this.icon = icon;
        this.type = type;
        this.amount = amount;
        this.material = material;
        this.word = word;
        this.repeatable = repeatable;
    }```

Maybe Task, TasksRequirements, TaskRewards, TaskType?

[hasty lotus]

You could also use some builder for it if you wanted to also

[rancid fern]

Record + RecordBuilder

[muted sentinel]

Actually already changing it to record

public record Task(String name, String description, String icon, TaskType taskType, boolean isRepeatable) {

[hasty lotus]

https://github.com/Randgalt/record-builder

This looks really cool

[rancid fern]

Yeah it is nice

[hasty lotus]

You could also use Lombok if you wanted to

Or wait nvm that’s different purpose

[muted sentinel]

Why is it everytime i look at an API i get overwhelmed?

[hasty lotus]

Well actually @Builder I guess

[random yacht]

I don't know why it chose to do TypeBuilder.build(). That seems redundant, no?

Couldn't they have generated a Type.builder() method instead to return the builder type?

I also really don't like the removal of new using a static factory method lol. But to each their own I suppose

[woeful pasture]

I've always wanted to make something along these lines but making IDE plugins is such a real turn off

[random yacht]

You don't need to make an IDE plugin. You just need to make an annotation processor

The IDE should handle the processing

[woeful pasture]

I've always been under the impression you'd need something to understand the methods are actually there

[random yacht]

Yeah. That's what the annotation processor is. It generates code. But the IDEs know how to deal with the code that an annotation processor generates

It just has to be aware of it

[woeful pasture]

huh neet

[random yacht]

Lombok's got an installer for Eclipse, but all it does is add the agent to the ini file

It's not a plugin

[woeful pasture]

its a plugin on intellij and VSCode too

which is why I was confused its the only annotation processor I've ever used

[sharp epoch]

what do you use to look into client code? Yarn mappings or neoform?

[random yacht]

I just had a Fabric environment open at the time

Mojmaps

[rancid fern]

I usually use Paper userdev to look

[late smelt]

Hypixel is making a fabric client-side addon mod

Confirmed

[random yacht]

Nah I make my own enhancement mod for small quirks I think would be useful lol

We do have an official modding API though, finally. It's extremely primitive, but it exists

https://github.com/HypixelDev/ModAPI

WIP obviously

[dense leaf]

via the PMC lmao

[frank lotus]

I don't think they'll remove it. The code, or at least the method, has been in the ItemStack class since version 1.17. (I really hope they don't remove it, or I'll have to do it manually again, like I was doing at the start 💀)

[sharp epoch]

that's the standard way for server and clients to communicate on modding platforms

mojang added them for that very purpose, rather

https://web.archive.org/web/20220426101624/https://dinnerbone.com/blog/2012/01/13/minecraft-plugin-channels-messaging/

[late smelt]

It uses a built in packet

Meaning clients without the mod won’t freak out

Or servers without the mod

[red bison]

https://github.com/HuskyDreaming/Settlements Can somebody review this code (You might need to look at HuskyCore also), and would be nice if someone could join the testing server to test

[jagged rock]

Not a fan of the collapsed if statements

If you're opening braces on for loops I feel like you should open them on if statements too

[red bison]

Thanks, I will do that better. It's a good practise

[jagged rock]

Hoping there's an isEmpty method available. Computing the size often takes longer than just checking if an entry exists

I'd also like to see more registries and less enums. If I want to hook into your plugin and add a flag type I'd need to hard fork it

[red bison]

There isn't 😦

[jagged rock]

I feel like DependencyType is stupid, should rely on interfaces instead

And your Placeholder expansion seems to have a bunch of duplicate code. could rely on even more interfaces

Not sure what versions this plugin supports but having hard references to locations can result in memory leaks below 1.13.2 if the world is unloaded

[red bison]

This plugin should only be downloaded on the latest versions, I am going to make an update soon

I don't like supporting lower versions

[jagged rock]

I also feel like your Config class should be immutable. Config files are meant to be read-only at runtime

[red bison]

Needs to be mutable because players can change configuration in game via GUI 😉

[jagged rock]

And I'd like to see you split your API into an API module. That allows you to expose an API without any of the implementation quirks

It also feels nicer to import a com.huskdreaming.settlements.api.whatever package instead of com.huskdreaming.settlements.services.interfaces package

As for package naming, it makes sense as a developer to toss all your enums in a corner but I tend to organize my packages by feature instead of by what they contain

[red bison]

Sounds good, I will make sure to organize it better and make it friendly for other developers to use

[jagged rock]

Now, on your API, you should also validate the inputs

Because there's going to be an idiot tossing null players at some given point

It's fine to throw an exception but make it clear it's their fault, not yours

[red bison]

Good idea, will make sure

[jagged rock]

On your flag service, it isn't clear what this runnable's purpose is. If it is a callback, try using futures

Something like

CompletableFuture<FlagOperationResult> addFlag(Settlement settlement, FlagType type);

That allows them to chain into the future if they'd like a callback, or ignore it completely if they're just going in blind

The result can be an enum that can, for example, tell if the flag was already set, if it's successful or if an exception was thrown

This can be reworked into ProtectionHook and FactionHook or something

Because while it might not be worldguard protected it can still be claimed with something else

[red bison]

Yeah and it’s technically also not a service. I will make a better interface for them

[jagged rock]

hasNoInvitation is negated by default. Prob your IDE yelling at you but this should not be negated

and what do the longs even mean

yikes you're returning a mutable set

[red bison]

Oopsie, got lazy

Do you suggest collections.unmodifiable or something else?

[jagged rock]

This naming makes no sense. call it init or something

[jagged rock]

Set.copyOf will do the trick

what if settlement was immutable and this was just a builder 🤔

Do these really need to be a set? what stops it from just being a generic collection

[red bison]

Yeah I was thinking of making it a builder. Wasn’t sure if it was overkill because I only need todo it once but yeah 100% agree on it being immutable

[rancid fern]

Record + RecordBuilder

https://github.com/Randgalt/record-builder

[red bison]

@jagged rock anything that you see that is a big problem or overall looks pretty okay?

Just some minor changes and quality of life improvements?

[jagged rock]

Make sure you're not doing IO on the main thread type deal

haven't seen your repository impl

more nitpicky: this creates multiple instances of the same object. Reuse your variables

[rancid fern]

Add a gitignore to remove the .idea folder, target folder and dependency-reduced-pom.xml

[red bison]

Yeah sometimes it’s just me being lazy

Overall good code?

[jagged rock]

decent

I won't ask too many questions here

[red bison]

Yeah 😬🫣

[clever crag]

https://github.com/MinesonTeam/MinesonAPI

[dense leaf]

epic markdown fail

[hasty lotus]

You should be using read and write locks and making sure your configuration class is thread safe (for example synchronized)

[clever crag]

omg

[cedar drift]

https://github.com/cbhud/Castle-Siege

[woeful pasture]

• you should exlclude your .idea folder
• Don't call your main class Main for rational see https://www.spigotmc.org/threads/why-you-shouldnt-name-your-main-plugin-class-main.493289/
• I'd personally avoid star imports though that's up to you
• avoid § instead use Bungee's ChatColor class instead, Most keyboards don't have the section symbol also ChatColor constant use makes the goal of the string join more clear and less arbitrary e.g. §l could be confusing to someone where as ChatColor.BOLD lacks any such confusion
• Don't run database queries on the main thread its a good way to cause the server to freeze or possibly time out if queries run long or too many of the action happens at once. In your case its players joining. You should really be caching this stuff instead of just going raw straight from the databse on the main thread. see PlayerConnection#L38
• Don't hardcode messages. You already have a messages config I don't see the reason for any of these messages to be hardcoded.
• I wouldn't hardcode kits. Make kits configurable Ideally you should have a very rich configuration system for kits. Otherwise things get dull from a customization standpoint. Likewise don't hardcode kit price etc. The user should beable to configure these things.
• PlayerKitManager#getKitPrice is just a bad method it makes no sense. If you're switching over a enum and you don't want to make anything configurable why not just bundle kit price in the enum?
• I wouldn't start import PlayerStates in PlayerManager it gets confusing especially with the clash between PlayerStates.SPECTATOR and GameMode.SPECTATOR

• Especially on spigot DO NOT USE INVENTORY HOLDER EVER see rational https://www.spigotmc.org/threads/why-items-with-lots-of-metadata-actually-cause-lag-an-inventoryholder-psa.607711/ instead follow this guide for sensible inventory gui design https://www.spigotmc.org/threads/a-modern-approach-to-inventory-guis.594005/
• Exclude the dependency-reduced-pom.xml from git it doesn't need to be there for git
• In your kit selector I'm confused behind why you define the ItemStack variables before assignement? The naming of them too is just bad you aren't a compiler KitSelected#L24-L67
• Your entire GUI design is primitive, but switching away from InventoryHolders should help your design especially with the guide thread above.
• Another nitpick is just that this code plain and simple isn't reusable at all if you were to ever make a minigame again. The lack of a more generic Team definition or kit system just leaves a lot to be desired both from a server owner and developer perspective. You could make things way more configurable than they currently are with a tad of redesign

[sharp epoch]

Eh, one doesn't necessarily have to exclude the whole idea folder, there are environment settings there that could be of use

see https://intellij-support.jetbrains.com/hc/en-us/articles/206544839-How-to-manage-projects-under-Version-Control-Systems

also, what is wrong with star imports? They're fine, in fact, Java is encouraging to go towards that route with module imports now

[woeful pasture]

They don't take advantage of .idea configurations that would benefit from .idea being g included

[woeful pasture]

Nothing per se it's stylistic, though I'd say personally I don't like them because it makes it less clear what I'm using from each package. I like to know especially when refactoring

[cedar drift]

Thank you i have improved so many things if you have time check it now!

https://github.com/cbhud/Castle-Siege

[sharp epoch]

java 8 and spigot 1.17.1

[sharp epoch]

also unrelated, but you should put a license on that repo

something like MIT will do if you don't know which one to choose

nitpick: the comments look kinda over the place, TODOs above the package declaration (only thing that usually goes there is the license header), // Added this field comment which is just ??

[junior holly]

same nitpick from me, I really don't understand these kinds of comments

[dense leaf]

smells like GPT ^

[sharp epoch]

the code seems fine, but it is kinda all over the place, the abstractions don't have a sense of purpose

[junior holly]

I don't like the nesting here and from what I've seen, everywhere else, use guard clauses instead

[sharp epoch]

we're really just nitpicking since the code is fine as is lol

[junior holly]

I've always been told, "ask for a review, receive a review."

[alpine anvil]

thats the point of the channel

[junior holly]

I think the biggest point of having your code reviewed is so that you can fix these nitpicks and ultimately make your code better

[sharp epoch]

my major trouble with it is just the fact that it is using java 8 really, and I would personally work out more the architecture of the plugin itself

[junior holly]

Otherwise if it works, why do you care to have the code reviewed

[sharp epoch]

I think it really depends, in this case there's really not much room to improve anything since most of what's been suggested are majorly stylistic choices

[junior holly]

Again back to this point

You can't argue the fact that they asked for this, at the end it is their choice, we just offer the advice as you imply

[sharp epoch]

oh, I'm not saying to not review the code, I'm simply emphasizing to the dev that there's nothing to really take at heart here

if they make the choice to follow what we're saying then good, but either way would be fine

[junior holly]

If it's a nitpick now, it's probably a nitpick later. Why not practice writing your code in a way that looks cleaner and is easier to read? If the nitpicks were meaningless then again there would be no reason for a review unless some other major issue was the focus

But as it seems, this project is complete and only what I can assume to be tested, therefor the nitpicks are what this person is after

[sharp epoch]

nitpicks are meaningless, hence why they're called nitpicks

[junior holly]

Not if it's what the person is after

Nitpicks in the way you put it are stylistic choices, in which that person may find value in

so really they have subjective value

It's all we can do to offer as advice because again, this project seems to be complete

[sharp epoch]

if anything, I like the fact that the code is simple, they're not trying too hard to make things complex

it is really easy to fall down that path the bigger your project gets

[junior holly]

My nitpick about the guard clauses just make it easier to read imo

But hey if it works it works and it's only up to them if they want to make any changes

[spring reef]

2 days without activity, i will throw one into the round:
https://github.com/godcipher/gutil please for review :)

[woeful pasture]

I think the only thing I notice is I'm not sure if I vibe with the ListPaginator design. The use of subList is quite strange imho. Sublist provides a mutable view, which is great if you want to mutate pages, but yet there is no way to add extra pages to the ListPaginator easily. I feel like if you were to allow mutating of the pages themselves wouldn't it make sense to also allow the addition and removal of pages as a whole?

[spring reef]

good catch, that wasnt intended. yeah i agree its a bit odd in this context. i will make it immutable, thank you 👍

[woeful pasture]

Wrote this in class today fire away ladies and gents https://github.com/Y2Kwastaken/AdvancedGreetingService

[random yacht]

wtf am I reading

[alpine anvil]

Nit pick it. Go enterprise on him

[sharp epoch]

I just stopped reading since I can't review something I can't take it seriously lol

[spring reef]

so i wasnt the only one being confused

[woeful pasture]

Not confusing at all its just a multi lingual hello world program

[spring reef]

i expected something else, so i was confused

first thought its a greeting service for devices

[woeful pasture]

I should add android support that is a good point

[spring reef]

not what i was trying to say, but go for it

[novel meteor]

After some time of just copying them every time, I put some classes together in a small library
https://github.com/steve6472/SteveCore
Mostly interested in review of settings and registry
Very basic implementation is in https://github.com/steve6472/Orbiter
Rarity probably shows the registries best I think.

[cobalt trellis]

You're missing a license file and something else is amiss when looking at the root directory structure

https://github.com/steve6472/SteveCore/blob/8589edb4f84fadebf86edd40ef7abc640fdaa0da/src/main/java/steve6472/core/util/Preconditions.java#L14-L26 let's not ignore that the Objects class has similar methods for null checks.
https://github.com/steve6472/SteveCore/blob/8589edb4f84fadebf86edd40ef7abc640fdaa0da/src/main/java/steve6472/core/util/Preconditions.java#L60-L70 checkAboveZero is a misleading name when 0 is also in the supported range of value.
https://github.com/steve6472/SteveCore/blob/8589edb4f84fadebf86edd40ef7abc640fdaa0da/src/main/java/steve6472/core/util/Preconditions.java#L72-L106 at this point I'd call them assertX and throw an AssertionError, but I suppose it depends on the usecase.
https://github.com/steve6472/SteveCore/blob/8589edb4f84fadebf86edd40ef7abc640fdaa0da/src/main/java/steve6472/core/util/RandomUtil.java#L19-L38 calling it randomRadSin etc. is a bit misleading, since sin and cos return values from -1 to 1, making it an implementation detail whether the sine and cosine were obtained through a random radian or through a random degree.
https://github.com/steve6472/SteveCore/blob/8589edb4f84fadebf86edd40ef7abc640fdaa0da/src/main/java/steve6472/core/util/RandomUtil.java#L108 probably not the most … inline naming out there. randomBoolean might be a slightly more conventional name there.
https://github.com/steve6472/SteveCore/blob/8589edb4f84fadebf86edd40ef7abc640fdaa0da/src/main/java/steve6472/core/util/RandomUtil.java#L113 using an integer for probability can easily be a bit misleading. Unless you need the performance, the good old return (randomDouble() < probability); is more intuitive in what it means. If you want to go the integer route, compare with 0 instead since that results in fewer Opcodes, meaning it's more likely to be inlined

[novel meteor]

Preconditions are indeed shitty and unfinished, not sure if I even want this.
RandomUtil is very old. All your comments make sense, I'll implement those, thank you.

[round fossil]

I mean in Preconditions, you may wanna throw more specific exceptions than just runtime exception

can be useful if u unit test around a special type of failure for example

[cobalt trellis]

junit's assertions are usually good enough in tests and IDEs have auto-completion built-in for them and even do static imports (which is like the only time eclipse does static imports by default, but it's a coding style I don't really oppose to)

[cobalt trellis]

Rate my JLS-compliant code: List<String> = new <Integer, Long, Boolean> ArrayList(); (credits to the VF discord who figured out that this is valid Java code)

[covert marsh]

Yeah, could you please not?

Also im totally gonna look into this when im home... What a mess

[sleek shard]

for the randomRadSin and other functions that use sin and cos, couldn’t they just return a random number between -1f and 1f

lol

[cobalt trellis]

Then you might need to check the graph of sin and cos again

they aren't linear distributions

[sleek shard]

Well what I’m saying is that, because it returns a double, and the double is the sin/cos of a random number between -PI and PI, wouldn’t that basically just be a random number between -1 and 1? Or am I wrong in thinking that?

[cobalt trellis]

it would be a random number between -1 and 1. But the frequency of those values will not be uniform

[sleek shard]

Ah I see what you mean

[cobalt trellis]

This is mainly caused by sin'(x) not being 0 for every value of x

[late smelt]

What

[cobalt trellis]

It's thanks to UnqualifiedClassInstanceCreationExpression defined by JLS 22, §15.9. I'm not entirely sure what the TypeArguments are there for though

[late smelt]

Unqualified who what where

[cobalt trellis]

Class instance creation expressions have two forms:

Unqualified class instance creation expressions begin with the keyword new.

An unqualified class instance creation expression may be used to create an instance of a class, regardless of whether the class is a top level (§7.6), member (§8.5, §9.5), local (§14.3), or anonymous class (§15.9.5).

Qualified class instance creation expressions begin with a Primary expression or an ExpressionName.

A qualified class instance creation expression enables the creation of instances of inner member classes and their anonymous subclasses.

[tall trout]

import me.jonnite.wardensmp.WardenSMP;
import org.bukkit.Bukkit;
import org.bukkit.potion.PotionEffect;
import org.bukkit.potion.PotionEffectType;
import org.bukkit.scheduler.BukkitRunnable;
import java.util.List;

public class passiveTask extends BukkitRunnable {

    private final WardenSMP wardensAPI;

    public passiveTask() {
        this.wardensAPI = WardenSMP.getInstance();
    }

    @Override
    public void run() {
        var onlinePlayers = Bukkit.getServer().getOnlinePlayers();
        onlinePlayers.forEach(player -> {
            var playerData = wardensAPI.getPlayerData(player);
            List<String> passives = playerData.getPassives();
            if (passives.isEmpty()) return;
            passives.forEach(passive -> {
                switch (passive) {
                    case "speed1":
                        player.addPotionEffect(new PotionEffect(PotionEffectType.SPEED, 30, 0, true, false));
                        break;
                    default:
                        break;
                }
            });
        });
    }
}```

is this the best performance wise way to do this? btw there will be more than just speed1 passive?

[rancid fern]

Using a regular for loop is better

also just use contains and passives could be changed to a Set

[random yacht]

I mean I don't know about "better", but yeah, generally best to reserve calls to forEach() with either single-line lambdas or method references. If you're making a block out of your forEach(), you might as well just use a for loop

It literally makes no difference at all, but stylistically it's just clearer

I would also opt for just a single if statement over the switch, but I guess that's preference too

Ultimately I would have written your class like this,

public final class passiveTask implements Runnable {

    private final WardenSMP wardensAPI;

    public passiveTask(WardenSMP wardensAPI) { // Pass it directly
        this.wardensAPI = wardensAPI;
    }

    @Override
    public void run() {
        for (Player player : Bukkit.getServer().getOnlinePlayers()) {
            for (String passive : wardensAPI.getPlayerData(player).getPassives()) {
                if (passive.equals("speed1")) {
                    player.addPotionEffect(new PotionEffect(PotionEffectType.SPEED, 30, 0, true, false));
                } else if (passive.equals("whatever else you want to add")) {
                    // blah
                }
            }
        }
    }
}

But you should really look into an alternative for how you define your passives. Maybe you want to actually define a PassiveAbility or PassiveEffect class that would apply whatever needs applying to a player, then you wouldn't have to operate based on strings and add a new if condition for each new passive you define somewhere else

Your whole run() method could look like this:

@Override
public void run() {
    for (Player player : Bukkit.getServer().getOnlinePlayers()) {
        List<PassiveEffect> passives = wardensAPI.getPlayerData(player).getPassives();
        passives.forEach(effect -> effect.applyPassive(player));
    }
}

[jagged rock]

still no naming conventions

• ew if statements where an interface could be in place

[night knoll]

Quick question, I recently forked a project from GitHub and made a few changes to it, can this be reviewed as well?

The problem is, I don't really have a GitHub account and the plugin is based on an extremely old API

[sharp epoch]

I mean, just make a github account? Lol

or host it somewhere else if you don't feel like using github, there's plenty git hosting providers nowdays. Gitlab, bitbucket, gitea

[night knoll]

I'll give this a try, thank you!

[hybrid osprey]

I love gitlab

[sly jackal]

we use gitlab at work, its pretty nice

[dense leaf]

gitlab my beloved

[night knoll]

https://github.com/NazarbekAld/XeXeGui :)

[hybrid osprey]

val rizz = state(1000)

immediate F-

[night knoll]

1000 rizz isn't enough 🤔

[hybrid osprey]

I like your use of lambdas and anonymous classes

[night knoll]

Also code undocumented because of 3am coding

I'll figure all that out tmr 💀

[alpine anvil]

The module names aren't consistent and I don't get the point of tests not just being in core or the route module existing and not being in core

[night knoll]

Tests is a plugin so I can test the library manually. Route is not fully implemented yet I think it will be big.

[alpine anvil]

Use consistent naming, some is plural some isn't

[night knoll]

Sure

What about internal stuff?

[dense leaf]

object LinearCurve: (Float) -> Float

For the future, ALWAYS put a space after a class name, not LinearCurve: ... but LinearCurve : ...
See https://kotlinlang.org/docs/coding-conventions.html#colon

private fun minimessage(parse: String): Component {
    return MiniMessage.miniMessage().deserialize(parse)
}
private fun minimessage(parse: String, vararg tagResolver: TagResolver): Component {
    return MiniMessage.miniMessage().deserialize(parse, *tagResolver)
}

The first function is redundant, and I'd also make this public for use in other places, and have an extension fun on string for that

guiComposable.add( e )
val hooks = mutableListOf< ComponentState<*> >()
inline fun Gui.component( slot: Int, component: GuiComponent ) {
Please ffs don't do that spacing

I think you need to listen to more inventory events and you can just drag items out currently, don't know if that's intended or not

I don't get why you'd have a seperate module for a singular class

RouteCommandTest is a very inconsistent name considering all others are just ThingCommand

Other than that, it looks pretty cool

[sharp epoch]

is the first point something normal to kotlin devs

as in, is it defined in some code style guide

I'm a dumbass, you linked to it lol

ah, it makes sense now. I thought it'd be fine since it would look more consistent with how methods use the colon, however it'd definitely look weird with parametized classes

[jagged rock]

Use ktlint or something and stick to a standard

Your miniMessage methods could become extension methods

Make sure to stick those in dedicated extension classes

[desert ore]

https://github.com/RaydanOMGr/BasicModInfoParser rate my ass code

[dense leaf]

Why not make BasicModInfo a record?

[desert ore]

Java 8 compat 🙃

[dense leaf]

[desert ore]

I'd write it in java 5 but I fear that has no lambdas

[late smelt]

Can we ban raydan

[dense leaf]

yes please

https://tenor.com/view/bane-no-banned-and-you-are-explode-gif-16047504

[sharp epoch]

why

[desert ore]

simply because I can

[sharp epoch]

sigh

[desert ore]

and if I can, why shouldn't I

[alpine anvil]

[desert ore]

I think you wanted to say "forwards compatible"

[sharp epoch]

it is dumb compatible

modern software should be written using modern technology if there's the choice

[desert ore]

this is modern software written for ancient software

[dense leaf]

paper!?

[sharp epoch]

writing code that unnecessarily delays the inevitable deprecation of older technology is a sacrilege as a developer

[sharp epoch]

there's nothing in that code that requires it to be written in java 8

anyhow, as per the review goes

it looks fine, kind of annoying that it uses older IO APIs but that's fine

would rather see take advantage of ZipFileSystem instead of JarFile, and maybe a DirectoryStream or a Files#walk for the archive lookup

the code for FORGE_LEGACY and FABRIC is pretty similar, you could probably take better advantage of Gson by creating records/classes to parse the metadata, unifying the logic

it's pretty well-written as far I can see, just a shame that it doesn't really take more advantage out of the newer I/O APIs out there in the JDK and in the language as a whole

considering the amount of resources for the old IO is insurmountably larger due to it being the peak era for java, I guess it can't be helped

[alpine anvil]

nuh uh

[cobalt trellis]

The main issue is finding a J5 JDK I believe

And then the second issue is finding an IDE that even allows you to compile against J5. Eclipse (and thus every other system based on JDT) recently removed support of Java 7 and below, and I'm not sure how IJ is doing.

And honestly, going with anon classes isn't terrible for enum members. It's only more of an issue when you expect to have public API be based on it, but that's not really the case

[hasty lotus]

Let’s be FR who even be using Java 8 these days

Maybe boomer devs

[junior holly]

I use java 8 to show people why j21 is better :p

[cobalt trellis]

me using J6 for micromixin

[hasty lotus]

No no it’s the opposite, you make them jealous with all the j21 features

[cobalt trellis]

Jealous in respect to what?

[hasty lotus]

Java 8

[cobalt trellis]

The only real J9+ feature I'm missing and that one cannot easily port over to J8 world is named classloaders, but Multi-Release Jars are a thing so that's something limited to the production environment

[random yacht]

instanceof casting would like to have a word

[cobalt trellis]

Well, that you can downgrade to older java versions rather easily thanks to compile-time tools

[junior holly]

What about pattern variables

[cobalt trellis]

pattern vars? Probably never used them so there I actually don't find any reason for using them going forwards. And even then, that's syntax sugar which can be downgraded extremely easily

[junior holly]

Still have to put the effort into compatibility, I'd rather just have it available

[cobalt trellis]

I mean, it's pretty much only copy & paste once you have written the gradle buildscript once

[wintry fern]

https://www.oracle.com/java/technologies/java-archive-javase5-downloads.html

[cobalt trellis]

I don't think I really want to create an Oracle account, notwithhelding any other complications associated with it.

[woeful pasture]

I'll throw this one out here I just made it
https://github.com/PineappleDevelopmentGroup/PineappleEnvoys/

[jagged rock]

no entry loop?

Why is your list named as if it wasn't a list

This can be tossed up to save yourself performance

Pretty sure there's a take builtin method

Not sure why you're returning a "defensive copy". This list isn't mutable

You could use some sort of FSM / Phase system for the envoys themselves (wait phase, drop phase, collect phase)

Lots of hopes and prayers here

9/10 chance you have a memory leak if the world decides to unload

I'd make a dedicated ChunkKey class. Could just be a typealias, but using generic pairs is quite stupid

It might also be worth to have some central "chunk snapshot cache" if multiple envoys decide to fire around the same time

Also hardcoded messages ew

@woeful pasture

[woeful pasture]

debug messages stay hardcoded its just there for if shit explodes

I made everything that is actually generally supposed to be used configurable

[jagged rock]

are you sure about that

[woeful pasture]

what class is that in

[jagged rock]

command

one of them

[woeful pasture]

which one

[jagged rock]

debug

[woeful pasture]

because I have DebugCommands

which has everything hard coded

[jagged rock]

loot table

[woeful pasture]

yeah I should switch that over to a String with a getter to the actual world

[jagged rock]

or.. some world reference class

[woeful pasture]

yeah gp there

[woeful pasture]

yeah for sure the actual "game" portion could use some work tbh

[woeful pasture]

where is that at?

is that my EnvoyTile?

[jagged rock]

probably

[woeful pasture]

if so it can kind of just explode If the input never lines up it should explode and it'll be deleted pretty much

[jagged rock]

Also why are half the variables nullable

Wouldn't it be better to migrate to some sort of builder -> constructed object pattern

[woeful pasture]

its kind of an unfortaunte symptom of the top level library load and save design. There isn't a much better way to design a super generic load save system that could work for every conceivable thing without reflection thoguh

[safe silo]

Here we go I guess
https://github.com/notFoxils/foxutils/tree/master/foxutils-items (utility for adding custom items, you can poke at the other stuff, but its all broken i think lol)
https://github.com/notFoxils/SynthSMP/ (implementation)

I'm quite new to Java, OOP all of that

and I have a feeling I may or may not be making some mistakes here and there, so id like to hear some feedback or just get some pointers in the right direction (which i guess is the whole point of the channel)

[sharp epoch]

looking at foxutils-items right now, and the main class looks good so far. Just one mistake: you're using Objects#nonNull wrong

I am aware that IJ suggests that for hiding the warning, however it isn't a good solution as it is an assertion and not an assurance

[safe silo]

i believe i just put those there to make intellij stop complaining lol

[safe silo]

So, like, we are telling the editor (compiler maybe idk) that ooh this is never gonna be null, but we never check it? That seems about right

[sharp epoch]

yep, so it is just gonna throw instead. Though that might be fine in this instance since getCommand only ever returns null when you forget to put it in the plugin.yml, or forget the plugin.yml altogether lol

[safe silo]

a question relating to this, im i think my doubt in the structure of the modules i have there is from a lack of understanding of OOP/OOD

im kinda just throwing shit at a wall and seeing what sticks atm (foxutils-hud module) and yknow i end up wasting a lot of time that way (foxutils-hud again), just to create something im not really 100% confident in, or just something that dosent work (foxutils-hud, AGAIN)

[sharp epoch]

what is this supposed to be, an API for hud configurations?

oh you got your own config API too, fancy

[safe silo]

it was a bit ambitious, but the main goal was to get something working where i could specify a "hud" configuration inside a dedicated configuration file EX:

hud-configs:
  five-hud:
    x-offset: 0
    texture-unicode-character: "\uE000"
    # Other custom data can be stored here for usage later for implementing plugins
    alternative-texture-unicode-characters: {
                                              "bleh"
    }
  player-hud:
    x-offset: 0
    texture-unicode-character: ""

[spring reef]

first thing i instantly noticed: your registerItems method is awful

[safe silo]

and then get the api to consitiently send action bar messages with custom unicode characters to the player

[spring reef]

rather put it into another class or hold constants, f.e. your ItemAbilities.

[sharp epoch]

Eh, it is fine

[safe silo]

while having those same hud able to be extended like a class, or atleast manipulated through a class, and get info from a custom config class

[sharp epoch]

it isn't big enough that it couldn't just be in the main class, they can move it later when it becomes too much

[spring reef]

its already too much

each of them could be a constant, all the hardcoded strings are a bad practice

rather save it somewhere

have a repository for them

the names could be an enum as well

[sharp epoch]

it's 135 lines, that's not much

[safe silo]

that just flew over my head, could you further explain that?

[spring reef]

you have a huge method to instantiate the itemabilities f.e.. you could put that into f.e. a ItemAbilityHandler, -Factory, -Repository, -Service(if you load it from a db or smth f.e.)

and handle the logic there

you can modulize it into smaller pieces as well

[sharp epoch]

ohh, that's kind of like this plugin: https://www.spigotmc.org/resources/gui-api.95468/

[spring reef]

f.e. the name as an enum and map the ability to it

[sharp epoch]

unfortunately I am not too informed on the whole deal with negative fonts and how to make custom HUDs with them, however you may take inspiration from something similar

that said, the whole thing looks pretty solid, you'll probably find the rough edges the more you use it

[sharp epoch]

is that necessary when there's like, 10 custom items max

[spring reef]

nothing is necessary, but QOL and best practice

[sharp epoch]

at the point it starts to scale, they'll probably make some kind of configuration-based approach like they did with huds anyway

[spring reef]

bet he already has to search for single abilities if he wants to modify them

by making it modular and having a handler and factory for it he probably dont have to

[safe silo]

i mean the implementation is there

you can check how i did that

[spring reef]

most what sucks in it is the description, but thats not on him but just how it has to be done sadly. or you save it in a resource properties and modify it there.

[sharp epoch]

ideally it'd be just a whole of translatable components, but we can't do that in spigot, can we sigh

[spring reef]

yeah i see, thats well structured. i would refact it still tho by extracting methods but it looks good

its just the instantiation that caught my eye

having a properties with the description strings which you can just dynamically load in would be top notch tho

by that you get away from hardcoding string literals

[safe silo]

not to sound like an idiot, but instantiation being when i do

new WhateverItem(bla, bla, bla)

and you are talking about how the constructor ends so huge (which i was talking about with another developer friend moving most of the bulk to a config file)

[spring reef]

exactly, but im referring to the description strings to be exact

[sharp epoch]

they are suggesting you consider i18n-ize your item lore/names

[spring reef]

^

[safe silo]

the whatanize 😭

[spring reef]

simply said put your ability descriptors into a file

[safe silo]

okay so mainly the ItemAbility class and how those can turn out quite huge

[safe silo]

okay, makes sense

[sharp epoch]

i18n is a short for internationalization (there's 18 letters between the i and n in internationalization, so i18n), it means having your strings be configurable/translatable via a properties file of your choice

[safe silo]

okay yeah i see what you mean now

[spring reef]

yes, by that you have everything at one spot and dont have hardcoded descriptions

[safe silo]

yep yep, so move that to a config, and along with that i could probably move some of the other properties like material, names, custom model data, recipes probably

[spring reef]

leave it to the strings first

and lookup Properties

java has an integration for that

everything else is serialization

[safe silo]

this? https://www.baeldung.com/java-properties

[spring reef]

https://github.com/notFoxils/SynthSMP/blob/master/src/main/java/me/foxils/synthsmp/databases/Database.java if you want to have something more robust you could think of switching to a database framework

[spring reef]

yup

[safe silo]

thats why i had the foxutils-data module, but i got stuck on constructing a solid structure for that

[spring reef]

f.e. using hibernate: https://github.com/Metaphoriker/antiac/tree/master/src/main/java/de/godcipher/antiac/hibernate

[safe silo]

.

[spring reef]

but most would argue that hibernate is too bloated for a minecraft plugin

[safe silo]

isnt it quite similar to what you can do with FileConfigurations in bukkit?

[spring reef]

yesnt. one is properties, one is yml

its not a config to be clear

[safe silo]

wait

whats the difference

i know yml files are supposed to have a specific structure to them

but in the end you can do i/o on both of them?

[spring reef]

properties = the properties of your application
config = users can configurate your application

im really bad at explaining lol

[safe silo]

so properties are never seen by the end user? its just so that i have an easier way to edit the info of the ItemAbilities?

[spring reef]

exactly, the properties are just inside the jar so people would have to decompile it to modify your properties

[safe silo]

okay

i see now

[spring reef]

https://github.com/Metaphoriker/antiac/blob/master/src/main/resources/messages.properties i have that as well here

https://github.com/Metaphoriker/antiac/blob/master/src/main/java/de/godcipher/antiac/messages/Messages.java used like this

so i can just call Messages.getString("the.message.i.want.to.get");

[safe silo]

okay

[spring reef]

by that your code also doesnt changes when you have to modify your descriptors f.e.

[safe silo]

okay so in the same way i could hold the descriptions, would it be preferable to hold other constants like the Material, Custom Model Data, etc inside another properties file?

[spring reef]

i wouldnt see why, since they are already constants

but you could wrap the custom model id and turn it into an enum or whatever so you also have them all at one point centralized

[safe silo]

what would be the point in that?

[spring reef]

that you dont have magic numbers flying over your code

• turning it into a value object and having a constant for it increases the readability since you know directly what it is

[safe silo]

i know what they are, but i only ever specify the custom model data in the constructor, i never check for it

[spring reef]

f.e.

CustomModel.MY_MAGIC_SWORD is more readable than 1027

[safe silo]

i guess thats true

[spring reef]

or if you are working with value objects

new CustomModelId(1027); is more descriptive than 1027

• changes to the code of CustomModelId doesnt directly affect the code where it is used

and ofc an underrated point, you can just ctrl click on CustomModelId to see where you have custom model ids put

with only integers in your code you cant

[safe silo]

alright, on all that, do you have any resources where i could learn how to properly plan OOP projects?

not the programming part, but just learning how everything should fit together i guess would be the right phrase?
So instead of just throwing a whole day into something that is only like half functional, i could plan a proper structure for my projects and then build based off of that

[spring reef]

resources not really, just a mindset

always ask yourself what could be better

[sharp epoch]

I mean, from the looks of it you're doing OOP properly

[spring reef]

first put your stuff together. do what you need. then think of optimizing

[sharp epoch]

what you seem to be concerned about is more on the API design side, and that mainly comes from experience to be honest

I believe your code is good since it doesn't try to be clever, so intent is pretty clear from a first glance

[spring reef]

yeah. there is no proper resource on how to do stuff the right way. everything is more like an opinion people share

but what you could look up is Clean Code, Design Patterns and Best Practices for java

that will help you build a fundament of a good structured code

everything else is software architecture

[safe silo]

alright

[spring reef]

and SOLID is worth a look at

https://en.wikipedia.org/wiki/SOLID

[safe silo]

thats the main part im really trying to get at, but like flores said, the design side probably just comes from experience

[round fossil]

i mean for most part, if u've never coded the infrastructure of some feature, its really hard to know what the "right structure" in presupposition

so yea experience does help quite a lot with familiarity and confidence in correctness

[spring reef]

https://medium.com/@peterlee2068/software-design-principles-every-programmer-should-know-c164a83c6f87 thats a good kickstart

to gain practical experience you have to understand the theory behind it

those principles make it easier and give you a direction

or if you want to deep dive https://en.wikipedia.org/wiki/List_of_software_development_philosophies

[safe silo]

alright, sounds good then, ill read up on all the resources you guys mentioned

[woeful pasture]

Clean code is crazy just learn your data structures and common design patterns

You'll develop your own style from there, but learning common design patterns and data structures is important

[random yacht]

Also be willing to adapt to feedback and your environment. My code style differs depending on which code base I'm in

The biggest keys to a good codebase are (a) malleability and willingness to learn, and (b) consistency

[lyric stump]

https://github.com/InfLabss/VoucherZ/

[woeful pasture]

Use PDC
https://github.com/InfLabss/VoucherZ/blob/main/src/main/java/dev/infnoxx/voucherZ/manager/VoucherManager.java#L123-L129

[lyric stump]

Noted

Anything else?

[night knoll]

?paset

?paste

[rich fog]

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

[night knoll]

weird ass strings are for debugging

i just want someone to rate my code

[dense leaf]

Please don't just throw around Objects.requireNonNull it will just throw if it's null

Use actual error handling instead

[night knoll]

Okay, thank you

[dense leaf]

In your send method why not just use the scheduler instead of doing anonymous classes?

[night knoll]

sorry, what do you mean?

[dense leaf]

Bukkit.getScheduler().runTaskLater(plugin, 0, 3, () -> {...})

I believe those are the right arguments

Oh you used runTaskTimer

well doesn't change much

[random yacht]

.runTaskLater(plugin, () -> {...}, 0, 3)

So close

[late smelt]

plugin, runnable, delay, interval

[night knoll]

Oh okay

[late smelt]

Or just plugin runnable delay for runTaskLater

[dense leaf]

what a nerd

[random yacht]

You use them for 10 years, you start to remember them after a while

[dense leaf]

And why call Main.getInstance() every time when you have a plugin field already?

also

?main

[rich fog]

https://www.spigotmc.org/threads/why-you-shouldnt-name-your-main-plugin-class-main.493289/

[night knoll]

Damn u right

[dense leaf]

new CreateScoreboard()
that's an odd class name

And please don't just scatter fields around your classes, it makes it hard to find them

[night knoll]

Okay okay

[woeful pasture]

Did they delete the link To see the code?

[dense leaf]

Yup lmfao

[woeful pasture]

No one is stealing your code lmfao no point in deleting it

[dense leaf]

You also don't need to be ashamed of your code

if that's your reason

[late smelt]

In the industry bad code is a requirement

Because we have to get this shipped yesterday

[dense leaf]

Deadline: 37 days ago