[MEGA THREAD] Get Your Code Reviewed or Review Others

[digital galleon]

Don’t ping me for your shit

[jagged rock]

how cute

[digital galleon]

ily @dense leaf no hard feelings

[dense leaf]

Don't ping me for your shit

:)

[jagged rock]

you guys are like an old couple bickering

[digital galleon]

we’re fucking done

[late smelt]

Rad got married?

And now divorced

[woeful pasture]

Spent the last day or two working on this one. Right now I just have the basic AST, tokenizer parsing done. I need to make a module regarding actually turning this into anything useful yet, but yeah. An example of the file format is in the test resources folder. I'm going to use it in another project for code generation so I'm curious if anyone has comments on the parsing

https://github.com/Y2Kwastaken/Artisan/tree/master/artisan-format

[hexed agate]

@woeful pasture what is it

[woeful pasture]

Basic code generator format. E.g. with the ATs I define that I'm writing ATs and it'll parse it. Then one I finish coding this part it'll apply the AT to a jar file

[hexed agate]

But what’s the end goal here

[round fossil]

if I understood correctly? this does both lexing and partially parsing also?

[woeful pasture]

Basically ASM transformations at the end of the day. Whether it be basic like ATs or slightly more complex like method generation

[woeful pasture]

The goal of the format module is to parse into a syntax tree that can then be handed off to my other module which will be where I parse then do transformations from that tree

[round fossil]

yes but fyi tokenizer parsing is usually what we call lexing, or yk a lexer does it, but yea I did understand the goal ^^

[woeful pasture]

Ahh okay I've never really done this stuff before so idk terms really

I'm just kinda going ground up haven't particularly learned how to make a proper parser or anything

Most of my knowledge is what I learned from making a mini message clone which ended up working fairly well, but I decided to stretch these muscles again instead of just depending on a library

[round fossil]

it looks decent, i havent rly done any meaningful lexing, or parsing on my own but i think programming language theory might be something of ur interest if u do look to deepen ur knowledge in this :>

edit: usually lexing does more than tokenizing, that is fill in context and info around the tokens

[woeful pasture]

I really want to take language design class at school next semester hopefully it's actually meaningful

[alpine igloo]

Anyone got any advice to create licence system ? I mean whats the best obfuscator to protect own code?

[dense leaf]

don't obfuscate

[round fossil]

this is not code review, you further may ask of assistance regarding your issue in other suitable channels

[dense leaf]

when are we gonna review obf code

[alpine igloo]

oh didnt notice thats thread :# my bad

[charred forge]

I remember pre decent modding APIs I use to have to read the obf source to update mods I made 😭 back in Risugami's ModLoader days

[dense leaf]

@desert ore risugami's mentioned

[desert ore]

Best modloader fr fr

Got jd for it

[charred forge]

xD

[desert ore]

The jd looks like something from 2011 (which it probably is)

[charred forge]

the simple days of Risugami's and hMod

[desert ore]

Both of them make me feel sick nowadays

[dense leaf]

https://github.com/radstevee/packed (example) would like some opinions other than the fact that it's kotlin

[sly jackal]

its kotlin

[pine grail]

The fact that you store the credentials in a class like that feels insecure, but otherwise I don't see any issues tbf

other than maybe being able to store bitmaps instead of rewriting them each time

[dense leaf]

that's kinda just the example yk

[pine grail]

Fair enough, but it would be good to show that you can. I assume that you can just store the result of bitmap in a variable to then reuse it?

[dense leaf]

i mean yeah, that builds a bitmap, adds it to the font and returns it

[pine grail]

can you use it outside of addFont

[dense leaf]

I mean yeah

https://github.com/radstevee/packed/blob/v1/packed-core%2Fsrc%2Fmain%2Fkotlin%2Fnet%2Fradstevee%2Fpacked%2Fcore%2Ffont%2FFont.kt#L144-L146

[pine grail]

so like you can take the bitmap out of these and just reference it like

val myBitmap = bitmap {
    key = Key("packed", "font/bitmap.png")
    height = 8.0
    ascent = 7.0
    chars = listOf("\uE001")
}

pack.addFont {
  key = ...
  myBitmap
}
```or something

[dense leaf]

Oh I don't have a seperate builder function ig

Could add one

but yeah, that'd just be addProvider(myBitmap)

[pine grail]

ah, yeah I meant it mainly in the case that you have two equal bitmaps

instead of rewriting by hand you either make it a function or store it, tbf storing it seems better

[dense leaf]

Yeah it's definitely possible

[pine grail]

That's my only nitpick with it otherwise from a usability perspective it's fine

[dense leaf]

just create an instance of FontProvider.Bitmap and you're good to go

I should add builder functions outside of the font tho, you're right

[pine grail]

pack.addBasicSound(Key("packed", "my_sound"))

what does this sound reference?

like what would it play

[dense leaf]

the key there

[pine grail]

where would you add the sound file ig

[dense leaf]

I believe I did state that in the docs of that function

assets/packed/sounds/my_sound.ogg

[pine grail]

Looking at it on GH because I'm not on linux and don't have any IDE here lol

[pine grail]

ah so you just put it in your project folder, I see

[dense leaf]

yeah I wasn't requiring you to clone this lol

[pine grail]

yeah meant more so to quickly look at docs, but yeah I understand

[dense leaf]

Well it depends on your asset resolution strategy, you can put it in some directory, a git repo or in resources

[pine grail]

yeah just saw it at the top

btw what are ItemDefinitions? Do they just allow you to add some model data or whatever to an item to make it look like a texture?

[dense leaf]

item model definitons, from 1.21.4 (I believe?)

[pine grail]

ah fair enough

what about pre 1.21 though? Do you just add a custom model data with a tex or do you not support it lol

[dense leaf]

Just custom model data yeah

[pine grail]

Don't mind if I "inspire" (steal) a couple ideas 💀

Mainly some item texture stuff, I like the key based approach

[dense leaf]

that's just how resource works yk

resource packs* god I'm dumb

[pine grail]

it's fine I still call them texture packs 😭

[dense leaf]

no I just forgot a word

[pine grail]

lol

[dense leaf]

see https://github.com/radstevee/packed/blob/450d66cb00363b70d17dbf97169521993435cfca/example/src/main/kotlin/net/radstevee/packed/example/Main.kt

horrendous outdated mess but yea

[dense leaf]

This is possible now in 1.0.0

[noble eagle]

https://github.com/Pixel-Services/PixelToolkit/tree/main/src/main/java/com/pixelservices/logger anyone wanna take a look at my logger, its not much yet but idk what to add next

[pine grail]

Can you add an example?

I'm on phone rn

[noble eagle]

its on the readme

[pine grail]

I see no readme

[pine grail]

I am blind ash my bad

[noble eagle]

all g

[desert ore]

Does it have a slf4j impl

[dense leaf]

it's from scratch

[desert ore]

I'd use it if it had an slf4j impl

[noble eagle]

then entire reason i made this was to no longer rely on an external logger

[desert ore]

slf4j is a facade, not a logger

[noble eagle]

well

still a depedecy

which i didnt want

xd

[desert ore]

not necessarily

an slf4j impl can be seperate of the logger itself

[noble eagle]

and your house is flammable so watch out

[desert ore]

[dense leaf]

why do I have this? no idea

[woeful pasture]

quick make this a gif

[noble eagle]

make a mobot module that turns images into gifs

oh wait

mobot is broken

😔

[woeful pasture]

I'm sure I could make something with code

but like that's a lot of research on image generation I can't be bothered to do

I just used a website

[woeful pasture]

https://cdn.discordapp.com/attachments/1309927963541770294/1332730889481752726/ezgif-2-f225fcbda0.gif

[noble eagle]

theres probably countless libraries for that

[woeful pasture]

no fun unless you rolll your own

[dense leaf]

ImageMagick

[noble eagle]

theres like 5 people in my dms asking me to fix mobot

but 10 voices in my head telling me to just ignore them

idk who to listen to honestly

[woeful pasture]

wtf is mobot

is that your discord bot

you should fix it if you maintain that

[noble eagle]

well

yesnt

its a modular discord bot but i dont make any modules

i just made the framework

:p

[woeful pasture]

oh then its not on you to fix the modules

lol

especially considering you don't even run the bot

[noble eagle]

well the module loader is broken

using dependencies doesnt really work

and ive been working on a new module loader similar to the one spigot used for a while now

but it’s complicated

[woeful pasture]

erm I wouldn't necessarily use spigot as an example for a dep loader kekw there are deff some things that could be done better there especially when dependencies come into play

if you're not dealing with dependencies its pretty straight forward honestly

[noble eagle]

well i am xd

im trying to implement https://pf4j.org/

but since i have custom configs

and 4 lifecycle methods

i need to rewrite alot

kinda wanna make my own plugin system rn honestly

[serene scroll]

made some terrible sql interface, are there any glaring design issues (aside from the wrong setup query statement)?
https://paste.md-5.net/gaxamasowa.cs

[sly jackal]

I have no idea what you mean by those partialString functions

[serene scroll]

they're chaining together the primary keys and other similar parts for the statement
its mostly to keep the functions providing the statement strings readable, and i cache it to only call them once

[noble eagle]

i mean like..

why?

[serene scroll]

so that i can do this in any future projects
connector.requestTable(KeyType.NAME, ValueType.STRING).setKey("test").setValue("info").executeUpdate();

with slightly more appropriate values

this is from my tests

[distant gorge]

A utility class I made, not much here tbh.
https://paste.md-5.net/yaxemijuze.php

[dense leaf]

legacy chat
paper

also Player#getPing?

[round fossil]

from what I know

utility classes can be completely avoided in kotlin

as the language offers other, better means of dealing with such functions

@dense leaf opinion?

idk if im outdatd abt this or nah

[dense leaf]

Correct

I mean, there's nothing wrong with utility objects, it's structured a bit more nicely, but

there are better ways such as top-level functions and extension functions

[tidal basin]

how can I use it please ?

[woeful pasture]

Copy the java files is the most dependable way here

I'm not going to reccomend you to depend on me

Due note my lib is licensed under Apache, but I don't really have plans to enforce that so do as you please

[tidal basin]

That's what I've done

I was saying, like how I can have a RegistryKey with type String and a value with another type ?

[woeful pasture]

Registry key is something your value class implements

[tidal basin]

oooh

okay thanks

[woeful pasture]

Dog implements RegistryKey<String> as an example

Than register(new Dog())

[tidal basin]

perfect thanks 😄

[round fossil]

its em top level higher ordered infix inline functions init

[dense leaf]

?

[round fossil]

:)

[dense leaf]

are you trying to correct me or..? I really don't understand your message lol

[round fossil]

Oh noo

I was just spitting nonsense

as a functional programmer i suppose

[dense leaf]

average cowowoncluwuwube moment

[rancid fern]

https://media.discordapp.net/attachments/997393966494392323/1197680540548075591/watermark.gif

[dense leaf]

so true

I think that gif is even one of mine

[rancid fern]

probably

can't remember who I took it from

[late smelt]

You can’t convince me this is a sentence and not just a jumble of tech words

[dense leaf]

Yea

[round fossil]

rad dont act up maaan :c

[dense leaf]

smh smh

[round fossil]

look mf = (. map) . (.) . filter how beautiful

[alpine anvil]

josh are you gonna ever unchristmas

[late smelt]

Did you remember to reseat the firewall and defragment the RAM cache

[dense leaf]

not beautiful

I'll show you some beautiful functional code I wrote later

[round fossil]

shuuuuuuuuut

[dense leaf]

val durationSeconds = instance
    .controller
    .schedule
    .collect()
    .filter { state -> state.stateKlass == PvPState::class }
    .mapNotNull(GameSchedule.ScheduledState<*>::duration)
    .sumOf { duration -> duration.inWholeSeconds }
    .minus(30)

beautiful

[round fossil]

not point free tho

:|

[flat tiger]

so uhh do i just post code here and people will be brutally honest about my practices?
here goes nothing

https://github.com/SiliQon-Development/CosmicCosmetics

[sleek shard]

rad is a nerd

[dense leaf]

Map<Player, ...>

dies inside

[alpine anvil]

i love memory leaks

i also love public fields and direct map access

[dense leaf]

so true

registries 🙏

[sleek shard]

I mean instinctively, a list of Player, or a Map of player isn't actually BAD if you REMOVE THEM, yk

[alpine anvil]

id probably change it to just menu-item, no need to put cosmicosmetics as thats already in the key

[sleek shard]

but yeah you SHOULD use UUID for best practice

[dense leaf]

and please use registries instead of enum spam

also don't see why your gui lib would be a utility

and why is half of your fields package-private

[alpine anvil]

main stuff i can see is:

provide getters for stuff and non-direct map access
use the primtive booleans where you can
use uuid over player, even though you remove it from the map its still safer to use it

oh and set ur api version correctly

[dense leaf]

otherPlayer.spawnParticle(particle, player.getLocation().add(0,1,0), density * 5, 0.3, 0.5, 0.3, 0.05, new Particle.DustOptions(Color.fromRGB((int) (Math.random() * 256),
        (int) (Math.random() * 256), (int) (Math.random() * 256)), 1f));

magic numbers final boss

if (pdata != null) for (Integer taskId : pdata.getTaskIds().values()) {
    plugin.getServer().getScheduler().cancelTask(taskId);
}

questionable formatting

public class effects {

https://github.com/SiliQon-Development/CosmicCosmetics/blob/de4199940a6960b95e229ac3a4f4871917ccd3eb/src/main/java/com/siliqon/cosmicCosmetics/utils/general/effects.java#L77-L89 registries, please

[flat tiger]

no matter what im coding in what language i always think "what will other people think" or "is this really the best way to do this" and now i have a thread full of people to tell me off about it 🙂
time to get better

[dense leaf]

that's how you learn

[flat tiger]

yessir

also what does epiceb mean by set my api version correctly

[dense leaf]

you're depending on the 1.21.4 api but have your api-version set to 1.19

[flat tiger]

i also noticed i did not follow the holy naming convention in some places because i forgor so ill be sure to fix those too

the reason i used Player and not UUID was cuz i was removing it and i didnt need to access OfflinePlayer anywhere or anything but ig ill switch over

[flat tiger]

so id have like

EffectNameRegistry
EffectDescriptionRegistry
EffectTypeRegistry
...

right?

[junior holly]

Simple but yes

You could go the true OOP route and have a base registry type which you extend as such: Registry -> FreezableRegistry extends Registry -> and so on

Or just Registry -> RegistryImpl implements Registry

[dense leaf]

kat stop infecting everybody with your registries

smh

[junior holly]

No

[alpine anvil]

i so dislike registry impl

[junior holly]

I mean

That’s your opinion ebic :p

Simplest way is like you said, but you’re just gonna end up with a lot of duplicated code

[flat tiger]

i think OOP route is the way to go

i love OOP

everyone loves OOP

OOP is love, OOP is life

[tidal basin]

Hello, I don't know why but when I try to get an item in the player inventory, it stops the execution of the remaining code :```java
public class GameListeners implements Listener {
@EventHandler
public void onPlayerJoin(PlayerJoinEvent event) {
Player player = event.getPlayer();
player.sendMessage("Joined !");

    PlayerInventory inventory = player.getInventory();
    inventory.clear();

    inventory.setItem(8, Stacks.SPELL);

    ItemStack item = inventory.getItem(0); // Block
    player.sendMessage(item.toString()); // No message

    for (int i = 9; i <= 35; i++) {
        player.sendMessage(String.valueOf(i));
        inventory.setItem(i, new EmerixeStainedGlassPane(DyeColor.WHITE));
    }
}

}

[dense leaf]

check the thread you are currently in

[jagged rock]

did you just extend itemstack

[desert ore]

Lmfao

[dense leaf]

omfg

[tidal basin]

👀

a common lib with other developers

[alpine anvil]

did they just extend itemstack

[tidal basin]

yeah

is it going to blow ?

[alpine anvil]

yeah

[tidal basin]

why ?

[alpine anvil]

because you shouldnt do it

[tidal basin]

okay

NOT MY FAULT

[jagged rock]

poor practice of abusing internals

IT IS YOUR FAULT FYM

[tidal basin]

I'm a good guy

[jagged rock]

doesn't mean your code is good

[tidal basin]

NOT MY LIBRARY

I should use it else the other devs will be like AAAAH DUPLICATED CODE AND THIS THING IS NOT HANDLED

the fact that we modify directly the core of spigot make things different ?

[tidal basin]

fixed, it was just in the console (I DON'T HAVE ACCESS TO IT PLEASE HELP ME IT'S DIFFICULT TO LIVE)

[woeful pasture]

If you have a fork why do you have a lib?

Just include fork api

[dense leaf]

lmao

Why do people call it "the core"

I've seen so many people say server core

It's just a server mod

[tidal basin]

I don't fucking know, everything is shaded, the plugins are 60MB

[dense leaf]

How the fuck do you get 60 megabytes

[tidal basin]

but it works

https://tenor.com/kth3PLxcMCg.gif

[dense leaf]

Are you shading fastutil, the spigot API AND minecraft???

[woeful pasture]

Probably DB drivers

If I had to guess

[tidal basin]

yeah, redis

[woeful pasture]

DB drivers can get fat

[dense leaf]

I shade pg, exposed and lots of other shit and only get to 25mb

[tidal basin]

see by yourself lol

[dense leaf]

javassist

[woeful pasture]

Why do you shade netty?

[woeful pasture]

The server already has netty

[dense leaf]

Yea I've had netty shaded by accident before too

[woeful pasture]

Holy fucking library dependence

[tidal basin]

I said it

they are like "well, it works and we don't care about it"

[woeful pasture]

Lombok shaded too?

Why

It's an annotation processor

You need to find a new team this is horrid

[dense leaf]

why the fuck are you shading slf4j and presumably log4j too

[woeful pasture]

Which is also provided 😭

[dense leaf]

and jetbrains annotations

also provided

[tidal basin]

it's just the owner who is like that

[woeful pasture]

Shading junit at runtime

[dense leaf]

[tidal basin]

it's spigot 1.8.8 also

[dense leaf]

I love testing at runtime

in production

[woeful pasture]

Oh my god

What the fuck

1.8.8 explains everything

[dense leaf]

tbh the only 1.8 codebase I've seen is @sleek shards

And he writes actual good code

[jagged rock]

cap

every other project I enter talks shit about purple

[dense leaf]

well for that project at least

idk about the others

[sleek shard]

:(

it's because I was shit at coding back den

I think that was back further than 3+ years ago now tbh

[dense leaf]

Would anybody like to see some of my 8 year old code

It's JS btw

[hybrid osprey]

no

[dense leaf]

good

[pine grail]

yes

[dense leaf]

https://pastes.dev/yXUa1UwgLl

[pine grail]

please no

[flat tiger]

wheyah

[dense leaf]

literally almost any class

[flat tiger]

i switched over to registries btw

and UUID

im slowly going through the list of stuff

[flat tiger]

but like-
isnt public da wae?

[dense leaf]

I'm not sure you understand what I mean

https://github.com/SiliQon-Development/CosmicCosmetics/blob/de4199940a6960b95e229ac3a4f4871917ccd3eb/src/main/java/com/siliqon/cosmicCosmetics/guis/HaloCosmetics.java#L26

Took me 2 seconds to find

[flat tiger]

ah

so im just blind

i spent a good few minutes trying to find a no modifier field

i would go to sleep and not code at 3am but i dont think thats allowed

i dont see too many of those tho, i could only spot them in the guis package and almost nowhere else

magic numbers final boss has been defeated 😎

[flat tiger]

https://github.com/SiliQon-Development/CosmicCosmetics
how about now

[dense leaf]

Yeah that implementation is very questionable

[flat tiger]

why so?

[dense leaf]

Okay lemme get started

1. Please use fucking generics
2. It has no need to be abstract
3. Don't populate it on init
4. Never expose mutable views

More from @junior holly and @alpine anvil

[junior holly]

It seems the registries are supposed to be static in the sense of you not registering/unregistering anything... population on instantiation, if this is the case then there's no reason to even give the option of register/unregister and at that point just use an enum kek

https://github.com/SiliQon-Development/CosmicCosmetics/blob/main/src/main/java/com/siliqon/cosmicCosmetics/registries/EffectParticleRegistry.java

Also this just doesn't make sense

If the point was to get EffectType.SPLASH, that's a static call anyway so why are you mapping it to itself

[flat tiger]

its mapping to Particle.SPLASH

[junior holly]

oh yeah my bad

Still though it doesn't make much sense

if (type == EffectType.SPLASH) doSomethingWith(Particle.SPLASH);

Switch for those kinda things would work too

[flat tiger]

well the thing is, it makes it easier because im dealing with effectType

the action isnt different for every effecttype

i just grab all the different values and put them into the same process

[dense leaf]

registries are also usually either a string or identifier as key

and not some other enum

[flat tiger]

well i was told to make a registry-

[junior holly]

And we are telling you how to make a better registry

[flat tiger]

yes

[junior holly]

I mean the way I see it, your registries are basically just enums

[flat tiger]

yeah i was confused why i had to make em

[flat tiger]

generics where
when should i populate it then
whats a mutable view again

[alpine anvil]

generics on the registry

[flat tiger]

instead of object

ye

[junior holly]

Registries are useful when you need to hold certain data in mem, for example: UUID -> PlayerData which allows you to grab whoever's data during runtime. That being said, the way you have it setup seems to never want to change what is held which again at that point should just be an enum

[flat tiger]

yeah thats why it was an enum to begin with

[junior holly]

Registries are also useful when you WANT things to change or added or removed

[flat tiger]

yeah i got that

i read an explanation on gfg :p

[junior holly]

IE: other plugin devs who want to make a custom effectMaterial or whatever

[flat tiger]

so wont just having the registry like how it is rn let them change it?

[junior holly]

Yes however, for your purposes they don't look like they even need to be registries

[flat tiger]

but if i want to let other devs make changes, and create an api later down the line

[junior holly]

yes

[flat tiger]

shouldnt i let them be

[junior holly]

Well it needs some work but yes

[flat tiger]

ok

[dense leaf]

For the love of god please use generics

[flat tiger]

so use generics in the registry
dont populate registry on init (make a method to populate?)
whats a mutable view again?

[junior holly]

You return the mutable view of the registry ie:

    return this.registry;
}

How it should be: 

getRegistry() {
  return Collections.unmodifiableWhatever(this.registry);
}

[alpine anvil]

i mean the registry can be returned just fine

[flat tiger]

well i blame GFG

[junior holly]

Yes but it depends if you want to return a mutable view or not

[flat tiger]

there was a whole article on registries

[junior holly]

Most cases you probably do not

[flat tiger]

and they had sample code too

so me being a human i thought they werent fuckin with me

[alpine anvil]

ideally you never want to give direct map acess, either provide a clone or add modification methods

[junior holly]

Well for example you return a mutable view of the registry which could let them replace the entirety of what's held potentially removing things that weren't supposed to be

[flat tiger]

ok i see

so ill do that
ill use generics in the registry

so for populating should i make a method in the registry to populate it?

and why shouldnt i populate on init?

[alpine anvil]

most of the time registries are data driven/backed

[junior holly]

imo it gives the impression that they're not to be modified in anyway

[dense leaf]

I wouldn't call that populating even

You're just setting the map

[flat tiger]

so should i do that in a seperate method in the registry

i think thats the correct way?

#setDefaults() or sum

[junior holly]

Assign the map -> map = new HashMap<> then populate

[flat tiger]

but the RegistryTemplate, which i am extending, already sets it to new HashMap<>()

[junior holly]

For convenience you could also do a #set(Map<something, something>) method

[alpine anvil]

i would go with an add over a set

[junior holly]

Well yeah

depends on the use

[flat tiger]

well its #register(key, value)

[junior holly]

I find it useful to have a #set in some registries

[flat tiger]

so what should i really do about the populating on init

[junior holly]

populate it like normal instead of assigning it

[flat tiger]

so painstakingly #register every value on init

[alpine anvil]

i mean all the effect particle registry is a random effecttype enum to particle, why not just accept a particle there

[junior holly]

You still type the same amount as the Map.ofEntries

[alpine anvil]

by that i mean just remove effect type, it doesnt do anything other than map it to a particle

[flat tiger]

refactoring and stuff on updates
also makes data saving simpler

remember how 1.20 uses VILLAGER_HAPPY and 1.21 uses HAPPY_VILLAGER
if someone updated from 1.20 to 1.21 it'd break shit real quick afaik (lang file and stuff too)

its just easier.

and if i wanna change the particle of a specific effect type

[alpine anvil]

so only support 1.21

[flat tiger]

i can add effect type names which arent bound to the Particle. names

a bunch of stuff really

which is why i chose enums over Particle.

[junior holly]

I mean it seems like this sorta thing would be just fine then

[alpine anvil]

for some reason you have the api dep of an obselete version that cant be built anymore too

[flat tiger]

i think i fixed my api version

only supporting 1.21 for now

[alpine anvil]

you cannot build 1.21

1.21 is obselete and replaced by 1.21.1

[flat tiger]

i didnt do 1.21.4 because i thought something could break if someone installed the plugin on a 1.21 server

or smth like that

[alpine anvil]

so only support latest

[flat tiger]

well i wish to support latest-ish

[alpine anvil]

if anything your api dep should be 1.21.1

[flat tiger]

im only supporting 1.21 rn because i built the plugin initially in 1.21 and dont feel like changing the particle names (yeah im lazy tell me about it)

ok ill change it to 1.21

[alpine anvil]

if anyone is still runing plain 1.21 they need to update as it contains bugs

[flat tiger]

ok yeh

changed that

so finally i should do the #register for all the values then yes

[dense leaf]

Commodore probably rewrites those

[flat tiger]

commodore who

[woeful pasture]

Hey I'm going to critique your code :D

• please follow java's coding conventions for packages you can see these here com.siliqon.cosmicCosmetics breaks coding conventions I suggest you give them a view.
• I noticed you have lots of public fields? Why? Public fields generally break OOP, the goal is to keep things final that can be final and also wrap around what operations you want to allow right now I could do this outside of oyur plugins main class with an instance of it

yourPlugin.config = null

Sure not a great idea, but also kind of silly to let it be easily mutable like that. it's far better to instead use setters and getters like so

private MainConfig config;

// other stuff here
// other stuff here
public MainConfig getConfig() {
  return this.config;
}

This pattern can be replicated for many of your fields.

• Second I'm just going to go out of my way here and say absolutely NO

    private static CosmicCosmetics INSTANCE ;{ INSTANCE = this; }

Those instance blocks would cause trouble if your plugin doesn't do things properly on startup instead you should delegate the INSTANCE = this to onEnable and onDisable like so

@Override
public void onEnable() {
  INSTANCE = this;
}

@OVerride
public void onDisable() {
  INSTANCE = null; // not needed but a /reload safety net
}

• Start imports are generally discouraged its far more explicit to import everything you want to use. There are some notable exceptions like JUnit, but you aren't using that testing framework here.
• Files.java breaks OOP principles pretty violently, these things don't need to be static at all you should handle their initialization in a new object or instead register them properly in your Main class through non static methods. If you some reason feel the NEED to do it this way use parameters and stop abusing static fields the amount of static abuse is crazy.

• Again in your Messaging class you break oop principles why do you use this weird inbetween class? You could easily expose a public static Logger logger() method in your main class, which removes most of the need for this class and would remove the need for the crazy amount of static passing I'm seeing.
• Misc you also have the private static final CosmicCosmetics plugin = CosmicCosmetics.getInstance(); field stop using it and pass parameters properly there are better ways to do this.
• Why is the return method of Misc#checkPlayerPermission a Boolean object. This shouldn't be nullable primitive null auto returns false just use a primitive here.
• Misc is a bad class name Utils makes more sense a lot of these classes could be combined and most of their methods removed.
• Your Storage class is again a complete mess of static passes that just don't need to be this could be an object and not just could it should be an object.
• Your Registries are not registries just unexposed maps. A registry has a couple characteristics
  Firstly, some nice wrapper you could use e.g. defining a public class Registry<T> class that can be extended.
  Second, you don't need to put getInstance in every registry class instead i'd reccomend a helper Registries or using your defined Registry<T> class to declare all of these registries and their defaults in one place.
  Now some other nitpicks regarding your registries, use generic the T will allow you to return T from the map using raw objects is straight up dangerous it breaks OOP it breaks type safety. Extremely dangerous. I suggest learning about java's generic system
• Why static in your listeners when you could easily pass in the Plugin object to the constructor? Seems silly to have your plugin be static here when it could easily be an instance field

There is many other things but I will just start with this far now

My overall assumption from reading this code is not being super familiar with OOP, which is okay, but you should really look into some of its important concepts. The overreliance on static ruins any portability your code could have for other projects. And there is genuinely good stuff here you could just move from project to project in a library, but your static plugin instances everywhere pretty much gets rid of any reusability you could have

[random yacht]

private static CosmicCosmetics INSTANCE ;{ INSTANCE = this; }
ngl, as much as I hate this, I have to applaud its creativity lol

[alpine anvil]

;{

its sad

[woeful pasture]

you're sad

[alpine anvil]

yeah and what

[woeful pasture]

the issue with it is it just doesn't follow plugin lifecycle

[random yacht]

It has the same vibe as

new HashMap<>() {{
    add(a, b);
    add(c, d);
    add(e, f);
}};

Which also sucks, by the way, don't do that

[woeful pasture]

yay class path polution

[dense leaf]

Kotlin mapOf and buildMap my beloved

[flat tiger]

alright then

i shall take all of this very seriously

[sleek shard]

hey it's put! not add! mr hypixel dev, don't break something

[pine grail]

that instance block is straight out of C#

[random yacht]

Right, sorry, put. lol

I've been doing some C# recently and it's Dictionary#add()

[pine grail]

sidenote -- this is fine if you want to initialize a psf variable, it makes it look nicer imho, it only creates an extra class once then

definitely don't do it in anything that gets called more than once tho

[random yacht]

Map#of() exists, and if you're in an older version of Java, use a utility method. Even Mojang has one

It's like Util#make() or something

[pine grail]

this works for any class iirc

[random yacht]

Or if you really really want to, ImmutableMap.builder()

There are plenty of alternatives to creating anonymous inner classes and abusing the initializer block

[woeful pasture]

I use immutable builder sometimes

Fairly nice method

[pine grail]

I mean fair enough but it is funny to use sometimes ¯_(ツ)_/¯

[flat tiger]

im guessing hypixel has some super high code standards so id be glad to hear criticism from choco

i am also working on all the comments i got from everyone rn

[pine grail]

choco is the exception then (JOKING! Love you choco <3)

[pine grail]

extra one

for clarity I like to do

this. to access own fields, own methods, or overridden methods
super. to access superclass fields or methods

:P

i.e.

public class Test implements SomeOther {
  @Override
  public void someOtherStuff() {
    System.out.println("Some other stuff!");
  }

  public void runTest() {
    super.prepareSomeOtherStuff(); // Not overridden / final method
    this.someOtherStuff();
    this.doTestStuff();
  }

  private void doTestStuff() {
    System.out.println("Doing test stuff!");
  }
}

(really dumb example)

[random yacht]

That (usually*) is a preference thing tbh. I'm very picky on my this/super preferences too

[flat tiger]

i try to use this. in places for readability but if im honest i think i forget it 90% of the time

[random yacht]

I prepend this to field and method access if it's the start of the line

And I only use super if I have to

[pine grail]

yeah ^

I just do it to make it clear where something is, also I like the look of it lol

[flat tiger]

never had to use super but yeah i try my best to be consistent with this.

[pine grail]

Also, I usually never use static imports for the same reason, unless they are OpenGL/CL imports

[random yacht]

this.something(); // Good :)

if (this.something()) { } // NEVER >:(
else if (this.value == this.somethingElse(this.otherValue)) { } // I'd rather kms

[flat tiger]

honestly the private static final plugin is carried over all the way from back when i was watching tutorials
i was honestly just following what i was taught lmao

[pine grail]

I do all of those lol

[flat tiger]

i do the first and forget what im doing by the time i get to the others so im good i think

[random yacht]

Even if a method or field comes from a parent type, I still use this. The only time I use super as a qualifier is if I need to do it because I specifically want to call a super method that I overrode in the child type. It's a rare occurrence but it happens

[pine grail]

I like to use super. in order to make 101% sure I am calling what I want lol

[random yacht]

e.g.

@Override
public void foo() {
    System.out.println("hi :)");
}

public void bar() {
    super.foo(); // Won't print the "hi :)"
}

I actually had to do this at work somewhat recently and left a comment saying "This is a stupid hack" or something like that lol

Because it's hard to read and very awkward, but I had to do it for compat reasons

[flat tiger]

i sort of just call what i call
and if it works it works

[random yacht]

Ultimately though, it's preference. Doesn't change your code functionality at all (outside of instances like this ^). I just prefer the look of this at the start of a line but hate it mid-line for some reason lol

[flat tiger]

no ones reviewing my code
as long as its not horrible, it should be fine

[random yacht]

You should be writing your code with the mindset that future you is going to review your code and think "What the fuck was I doing?"

"What dumb-ass wrote this code?"
*checks git blame*
<--- me, the dumb-ass

[flat tiger]

yeah i always think while writing code "am i writing good code?" "what will other devs think?"
and that feeling started when i started coding in python 5 years ago and its still with me
but now i finally have a chat of people to actually answer that question

tbh i think ive improved immensely since the starting days, or even from 2 years ago

and i feel good about that

[random yacht]

You're going to be improving year over year for a long time

[sly jackal]

for me its more like, lets try to write the best stuff I can, will refactor this in the next year anyways

[random yacht]

I'm still improving

[flat tiger]

im coming for your job choco
one. day.

[random yacht]

Maybe one day you will!

I didn't think I'd get the job lol. Somehow I convinced 'em

[sly jackal]

well first you convinced yourself, that can often be the hardest part

[pine grail]

a lotta tricks

[flat tiger]

i still am not convinced that im even a decent dev
even tho i freelance lmao

i am generally more confident in my python code than java code tho

[pine grail]

that feeling never goes away, I think, I haven't had it go away yet

[sly jackal]

thats precisely what I meant xD

[flat tiger]

ive always found my passion to be programming, all the way from back in 6th grade
always been that programmer nerd in school
i hope to make a proper career out of this, i just gotta get better

one day man.

[flat tiger]

https://github.com/SiliQon-Development/CosmicCosmetics

alright this HAS to have some improvement

i took all of yesterday's feedback and put it into the code to the best of my understanding

afaik i did not miss any feedback

[jagged rock]

Not a huge fan of your plugin's main class naming

Should me CosmeticsPlugin IMO

wildcard imports are wild

More of a code style issue but make sure to add braces to everything

Or at least be consistent with it

(single line else has them but the single line if doesn't?)

[alpine anvil]

id still recommend not having public fields, provide getters or map modification methods
instead of having 5 different registries to store the data of an enum, create a data wrapper class

[jagged rock]

This isn't necessarily SOLID compliant

Neither is this

big .yml file for data storage 🤮

Inconsistent spacing between parts

HaloCosmetics is a horrible name for a menu

All these EffectWhatever registries can be converted into a data class

public class EffectData {

  private final Effect type;

  private final String whatever;
  private final List<String> whatever

  ...
}

single letter variable names 🤮

this is also cursed, include the 0 and I'd suggest having them in different lines

Also they can just be constants smh

keyset-get -> entrySet

You need to re-read the ENTIRE good habits post in #help-development

(there are more below the pinned one)

mm your Registry is just a Map wrapper

[dense leaf]

Yea we talked about that yesterday here

[flat tiger]

why not use the name of the actual plugin?
dont tell me theres a convention for that too because i will not believe you

[dense leaf]

?main

[rich fog]

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

[jagged rock]

I'd reserve the plugin name for any API and suffix Plugin for the main class to be explicit it's the main class

[flat tiger]

single letter variable name where

[jagged rock]

But ideally the events n stuff

look for it

I'm not your dad

[flat tiger]

ok yeah thats me

[jagged rock]

But ideally you just suffix your class name with the purpose it serves (CosmeticsAPI, CosmeticsPlugin, CosmeticsWhatever)

And if you expect it to be used a lot I'd probably also prefix by the project name

[dense leaf]

wait really

[jagged rock]

(SkyblockAPI, SkyblockDataManager, SkyblockPlugin, SkyblockIslandContainer)

[flat tiger]

someone said something about magic numbers

[flat tiger]

hm, i see now

[jagged rock]

I said magic numbers icky and started a 2 hour argument

[dense leaf]

That was me correct

and him

[flat tiger]

yeah rad is the reason i went in there and made sure there were no magic numbers

[dense leaf]

which is why you don't wanna argue with frosty

[flat tiger]

unless i understood what a magic number is wrong

[jagged rock]

yeah arguing with him is

[flat tiger]

since the last time you said that i tried my best to make this change in every place i noticed it fit

so do i revive the magic numbers final boss or what

[jagged rock]

NEVER expose collections

If you have to return a collection make sure it's a defensive copy

ideally immutable

[flat tiger]

yeah i did that on the registries, and afaik the only directly exposed ones left are the two in the main class

others have getters

[jagged rock]

and why would you have collections in the main class

[flat tiger]

wonderful question

[jagged rock]

zero point in having collections / configurable values / whatevers in the main class

because the purpose of the main class is to serve as an entry point to your project

Not to be everyone's mother

[dense leaf]

those should be managers

[jagged rock]

not managers, all of them represent a characteristic of an effect

it should all be bundled into one data class

[alpine anvil]

^^

[dense leaf]

or that yea

[flat tiger]

yep alright ok so

ill make a EffectDataManager

i will convert CosmicCosmetics to CosmeticsPlugin

[jagged rock]

reflect on your life's choices yeah

and re-read the good habits post

[flat tiger]

and that

and i will uhh fix that keyset thing in the listener (i was probably VERY high during that)

[dense leaf]

"convert" makes it sound like it's actual work

[flat tiger]

refactor -> rename

[dense leaf]

takes 2 seconds

shift f6

[alpine anvil]

not the yeconds

[flat tiger]

i will refactor -> rename it
is that ok now mom

[dense leaf]

no you will shift+f6 it

[flat tiger]

too bad its already done

but just to be clear

i did fix some stuff correctly from what was mentioned yesterday, right?

[pine grail]

https://github.com/IkeVoodoo/go-copy

flame me lol, first actual begin-to-end project in go

[woeful pasture]

https://github.com/Y2Kwastaken/VoidCR/pull/1/files

take a crack I made an event system curious what ya'll think

[pine grail]

LifecycleManager.XXX_PRIORITY should all ideally be config properties

[woeful pasture]

Why configuration properties? That seems pointless in a hard defined system from 0 to 99

Adding it as a config property would introduce uncertainty to plugin developers

[pine grail]

fair enough, my main worry is that sometimes you may need more priorities but fair

[noble eagle]

https://tenor.com/view/ratatouille-cute-rat-eating-spaghetti-gif-15430973

[muted sentinel]

Let me have it, ended up recoding this project

https://github.com/MrArcane/MineCore

[late smelt]

Move that unignore command where it belongs this instant!

It’s all lonely and sad

[alpine anvil]

first few things

packages should be lower case not snake case
the main plugin class should be something like MineCorePlugin
you have a command in the main directory not commands
in server utils, the only player command should be final and have macro case

[muted sentinel]

I did! for some reason github wont change lol

[late smelt]

Did you commit and push

[alpine anvil]

https://github.com/MrArcane/MineCore/blob/minecore/src/main/java/me/arkallic/mineCore/listeners/PlayerJoinListener.java#L26 is a bit funky to remove it twice
avoid exposing collections directly, have modification methods
https://github.com/MrArcane/MineCore/blob/minecore/src/main/java/me/arkallic/mineCore/managers/HomeManager.java#L25 no need to remove , add will override

[muted sentinel]

yeah. i will try again after i fix some stuff.

[rancid fern]

Logic like this should not be done in the constructor https://github.com/MrArcane/MineCore/blob/minecore/src%2Fmain%2Fjava%2Fme%2Farkallic%2FmineCore%2Fwrappers%2FYMLFileWrapper.java#L28

[muted sentinel]

Thats what i thought but it didn't so i ended up adding that.

[late smelt]

If homes is a set it won’t override

It’ll just ignore

If it’s a list it’ll just add another one

[alpine anvil]

appears to be a list

might be worth using a map for that to allow for updating + easier searching

[muted sentinel]

is there a reason?

[muted sentinel]

Ill look into that

I remember doing this because i thought having a hashmap per home would be a lot of overhead for what it is

[alpine anvil]

hashmap per player over per home

[random yacht]

The constructor should have no side effects. A constructors purpose is to construct an object

For what it's worth, I'm guilty of doing the same thing on occasion, but it really isn't good practice

[alpine anvil]

the difference is you might have been paid to do it

[random yacht]

There's functionally no difference between these two snippets

YMLFileWrapper wrapper = new YMLFileWrapper(...);

// vs

YMLFileWrapper wrapper = new YMLFileWrapper(...);
wrapper.createFileIfNotExists();

Aside from the fact that the constructor is no longer responsible for creating the file

[late smelt]

It’s okay to write bad code if you get paid for it

[muted sentinel]

oh so like a hashmap inside the playerData as like
Hashmap<String, Home> homeMap

[alpine anvil]

yeah

[random yacht]

While I'm in the file, a neat little thing to mention is that your YMLFileWrapper object is very tightly coupled to your MineCore object when it doesn't have to be. I know this isn't the purpose of the class, but say I wanted to copy/paste this class into another project, I'd now have to refactor it to get rid of the MineCore class.

Because all you're doing with that type is calling getDataFolder() and getResource(), you could replace your MineCore references with just Plugin which defines those methods. Now you can use that class with any plugin, including MineCore

You really only ever want to accept as a parameter the highest level of abstraction that's required for your code to function*
*there are exceptions to this rule, as with any rule

[alpine anvil]

i dont remember what its called, but when you define something like a hashmap set the type as Map then still use new HashMap

[random yacht]

Liskov's Substitution Principle, part of SOLID code

That's the principle I'm referring to as well

[alpine anvil]

yeah that one

[late smelt]

Programmers coming up with convoluted names for simple concepts:

[random yacht]

If for each object o1 of type S there is an object o2 of type T such that for all programs P defined in terms of T, the behavior of P is unchanged when o1 is substituted for o2 then S is a subtype of T.

Not confusing at all

[late smelt]

Ur a subtype of T

[random yacht]

tf?

[alpine anvil]

no i think hes a subtype of C

[dense leaf]

so true

[sly jackal]

isnt liskov just saying that you should be using interfaces?

[round fossil]

No not at all

It says whatever you depend on, that’s the level of abstraction you should require

[flat tiger]

wrong channel wrong time

[round fossil]

more of a never channel never time

[round fossil]

For example if you depend on any object that adheres to the contract of the Map interface, then depend on Map, if the map needs to support concurrent multithreaded usage then ConcurrentMap, if you specifically need a HashMap such that two keys A and B are considered the same if and only if hashCode(A) == hashCode(B) and equals(A,B) <-> equals(B,A) (ofc it follows A == B and; if A and B is equal -> hashCode by specification should be the same for A and B), a map that breaks this rule is would be IdentityHashMap, a map that is not breaking this rule, but is not “hashing” yet abides the specification of Map is an array map for example (can be found in fastutil)

[frosty grove]

⬛

[round fossil]

?

[digital galleon]

⬛

[round fossil]

I’m warning, don’t just spam nonsense here, same applies to you @frosty grove

[digital galleon]

i thought you'd f w it.

sorry

[sly jackal]

sure that makes sense, so just like pick the right layer that gets the job done

but nothing more, so that implementations can go wild

[frosty grove]

damn

unlucky

ok but what the fuck is liskovs substitution principle jsut call it like

idk

[sly jackal]

yes, idk is a way better name for it

thats why I always use Object obj = new MySuperConcreteClass()

[frosty grove]

same

and use reflection to call every method

[sly jackal]

no

[frosty grove]

yep

[sly jackal]

all my methods take Object and then just cast it

[digital galleon]

pfff amazing

[jagged rock]

It's pinned on #help-development

Basically you should be able to have a variable's type as the most generic you'll need (For example, Collection<String> instead of ArrayList<String>) and it should always work

ImmutableMap breaks LSP imo but it's more of a deep rooted issue that will never be fixed because too many ppl use it

Proper approach would be to make Map immutable and MutableMap mutable

[round fossil]

ImmutableMap doesnt break LSV

[jagged rock]

:(

feels like it does

[random yacht]

The immutable collections definitely don't break it

public <T> void print(Collection<T> objects) {
    for (T object : objects) {
        System.out.println(object);
    }
}

print(ImmutableList.of("a", "b", "c"));

This is still valid and a good example of the LSP

[sly jackal]

probably breaks all the other letters in solid

[random yacht]

If you want to call add() on that Collection, then you're accepting the wrong type

The Collection interface declares that mutable methods might throw exceptions if the collection doesn't support it

If a collection refuses to add a particular element for any reason other than that it already contains the element, it must throw an exception (rather than returning false).

Eh, I guess Collection isn't the wrong type, but you're still respecting the LSP and it's the fault of the caller at that point lol.

[round fossil]

yea, I mean, ImmutableMap does break the contract of Map depending on how you look at it, but not necessarily LSP/LSV - Java’s anonymous unmodifiable types tho are just built different

[random yacht]

Don't think it does break contract. Map also states that put() might throw an exception if it isn't supported

Technically values() and entrySet() state they support removal, so I guess that's a violation because an ImmutableMap would return an immutable view, but whatever lol

[jagged rock]

eh relying on a technicality

[junior holly]

https://github.com/NormalManV2/NormalShop/blob/master/src/main/java/normalmanv2/normalShop/api/Context.java

This was an attempt at making redundant tasks a bit easier... I suppose it's basically just a data class and am not sure if I overcomplicated it or not... My specific use case for the context would be for UI related tasks such as button clicking and what not. I figure a lot of buttons in a UI (shop UI anyway) have similar functionality and with the "modern" approach (being the whole #click(InventoryClickEvent)) it might be nice to pass a bit more information prior to the handling. ie: Context object. That being said y2k mentioned it may not be so great to allow for any data to be added to any context obj so I was thinking of how to do more strict type enforcement on entries.

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

Example ^, any sort of data can go in here, though this plugin isn't intended for public release so I mean I kind of have the excuse of "I know what I'll be doing" but then again I'm not so sure... as it stands any impl of context could hold any sort of data

[night knoll]

.

[late smelt]

No

Banned

[night knoll]

wot

[late smelt]

B a n n e d

[night knoll]

why :(

[late smelt]

For not posting code to review!

[junior holly]

Hey you didn't review my code D:

[sleek shard]

B A N N E D

[night knoll]

Very

[wintry fern]

she just making sure thread don't get removed 🙂

[dense pond]

private ItemStack parseTool(ConfigurationSection toolSection) {
        if (toolSection == null) return null;
        String materialString = toolSection.getString("material");
        if (materialString == null) return null;

        Material material = Material.matchMaterial(materialString.toUpperCase());
        if (material == null) return null;

        int amount = toolSection.getInt("amount", 1);

        ItemStack item = new ItemStack(material, amount);
        ItemMeta meta = item.getItemMeta();
        if (meta == null) return item;
        String displayName = toolSection.getString("display_name");
        if (displayName != null) {
            meta.setDisplayName(ChatColor.translateAlternateColorCodes('&', displayName));
        }
        List<String> lore = toolSection.getStringList("lore");
        lore.replaceAll(line -> ChatColor.translateAlternateColorCodes('&', line));
        meta.setLore(lore);
        item.setItemMeta(meta);
        ConfigurationSection enchantmentsSection = toolSection.getConfigurationSection("enchantments");
        if (enchantmentsSection != null) {
            for (String enchantName : enchantmentsSection.getKeys(false)) {
                int enchantLevel = enchantmentsSection.getInt(enchantName, 1);
                Enchantment enchant = Registry.ENCHANTMENT.get(NamespacedKey.minecraft(enchantName));
                if (enchant == null) continue;
                item.addUnsafeEnchantment(enchant, enchantLevel);
            }
        }
        return item;
    }

[alpine anvil]

any reason for not just storing the ItemStack directly?

[dense pond]

readable

    tool:                     
      material: "IRON_AXE"      
      amount: 1                 
      display_name: "Siekiera"  
      lore:                     
        - "Pierwsza linijka"
        - "Druga linijka"
        - "Trzecia linijka"
      enchantments:             
        efficiency: 5
        unbreaking: 3

this is yml

[jagged rock]

eh

not a fan of monolithic methods

if you want to add leather color etc it gets icky

I made item codecs for that :)

public interface ItemStackCodec {

    void encode(ItemStack item, ConfigurationSection section);
    void load(ItemBuilder builder, ConfigurationSection section);

    String getKey();
}

something like that

and then I made a few

[dense pond]

what does this mean?

[rancid fern]

One giant method that does multiple things

Instead of breaking it down

[dense pond]

oh

[dense pond]

what is this

can i see it

[late smelt]

I assume each one writes its corresponding part to the config section

Or reads it

[digital galleon]

Does "Codec" have a meaning in this usage?

[late smelt]

A codec is a computer hardware or software component that encodes or decodes a data stream or signal. Codec is a portmanteau of coder/decoder.

Thanks Wikipedia

Also TIL the origin of the word codec

[digital galleon]

oh yeah knew dat, like video codecs and shit, but I was wondering... in the context of these files.

[late smelt]

Well it encodes and decodes an itemstack

[digital galleon]

(I didnt read context of chat, so I was just curious what his classes were)

OHHH

[round fossil]

Feels like we’re mixing up codecs with serialization here though when they are in fact not entirely the same

[desert ore]

but they are related

[wintry fern]

serializing is a subset of encoding

however, it would appear that the above is more serializing then being a codec

Encoding/Decoding has to do with the bytes itself and how those bytes need to be interpreted or what they mean

Serialization has to do with the storage of data or object and not the bytes itself

for example, transforming xml to json is serialization. But taking the bytes from the xml and then converting the characters to something else is encoding/decoding

[tacit smelt]

Castlevania style?

[dense leaf]

https://pastes.dev/DMwsR82njY roast my game data system

[pine grail]

kotlin, terrible

jokes aside, it's not too bad

the only pet peeve I have is that getData could possibly be used to get per-player kits, and you'd need to specify which player, but I guess a map would work lol

[dense leaf]

It's per-instance data

[pine grail]

ah

well then I'd say it's good

[dense leaf]

For player data, every instance has a player data manager attached, which you can override with your own data

[pine grail]

yep makes sense

[dense leaf]

this is boring I want people to actually roast my code, I've only really gotten a "looks good" for everything I've posted here :(

[sleek shard]

lgtm 👍 (merge)

[dense leaf]

(has not looked at the code)

[sleek shard]

(breaks entire prod database)

(rm -rf / --no-preserve-root everything)

[dense leaf]

(runs dd if=/dev/random of=/dev/* bs=1K)

[night knoll]

Why pull request when you can git commit --force

[sleek shard]

😭

[dense leaf]

i love force committing

[dense leaf]

me when all block devices, stdio and tty gets written with random bytes at runtime

memory too potentially?

[dense leaf]

actually dd can only take one output: find -type f /dev -exec dd if=/dev/random bs=1K of={} \;

[woeful pasture]

If you want people to roast your code stop writing good code

[dense leaf]

but that's so hard

[woeful pasture]

Or write harder projects like reimplementing every data structure

[dense leaf]

that sounds fun

[woeful pasture]

Then I get to make fun of your shitty hashing algorithms

[dense leaf]

I should do that on company time

[hybrid osprey]

Write a doubly linked list in rust

worst ride ever

[woeful pasture]

That's even easy in C what tf did rust do?

[rancid fern]

Borrow checker I assume

[hybrid osprey]

Yep

[pine grail]

because as long as you don't delegate the server ticking to a server in Botswana using HTTP/1.1 clear-text requests and encoding the bytes as the string representation of the array it's not gonna be that debatable, if it works like it is supposed to and it's not complete garbage, it's down to preference lol

PS: sounds way too aggressive written out, sorry

[late smelt]

Well

There goes my idea

[hybrid osprey]

So a server in Mongolia is a ok?

awesome, I'm clear

[dense leaf]

Yea perfectly fine

[pine grail]

#verified message

Mainly for interface naming & structure, roast it if you want :P

[dense leaf]

https://pastes.dev/4qQGCNGkFa more refactoring!!

i love kotlin delegation :)

[alpine anvil]

delegate me out a window

[dense leaf]

always

[solemn vine]

yo this thread is cool

i will send my code here when i write good stuff soon

[sleek shard]

pog

[night knoll]

Make sure that you send front and back of credit card codes for us to review

[solemn vine]

https://tenor.com/view/credit-card-smacking-credit-card-on-table-gif-21340324

[dense leaf]

https://github.com/radstevee/axi my first ECS impl, the internals really suck but it's nice to use: #verified message

[alpine anvil]

rad is typing...

[dense leaf]

also rate my commands :3

[jagged rock]

Too basic

Doesn't support generic entities

Persistent components will need types and codecs

I'd like to see an "entity click" component

[dense leaf]

elaborate?

[jagged rock]

tracking data on regular mobs

like making an explosive arrow

[dense leaf]

well Attachable pretty much

or WrappedAttachable in this case

[dense leaf]

yeah they're not fully persistent persistent, just persistent across reconnects, I'm not sure whether I want that to change though, since you should be using a DB for the rest

[sullen hollow]

Hey yall, anyone wanna give me some ideas or tips for my first really usable plugin? It is a plugin that allows to create camera paths (drives) really fast and easy using a script language...
Here is the link https://www.spigotmc.org/resources/aquilacine.123182/

[dense leaf]

I wanna try implementing something with the ECS so, what does it do?

[rancid fern]

A component that does something when you click an entity ig

[dense leaf]

does it just accept a function and maybe a filter or something

[alpine anvil]

first few things things i can see from the code,

• setup a git ignore, you shouldnt push .gradle, .idea or /build
• name your main class something like AquilaCinePlugin

[jagged rock]

Yeah

entity.onClick { context ->
...
}

[dense leaf]

ah alright sure

[rancid fern]

kotlin >:c

[sullen hollow]

ohh I see... IntelliJ always creates a gitignore inside my .idea folder and that yeah... doesnt work like it should

[dense leaf]

no u

[jagged rock]

I ported my kt ECS to java

[dense leaf]

I'm assuming this is just a util for adding such component?

[jagged rock]

sure

[dense leaf]

okay I'll implement it once I'm back on my machine

[alpine anvil]

rad go home

[jagged rock]

way I do it on my java port is uh

[dense leaf]

I am home

[alpine anvil]

go to pc

[jagged rock]

ComponentEntity componentEntity = ComponentEntity.getOrCreate(entity);
componentEntity.addComponent(EntityComponentTypes.ENTITY_CLICK.createComponent((context) -> {
  ...
}));

[dense leaf]

What about ```kt
player
.onClick { entity -> }
.filtering { entity -> entity.type == EntityType.PIG }

[jagged rock]

meh

reminds me of my current ability syste

[dense leaf]

why meh? Looks nice to me

[jagged rock]

could use more finesse

[dense leaf]

oh god

[jagged rock]

 public static final AbilityTypeData TOGGLE_INSTANT_KILL = AbilityTypeData.builder("toggle-instant-kill")
        .withIcon("\uE021")
        .withActions(
            bind("entity-click")
                .withType(AbilityTriggerContextTypes.ENTITY_CLICK)
                .withRestriction(AbilityMetadataRestriction.is(ACTIVATION_STATE, ACTIVE))
                .to(InstantKillAbilityAction.create())
        )
        .withTemplate(AbilityTemplates.toggledAbility(parse("10 seconds")))
        .build();

🤔

[dense leaf]

hardcoding unicodes 🤢

[jagged rock]

the icon logic's just a quick utility thing I got a whole rendering sys

nothing configurable yet it gets complex

still working on this

[dense leaf]

I also have a whole rendering system hehe

[jagged rock]

I can prob do the usual thing and convert this whole sys to a scripting engine

should I 🤔

[dense leaf]

no

[dense leaf]

how do you properly seperate the actual component from the listener tho? a system or something similar?

[jagged rock]

The types manage the listener

types are singleton

[dense leaf]

interesting

[jagged rock]

Not the best approach but I found it surprisingly good

[dense leaf]

@jagged rock https://pastes.dev/hG5FcwPMLd something like this?

[jagged rock]

eh that can work

I usually split it into types and components

[dense leaf]

        player
            .onClick {
                player.sendRichMessage("You clicked a pig called <rainbow>${entity.name}</rainbow>!")
            }
            .filtering { entity.type == EntityType.PIG }

[jagged rock]

you could use more filters

also why is your filter mutable

[desert ore]

his filter is mutable?

[dense leaf]

idk the filtering function just sets it

onClick just returns an added component which you can do whatever you want with

[dense leaf]

        private suspend fun handle(player: Player, entity: Entity, isLeftClick: Boolean, isRightClick: Boolean) {
            if (!player.checkDebounce(EntityClickedComponent::class)) {
                return
            }

            val component = player.get<EntityClickedComponent>() ?: return
            val ctx = ClickContext(player, entity, isLeftClick, isRightClick)

            player.addDebounce(EntityClickedComponent::class, 1.ticks)

            if (component.filter(ctx)) {
                component.handler(ctx)
            }
        }

This is actually very fun I'm enjoying this a lot

I could use some kind of ID for the debounce stuff but I think classes make more sense here

https://pastes.dev/d1pJsOduuq it's so easy to implement stuff like this too

[solemn vine]

https://github.com/LumenMC/Lumen/tree/Rewrite

[solemn vine]

(btw this is a plugin loader for minestom)

[rancid fern]

Not sure why that is a fork rather than it adding Minestom as a lib

[solemn vine]

You are right about that

i will see to converting it into a lib

[muted sentinel]

is this bad?

 public void sendTPAMessage(MessageType type, Player player, String message) {
        TextComponent textComponent = new TextComponent(message);
        ChatColor color;
        Sound sound;

        switch (type) {
            case SUCCESS:
                color = ChatColor.GREEN;
                sound = Sound.ENTITY_PLAYER_LEVELUP;
                break;
            case WARNING:
                color = ChatColor.YELLOW;
                sound = Sound.BLOCK_NOTE_BLOCK_BASS;
                break;
            case ERROR:
                color = ChatColor.RED;
                sound = Sound.ENTITY_VILLAGER_NO;
                break;
            case TPA_REQUEST:
                color = ChatColor.AQUA;
                sound = Sound.ENTITY_EXPERIENCE_ORB_PICKUP;
                break;
            default:
                color = ChatColor.GRAY;
                sound = Sound.UI_BUTTON_CLICK;
                break;
        }

        textComponent.setColor(color);
        player.spigot().sendMessage(textComponent);
        player.playSound(player.getLocation(), sound, 1.0f, 1.0f);
    }```

I just noticed i used player, switched it to uuid

[late smelt]

I don’t see a reason to not use player

It’s not like you’re storing it anywhere

[alpine anvil]

^^

[muted sentinel]

I thought putting player inside paramaters is bad for memory leaks?

[rancid fern]

Not really

[alpine anvil]

only holding a reference to it, eg a collection after theyve logged out

[muted sentinel]

ah i see

[lunar vale]

is expiring it after some time the players has logged out a good way to prevent memory leaks?

[rancid fern]

You want to get rid of it a soon as the player quits

Not after some time

[round fossil]

I mean if you work with user data it can be fine to let the user data live around in the cache layer for a while where direct subsequent joins don’t necessarily have to load the user data from storage, with player entity objects the most important part is to not block GC from sweeping them at least (for example using WeakReference)

[muted sentinel]

Mine removes the map on quit and after 30 seconds

[alpine anvil]

atm my setup just uses uuids and purges the data after 15 minutes to avoid loading data again so soon

could probably shorten it by a chunk

[late smelt]

I didn’t know you could use chunks as a measure of time

[dense leaf]

i think he means that he could shorten it by 16^3

no that's wrong

16*16*384?

don't remember thr height of a chunk nowadays

[late smelt]

384

[dense leaf]

that's such a weird name

number

i literally cannot type

[late smelt]

It used to be 256, they added 64 above and 64 below

[alpine anvil]

so wouldnt that make it 320

[late smelt]

256 + 64 + 64

[round fossil]

After 30s?

I hope thats not during a session

[hard lantern]

https://codeberg.org/kentoj/Aasdasd/src/commit/91fc2152f5db3d0b3c723c35b375c81935061262/src/main/kotlin/me/kentoj/aasdasd/Guis.kt
usage is something like this:

private fun newPianoGui(): Gui {
    return Gui("Piano", 9) {
        for (slot in 0..<inventory.size) {
            setItem(slot, ItemStack(Material.COAL)) {
              // ...            
            }
        }
    }
}

not rlly sure how much oop i should do in kotlin

[sharp epoch]

as much as you're comfortable with really, even though kotlin leans more on the functional side, it is a multi-paradigm language so you can go either way with it

[dense leaf]

Rename IGui and make a builder function imo

IGui -> Gui, Gui -> GuiImpl or something, then you can call the builder function Gui (Yes this is a kotlin convention)

(the convention: static factory functions can have a PascalCase name if their return type has the same name and is abstract/an interface)

[hard lantern]

so i make a new function called Gui which is a wrapper around GuiImpl?

[dense leaf]

a builder yes

[hard lantern]

never heard of that term before

isnt builder the thing that returns its own instance?

[dense leaf]

well it's an object that lets you build another object pretty much

see https://github.com/radstevee/axi/blob/master/core%2Fsrc%2Fmain%2Fkotlin%2Fnet%2Fradstevee%2Faxi%2Fcommand%2FCommandBuilder.kt

where I just do ```kt
/** Builds and creates a command. */
public fun Command(
name: String,
plugin: AxiPlugin = AxiPluginHolder.plugin(),
block: @CommandBuilderDsl CommandBuilder.() -> Unit = {},
): Command = CommandBuilder(name, plugin).apply(block).build()

[hard lantern]

and whats a factory then?

or is it just different in java

[dense leaf]

An object/method that creates other objects, often based on specific parameters

Builders are different in the sense that they're often chained calls and are "more user-friendly", e.g. ```kt
val info = resourcePackInfo()
.hash(pack.hash)
.uri(uri)
.build()

[hard lantern]

kinda get it maybe

https://codeberg.org/kentoj/Aasdasd/src/branch/master/src/main/kotlin/me/kentoj/aasdasd/Guis.kt
and updated

[night knoll]

You can steal some code here :) https://github.com/NazarXeXe/XeXeGui/

[hard lantern]

woooow

but the project was rather for trying kotlin once again

[dense leaf]

https://github.com/noxcrew/interfaces-kotlin is by far my favourite

[solemn vine]

https://github.com/LumenMC/Lumen/

[rancid fern]

No README

[solemn vine]

yo, now it is not a fork anymore.

pls give me suggestions on improving the plugin loading mechanism

[solemn vine]

yeah im writing docs with vite press, will prolly port from that

[rancid fern]

Don't need the entire docs in the readme

but alr

Highly recommend using NIO instead of the older IO (Path instead of File) API

Would help with loading plugins from different file systems

[solemn vine]

Ok , will switch to nio soon

i want to know what u think about https://github.com/LumenMC/Lumen/blob/main/src/main/java/com/lumenmc/plugin/DependencyResolver.java

i ported it from stack overflow or smth a long time ago

i do not fully understand it

i tried to write it by myself a lot of times, but the soft depends didn't work at all

[alpine anvil]

• outdated gradle (cc @dense leaf)
• wrong shadow dep, use the gradleup version
• pascal case method names https://github.com/LumenMC/Lumen/blob/main/src/main/java/com/lumenmc/configuration/LumenYaml.java#L45
• are you sure you want default for some of these instead of throwing errors if not present https://github.com/LumenMC/Lumen/blob/main/src/main/java/com/lumenmc/plugin/PluginDescriptionFile.java#L15
• adding a prefix to logger seems funky, specify a global logger it uses or configure it in log4j

[dense leaf]

outdated gradle!?!??!!!??11!?1??3eleven

[solemn vine]

alr, i will fix all that

[round fossil]

You made it hard for yourself

Why use a set of raw maps to deal with this highly nieched graphing problem

I’d advice writing (or using) some sort of graph data structure to make it easier for yourself, in terms of readability and maintainability

Remember, code is often read more times than it writes

[sharp epoch]

eh, it's a pretty recent version of gradle

they can probably upgrade just by doing .\gradlew wrapper --gradle-version 8.13

though they are using the old shadow plugin, should be using gradleup's one you already mentioned it, didn't notice lol

[dense leaf]

gradle 8.10 is old

[solemn vine]

blame intelij

i just clicked create new project, but yeah i will fix it

[sharp epoch]

3 minor versions ago, released just 7 months ago

[dense leaf]

just 7 months ago

[sharp epoch]

that's hardly old for gradle lol, they release a new vesion every month almost lmao

[dense leaf]

I just always stay updated

it's that easy

[sharp epoch]

tbf, with a script like that, I would agree

but following gradle's pace is completely not possible as the build configuration gets bigger

[sharp epoch]

gotta update to 2025.1 EAP for 8.13 to work properly apparently

[solemn vine]

mine is this weird ver

i get free ultimate because i uploaded my student id to github 🥶

[sharp epoch]

that's the latest release, you got to be in the early access program for 2025.1

[dense leaf]

? no

I use gradle 8.13 and kotlin 2.1.20 on IJ ultimate 2024.3

and it works just fine

[round fossil]

2023 intellij goated

[alpine anvil]

Does that even support j21

[jagged rock]

2016 intellij goated

(I still use the old theme)

[round fossil]

oh ye the icons hit different, fax

[sharp epoch]

I couldn't imagine why one would do that

the old theme was horrible

[round fossil]

I do like the retro/legacy icons

[alpine anvil]

i used the legacy ui for a while but swapped back to the new ui and i like it

takes a minute to get used to icons not names

(you can enable them but meh)

[jagged rock]

certified hood classic fym

[sharp epoch]

it was just a very Eclipse-looking theme

now it has more charm imo

[solemn vine]

i switched discord back to ash

full on black sucks ngl

[sharp epoch]

it has no charm

like every other site with a black theme

I like the compacter compact mode tho

[sharp epoch]

@sullen hollow probably better here

[sullen hollow]

thx

[dense leaf]

definitely seeing some gpt code there, you're storing player references in your GUIs, don't exactly know whether you clean those up properly but that's about it

[sullen hollow]

Yeah I somehow f**ed my brain at some point... welp, but I am pretty sure that doesnt affect the Memory in any way

[dense leaf]

not disposing player references leads to memory leaks

[sullen hollow]

oh

[dense leaf]

since a new object is created each time you join

[sullen hollow]

wait huh

Oh, so the private final Player player?

Would

HandlerList.unregisterAll(this);
Fix that issue?

[sullen hollow]

like on quit

[dense leaf]

on quit, the object gets removed from the server

if you're still storing references, that will lead to a memory leak

[sullen hollow]

Can I use something like
private final WeakReference<Player> playerRef; then?

So garbage can be collected better

[dense leaf]

just store the uuid

[sullen hollow]

Ill try that

[sullen hollow]

Seems to be working... Thanks again

[desert ore]

Maybe we could get a PlayerList (List<Player>), PlayerKeyMap (Map<Player, V>) and PlayerValueMap (Map<K, Player>) which internally stores UUIDs but returns instances 🤔

Sounds redundant tho

[sullen hollow]

Replaced Player references with UUID in WaystoneGUI, seems to be working just fine

[rancid fern]

So your GUI classes aren't getting cleaned up

[sullen hollow]

Yeah, and that

on quit

[rancid fern]

Try not make each one a listener

[sullen hollow]

Made that plugin for a few friends, just praying it will work now... Just wanted to code a simple Waystone plugin and ended up with that shit lmao😭

[jagged rock]

or be a hypixel and make an EntityId class

probably as a uuid wrapper with a getPlayer method

[rancid fern]

or at least unregister on player quit and don't register the listener in the constructor

[sullen hollow]

Was a home plugin at first, then I got that idea and asked AI for help with that GUI and ended up with this haha... Prolly just gonna leave it now and never touch it again