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

1 messages · Page 17 of 1

hasty lotus
#

I relay it through minimessage translator

sharp epoch
#

it gets annoying when it comes to arguments in the message

hasty lotus
#

Yeah

sharp epoch
#

well, if you want named arguments and not just %s in your string

hasty lotus
#

There’s a way to do it

#

I just haven’t like done it

sharp epoch
#

what's with all the functional interfaces

#

NullComponent, UniComponent, BiComponent and TriComponent

hasty lotus
#

For number of arguments

#

Then you just do

#

Message.HELLO.build(arguments)

#

And get your component

dense leaf
#

<certain "fork" that shall not be named> has i18n support + adventure has an MM translator now

sharp epoch
#

and make Message an enum instead of an interface, but either approach is fine

hasty lotus
#

Yeah

#

But that’s generally how I handle the messaging

sharp epoch
#

yeah, that sounds fine to me

#

I'd make a plugin template from it and you'd be good

#

I always meant to make a cli tool for that which would allow me to generate them easily but I never get to it lol

#

not that I make much plugins nowadays anyway

hasty lotus
#

Yeah

#

I don’t really make many

#

I only do like maybe one or two at once

#

Nothing more or less

sharp epoch
#

I make zero or one a year

#

so yeah, not much use for me lol

hasty lotus
#

Things just kinda get boring after some time

dense leaf
#

yea I prefer working on other things ngl

pine grail
#

MethodHandles generally fix that, iirc their speed is nearly the same as invoking the method directly, and the lookups should be faster, too

#

Discord placing me 200 messages back will never not be funny

wintry fern
#

again depends

#

all depends on what you are doing

pine grail
#

If you're using method handles as static variables and just calling invoke repeatedly on a psf it's the same speed as direct invocation

#

Measurably so*

wintry fern
#

ok but that isn't all of reflection

pine grail
#

Well yeah

#

But realistically you should use reflection for few purposes other than that

#

Main one is probably proxy instances

round fossil
#

yea reflection now "dynamically generates bytecode for ::newInstance and ::invoke"

pine grail
#

Or registering events like spigot does

round fossil
#

and VarHandle/Unsafe access for Field::set/get

pine grail
round fossil
#

depends how u define ur method handles

#

lambdametafactory does that tho

#

but its complicated to use iirc, gets pretty damn verbose

pine grail
round fossil
#

🥲

ivory lotus
#

I use it for my config system

#

With anotation and things like that

dense leaf
#

I mean yea, you'll usually use reflection for configs

ivory lotus
dense leaf
#

reflection has a lot of use cases

ivory lotus
dense leaf
#

you don't need to ping me for every single message

#

accessing private declarations is one that many people use it for

ivory lotus
#

Oh, I'm not really looking to use the ping thing, just really reply to the message

ivory lotus
round fossil
ivory lotus
#

https://github.com/system32developer/SystemCore/tree/master/src/main/kotlin/com/system32/systemCore/managers/config

HI guys, I was trying to use reflection for a config stuff

class LunarisSpawnerPickaxe : JavaPlugin() {

    companion object {
        lateinit var instance: LunarisSpawnerPickaxe
            private set
        val configContainer: ConfigContainer
            get() = ConfigManager.get(ConfigContainer::class) ?: error("ConfigContainer not loaded")
    }
    
    override fun onEnable() {
        instance = this
        AdapterRegistry.register(ItemStack::class, ItemStackAdapter)
        ConfigManager.initialize(this, ConfigContainer::class)

    }

    override fun onDisable() {
        // Plugin shutdown logic
    }
}

for this class it works, but it takes 8000ms (8 seconds, sometimes more seconds) to load, is it a normal behaviour of using reflection or can i optimize something

@Config("config")
data class ConfigContainer(
    val host: String = "localhost",
    val mySubConfig: MySubConfig = MySubConfig()
)

data class MySubConfig(
    val port : Component = color("asad"),
)
#

i reduced it 2s load time, now is 6s, but yeah, is still high

#

should i make a Node system like configurate

alpine anvil
ivory lotus
#

i need to recode it again, for third time

alpine anvil
#

The life of a developer

late smelt
#

Or you wait for someone else to recode it 5 times and then you “borrow” it

#

😉

finite juniper
#

oh sorry

#

i was readin old messages lol

dense leaf
#

probably wanna profile that

late smelt
#

Yeah that’s certainly something, mine usually loads fast enough I don’t even notice

ivory lotus
#

I'll recode it later

ivory lotus
late smelt
#

You probably want some sort of spatial data structure to handle regions

#

R-Tree, Octree, etc

ivory lotus
hasty lotus
#

No they're tree data structures

ivory lotus
#

idk what is that actually

wintry fern
#

otherwise you will never know if your code could benifit from using those things and making it perform better

#

however though generally you shouldn't be trying to optimize something if there is currently no problems

ivory lotus
#

mmm probably

junior holly
junior holly
#

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

Updated, implemented better boundary validations, generation attempt cycle, and changed some of the weight factors to make it more balanced in terms of obstacle variety

pine grail
late smelt
#

Disc golf

junior holly
hasty lotus
#

I'm sure this idea has been implemented in several other places. I'm developing a library, and I don't want my users to interact with an internal component (in my case, it's a native matrix). But within my library, I want to be able to access it in other classes, because using the native matrix would be very fast. Since my classes are in a different package from the internal component, it's not like I simply put it as protected and call it a day

jagged rock
#

Dependency Inversion

hasty lotus
#

My idea is to use some examinable interface where there are properties. For my internal component, I will assign my native matrix to like some key that can be accessed

jagged rock
#

Put the methods behind an interface but the native matrix as a public variable that's only accessible by explicit typing

#

You can find an example in my pinned good habits post

hasty lotus
#

they are behind an interface

#

But the issue is my interface has like a 1000 lines of code because of the amount of impl possible

#

its not ideal

jagged rock
#

then yeah just make a getter that's not behind the interface

#

and use the impl class

hasty lotus
#

I can't because my filters interact with the interface

jagged rock
hasty lotus
#

not the impl

#

my filters are designed to be extendible by the user

jagged rock
#

So you want the filters to have access to fast shit but not the extending classes hm

hasty lotus
#

Yes

#

Thats why i proposed the examinable approach

jagged rock
#

and the only "natural" way of doing that is with package-private

#

hm

hasty lotus
#

That way no direct access

#

But i can access via my own api

#

and maybe some very dedicated users if they rlly wanted to

#

could get it

hasty lotus
prisma ridge
jagged rock
#

Before I berate you for a million things ask yourself

#

Can you notice something, anything I'd complain about?

prisma ridge
#

possibly the enormous constructor signature for ModConfiguration

maybe how i could create other classes to move certain logic outside of the main plugin class (for cleanliness sake)

debatably i shouldve used getters for the fields inside of the main plugin class

jagged rock
#

Cool

#

Anything else in mind?

prisma ridge
#

im still lacking added documentation in a few of the classes

jagged rock
#

Let's worry about actual code rather than javadoc

#

Before actually understanding any of the logic the first thing that stood out to me, in the main class was the lack of consistency and purpose

#

The styling across these is inconsistent

image.png
prisma ridge
#

would u prefer early-return when target != null?

jagged rock
#

I'd prefer if your if statements had braces

prisma ridge
#

ah

jagged rock
#

I'd also prefer for it to be a separate method so you can reuse some code

#

something like trySendModData(player, ModConstants.SYNC, fabricData) where it does a null check

#

and maybe just maybe a sendModData(player, fabricData, forgeData)

#

that you then call in your loop or whatever

#

If you get fancy you can do some abstractions with things like PacketAudience that can be a single player or every player in the server

# 
PacketAudience audience = target != null ? PacketAudience.player(target) : PacketAudience.allPlayers();
audience.sendModData(ModConstants.SYNC, whatever);
audience.sendModData(ModConstants.WHATEVER, whateverElse);
#

but that's overengineering

#

Regardless

#

This should be a separate class with some kind of registry with lambdas

image.png
#

As a freelance developer the thing I most hate is having to fork a plugin because they're doing something stiff like this

#

at least in this case I can hack into it using reflection but still

prisma ridge
jagged rock
#

You have a logger, ex.printStackTrace instead of something more robust is at minimum an IDE warning (you chose to ignore)

#

I'd also prefer for a traditional entrySet for loop for modConfigurations instead of the glorified .forEach

#

Avoid repeating these calls a million times

image.png
#

Something like Sonarlint would've yelled at you that the logger can do this kind of formatting, which might save 2 clock cycles if the logger decides your message is irrelevant

image.png
#

Realistically broadcastModData should be in a separate class anyways

#

So should modConfigurations as an encapsulated map

#

welp I think that's enough for the main class

#

ModConstants lacks a private constructor

prisma ridge
jagged rock
#

ModConfigurationBuilder should do sanity checks before allowing you to build without any params set

#

private constructor, public static factory method expected

#

ModConfiguration having a million constructor params is icky and it should be split up into several classes

#

ModSyncPacket#getModRange should return some kind of custom class that facilitates with checks

#

such as .isApplicable or whatever, using strings is icky imo

#

This could realistically be a private constructor that takes an InputStream and a million static factory methods that take all sorts of inputs like bytes

image.png
#

more ifs without braces

image.png
#

no event priority?

prisma ridge
#

at most id put MONITOR, i guess

#

id be stating that i only intend to react to the event, and not mutate it

#

i dont mean to nitpick your nitpick, im just genuinely interested in the significance of actually specifying a priority

jagged rock
#

you want to be the first to know when whatever fires

#

On your PlayerQuitEvent if you have some other listener that depends on your plugin and you unload the data before them you're just causing an inconvenience

prisma ridge
#

i suppose

prisma ridge
prisma ridge
jagged rock
#

I'm sure you don't need the array

#

vararg be like

prisma ridge
#

there is no vararg method in Logger

#

plugins use the java.util Logger, not log4j Logger

#

otherwise i would be using varargs, lol

prisma ridge
#

installed sonarqube... it doesnt complain about something that looks equally as autistic

image.png
#

lets goo

ivory lotus
pine grail
ivory lotus
#

💀

pine grail
ivory lotus
#

yeah

#

is my own lib

#

i mean someone could use it

#

but only kt

#

because i dont do pl on java

sly jackal
#

dont listen to voodoo, they are too entitled

#

xD

pine grail
ivory lotus
sly jackal
#

looks fine

#

pretty basic stuff

#

I cant write kotlin so idk if there is any lang specific things

#

but from just reading it like a programmer its good

#

though its a little weird that you made a Vector3 type

#

you use it like half the time and iirc there is also a Vector/Vector3 from Bukkit that does the same

ivory lotus
tidal basin
#

Hello, what do you think of my PDC inventory storing and how could I improve it ?
I don't want it to be stored in files, because the project itself will be a library, and I would like to make content accessing simple to implement for others, in case someone wants to make compatibility. (Also, pdc can't be corrupted that easy by server administrators, that's not the case for files lol)
https://github.com/Paulem79/Krimson

More precise classes/packages :
https://github.com/Paulem79/Krimson/blob/main/src/main/java/ovh/paulem/krimson/serialization/InventorySerialization.java
https://github.com/Paulem79/Krimson/tree/main/paper/src/main/java/ovh/paulem/krimson/paper/versioned
https://github.com/Paulem79/Krimson/tree/main/spigot/src/main/java/ovh/paulem/krimson/spigot/versioned
https://github.com/Paulem79/Krimson/tree/main/src/main/java/ovh/paulem/krimson/blocks/InventoryCustomBlock.java

jagged rock
#

Why make an itemstack codec and version it if you're also making your own stream system?

#

It'd be more logical to just setup a writeItemStack method, for example, or even a writeItemArray

#

Paper has itemstack[] <-> byte[] methods

#

I also don't see the point in your PropertiesField system for your gui

#

And instead of empty stub methods I'd like to see a typed event system

#

For custom "tile" entities at work I took a more straightforward approach

#

I dunno why I made this diagram like this but whatever

image.png
#

In short:

  • A block type contains data on how to encode/decode the block data and create a "loaded block" out of it
  • A "loaded block" contains its own data and type, along with extra logic, such as disposal / click handling
  • The block data codec is responsible for encoding/decoding the blockdata to a PDC
  • That PDC can then be serialized into an item, to, for example, represent items within a custom chest
#

In practice that looks a bit like this

image.png image.png image.png
tidal basin
tidal basin
tidal basin
# jagged rock Why make an itemstack codec and version it if you're also making your own stream...

the "versioned" name has been a bit derivated and I should rename it, it's actually for choosing between spigot for item serialization/deserialization (https://github.com/Paulem79/Krimson/tree/main/spigot/src/main/java/ovh/paulem/krimson/spigot/versioned/serialize/BukkitItemSerializer.java) or paper's one, which is faster and more lightweight (https://github.com/Paulem79/Krimson/blob/main/paper/src/main/java/ovh/paulem/krimson/paper/versioned/serialize/PaperItemSerializer.java)

tidal basin
tidal basin
#

and like java interface EventListener<T extends Event> { void onCall(T event); }
?

dense pond
jagged rock
#

calling your main class MyPlugin is a bit dubious

#

we use that name for examples, I'd call it BlackholePlugin

#

package name doesn't follow convention

#

you can probably do something to not repeat these as much

image.png
#

your recipe could be configurable

image.png
#

I'd reorder the methods in the main class in the following order:

  • onEnable
  • onDisable
  • any logic methods (createRecipe)
  • getters
#

Your item being hardcoded is also a big bruh moment

#

More of a nitpick but add braces to your one-liner ifs, makes the code look more consistent

image.png
#

Is this production-ready code?

image.png
#

You can toss a return here and avoid nesting with the else

image.png
#

You can probably yeet the entire processing logic to a bunch of separate tasks

image.png
#

These are poor names, get + containsKey can be replaced with a get + null check

image.png
prisma ridge
#

actually wait probably not much, i only used it for (usually one-time) registration of certain items

jagged rock
#

it's not that much

#

basically doubles how long it takes

#

which for a map is.. not much

tidal basin
pine grail
#

or was it microseconds?

#

I forgor

desert ore
#

40ms is really long I believe

jagged rock
pine grail
late smelt
#

That’s nearly a full tick

pine grail
#

yeah checked again

#

it's actually in the nanoseconds

#

my bad

#

~80ns vs ~40ns

tidal basin
jagged rock
#

Yeah

tidal basin
#

okay I'll do that, thanks

tidal basin
#

(and using an event bus and a dispatcher)

#

(please)

#

I've never done a thing like that so I don't really see how to achieve it

prisma ridge
#

my actual Event class is just there if someone wants "typing" otherwise listeners can actually listen to basically anything

#

however say u have an event that is like

class PlayerEvent

class PlayerMoveEvent extends PlayerEvent
class PlayerToggleSprintEvent extends PlayerEvent
``` if u wanted a listener that would handle all PlayerEvents and subtypes, that just isnt possible in my system
tidal basin
#

okay I'll check that thanks !

prisma ridge
#

np, i wouldnt worry about looking in impl.proxied since thats a lot of reflection hackery, just impl.direct can get u started on a basic but solid bus

#

note to self add comments/javadoc to that

tidal basin
#

okay okay

dense pond
dense pond
tidal basin
#

Hello, back again for code review, 1. focusing on this class, I'm getting very bad performances with a lot (30.000+) custom blocks at the removeIf in the ChunkUnloadEvent so I was wondering how I can improve it.

a. I checked for java-collections-performance-time, java-collections-complexity, and how-to-efficiently-performance-remove-many-items to see how I can improve my system but still it doesn't helped much. b. (Should I use a Queue ?)

I've also have done some profilers (here's the most relevant one)

c. I'm also getting a, logical, ConcurrentModificationException when using a synchronized set (that's why I'm using CopyOnWriteArraySet) but I was wondering if this also reduces my performances.

Side notes :

  • I'll work on a readaptation of the Properties system (used like that) because like @jagged rock said, we don't see the point and I would like datas stored in pdc to be accessible as a record or smth like that using a PdcTypeAdapter that I would make
  • I'll also work on the event bus and the typed event system. Because it's a pretty big project and sometimes my optimizations are... well, not that good, if anyone wanna work on the project, I would be very grateful !
jagged rock
#

line 190 can be turned into a guard clause

#

that 30k+ problem can be solved by grouping blocks into chunks and regions and stuff

#

you could use a concurrent collection or just use a mutex

#

Your PropertiesStore class is literally just a pdc wrapper

#

at this point just write a PDCContainer helper class or something

tidal basin
#

or you mean, processing them by chunks ?

#

(ticking them by chunks ?)

tidal basin
tidal basin
tidal basin
jagged rock
tidal basin
#

wouldn't that be more calculations and loops ?

jagged rock
tidal basin
jagged rock
#

tick chunks in parallel if you can joeshrug do you even need to tick all 30k

tidal basin
tidal basin
jagged rock
#

by splitting into chunks you get the benefit of being able to query by region and stuff

tidal basin
#

but still I need to store pdc datas in the block before removing the ItemDisplay, using mfnalex's lib for this
(is that the best way ?)

jagged rock
#

you can, for example, get every player, make a region around the player

#

merge all the regions and only tick the blocks in those regions

tidal basin
jagged rock
#

this is how I do it

#

blocks are only loaded for chunks that are loaded and we don't have 30k+ so eh

tidal basin
#

that's really powerful

#

(developing on spigot since 3 years and I'm still feeling like a big shi)

jagged rock
#

joeshrug I've been at it for like 10

tidal basin
jagged rock
#

9?

#

dunno

#

even then I can think of a dozen ways to improve this code

tidal basin
#

so many years that you don't count anymore 🧓

jagged rock
#

dawg I forgot

tidal basin
jagged rock
#

am hacker

tidal basin
#

noob vs pro vs hacker 😔

#

where's god

jagged rock
#

I have reached level 100 mafia boss status 😔

tidal basin
#

that's impressive

#

again, thanks for all your help ! I'm excited to try all of that and break code and break world and repair and it works and youhouu and then break it again but in the end it doesn't even matter how hard I've tried because it will works so hey that's cool

#

so I'll try all of that in few hours (after sleep basically) and maybe if you want to, bother you again because "🤓*put glasses on* I don't understand how to implement this" or smth like that

#

Thanks ImIllusion for this review !

tidal basin
#

let's do that

tidal basin
#

humm @jagged rock ```java
public <T> void setBlock(int x, int y, int z, T block) {
BlockHolder<?> oldHolder = blocks[key(x, y, z)];
blocks[key(x, y, z)] = new BlockHolder<>(x, y, z, block);

if (oldHolder == null) {
    validCount++;
}

}```

# 
[14:17:59 ERROR]: Could not pass event ChunkLoadEvent to Krimson v1.0
java.lang.ArrayIndexOutOfBoundsException: Index -385 out of bounds for length 4096
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.ChunkSectionBlockContainer.setBlock(ChunkSectionBlockContainer.java:40) ~[Krimson-1.0-1753618654431.jar:?]
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.ChunkBlockContainer.setBlock(ChunkBlockContainer.java:82) ~[Krimson-1.0-1753618654431.jar:?]
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.WorldBlockContainer.setBlock(WorldBlockContainer.java:73) ~[Krimson-1.0-1753618654431.jar:?]
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.GlobalBlockContainer.setBlock(GlobalBlockContainer.java:59) ~[Krimson-1.0-1753618654431.jar:?]
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.CustomBlockTracker.registerBlock(CustomBlockTracker.java:26) ~[Krimson-1.0-1753618654431.jar:?][14:17:59 ERROR]: Could not pass event ChunkLoadEvent to Krimson v1.0
java.lang.ArrayIndexOutOfBoundsException: Index -385 out of bounds for length 4096
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.ChunkSectionBlockContainer.setBlock(ChunkSectionBlockContainer.java:40) ~[Krimson-1.0-1753618654431.jar:?]
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.ChunkBlockContainer.setBlock(ChunkBlockContainer.java:82) ~[Krimson-1.0-1753618654431.jar:?]
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.WorldBlockContainer.setBlock(WorldBlockContainer.java:73) ~[Krimson-1.0-1753618654431.jar:?]
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.GlobalBlockContainer.setBlock(GlobalBlockContainer.java:59) ~[Krimson-1.0-1753618654431.jar:?]
    at Krimson-1.0-1753618654431.jar/ovh.paulem.krimson.regions.CustomBlockTracker.registerBlock(CustomBlockTracker.java:26) ~[Krimson-1.0-1753618654431.jar:?]```
#

should I do Math#abs ?

#

like this ```java
private int key(int x, int y, int z) {
x = x % SIZE;
y = y % SIZE;
z = z % SIZE;

return Math.abs(x + (y * SIZE) + (z * SIZE * SIZE));

}```

jagged rock
#

so yeah your XYZ should always be between 0-15 never negative

#

might need to fix that on my own

tidal basin
#

okay

tidal basin
#

I don't even know why

#

but it's like, incredibly efficient

#

Did this and though it would perform very bad, but looks like it's fire```java
private void tickBlocks() {
lastTickedCount = 0;

for (World world : Bukkit.getWorlds()) {
    WorldBlockContainer worldContainer = getGlobalContainer().getWorldContainer(world);

    if (worldContainer == null) {
        continue;
    }

    for (Player player : world.getPlayers()) {
        for (Chunk chunk : ChunkUtils.getChunksAroundPlayer(player)) {
            if (!chunk.isLoaded()) {
                continue;
            }

            ChunkBlockContainer chunkContainer = worldContainer.getChunkContainer(ChunkKey.fromChunk(chunk));

            if (chunkContainer == null) {
                continue;
            }

            for (BlockHolder<?> block : chunkContainer.getAllBlocks()) {
                if(block.getData() instanceof CustomBlock customBlock) {
                    customBlock.tick();

                    lastTickedCount++;
                }
            }
        }
    }
}

}```

#

even with 30*30*30 custom blocks

#

the client will crash before the server

tidal basin
#

I'll try to store datas in block, and making ItemDisplay packet-based

jagged rock
#

Yeah making them packet-based should fix it

#

crazy how most of the load is actually just getting the profiler and releasing it

#

along with moonrise doing its thing

jagged rock
#

Here's a better solution

# 
public Collection<ChunkKey> getActiveChunks(World world) {
  Set<ChunkKey> result = new HashSet<>();

  for (Player player : world.getPlayers()) {
    for (Chunk chunk : ChunkUtils.getChunksAroundPlayer(player)) {
      if (!chunk.isLoaded()) {
        continue;
      }

      result.add(ChunkKey.fromChunk(chunk));
    }
  }

  return result;
}
# 
private void tickBlocks() {
    this.lastTickedCount = 0;

    for (World world : Bukkit.getWorlds()) {
        WorldBlockContainer worldContainer = getGlobalContainer().getWorldContainer(world);

        if (worldContainer == null) {
            continue;
        }
    }

    for (ChunkKey key : getActiveChunks(world)) {
        ChunkBlockContainer chunkContainer = worldContainer.getChunkContainer(key);

        if (chunkContainer == null) {
            continue;
        }

        for (BlockHolder<?> block : chunkContainer.getAllBlocks()) {
            if (!(block.getData() instanceof CustomBlock customBlock) {
                continue;
            }
      

            customBlock.tick();
            lastTickedCount++;
        }
    }
}
#

this looks so fucking wonky with 2 space indent

tidal basin
tidal basin
#

didn't noticed because I'm... alone

#

(thanks)

tidal basin
#

I saw that many times and I don't see why

jagged rock
#

It's just a thing I do

#

There's no need to KNOW it's a set

#

So I return the most generic version possible

tidal basin
#

oh okay I see

jagged rock
#

It's easier to change in the future

#

fun fact

tidal basin
#

I see, now I know that

jagged rock
#

CoKoC is minikloon

#

even without seeing the account I recognize his code style

tidal basin
#

who is minikloon ?

jagged rock
#

worked at hypixel and cubecraft

#

pioneered minigame development and then came up with slime world format

tidal basin
jagged rock
tidal basin
jagged rock
# 
int[] whatever = {-1, 0, 1};

for (int x : whatever) {
  for (int y : whatever) {
    for (int z : whatever) {

    }
  }
}
#

no one else does this

tidal basin
#

yes

#

seeing this fucked my brain up

jagged rock
tidal basin
jagged rock
#

he's been doing it consistently for 10+ years

tidal basin
#

don't tell him about for-i loops

#

it seems to work very well

#

I'll try packet-based next

#

ImIllusion

#

your system is fire

#

fetching by block, location, whatever, is so powerful instead of interating

#

I was dumb using list

jagged rock
#

could be better but eh

#

if you want to use less ram but more CPU change the ChunkSection thing from an array to a map

tidal basin
jagged rock
#

Something like that

#

if you're fancy you use something like fastutil

#

Int2ObjectOpenHashMap or whatever

tidal basin
#

would fastutil be faster 🤓

jagged rock
#

less autoboxing

tidal basin
#

interesting, thanks !

tidal basin
#

if it's the case, I can do some kind of ArrayOrIntHashMapWrapper to let user choose

jagged rock
#

Or let them decide with some setting

tidal basin
#

Here's what I've done

jagged rock
#

You could pull a hashmap and dynamically change

#

No

#

That bad

tidal basin
#

ah

tidal basin
jagged rock
#

Something like this

#

set the load factor to 0 to always use arrays, set the load factor to 1 to always use maps

#

you can also just use a map directly

#

or an array

#

If you're fancy about it you can also implement a putAll method that pre-expands and yeets it all for a bit more efficiency

tidal basin
#

You're goated

tidal basin
prisma ridge
#

lambdas 😞

pine grail
random yacht
#

currentContainer::set

tidal basin
jagged rock
#

and reworking some of the methods to reuse them

#

only problem I'm seeing is that we can't really do size predictions because overwrites are a thing

pine grail
tidal basin
digital galleon
round fossil
#

lombok 😭

#

holy lombok

#

out of curiosity, why is all ur classes prefixed with Z?

#

the addon stuff looks fine though

#

Something that can be worth thinking about is to not tightly couple LifeStealZ to your class and instead directly pass Logger, ConfigManager etc

#

Nitpicking a little bit, you're arguably doing too much in a generic try-catch-block of Exception, its a bit hard to follow what kind of error you're trying to handle, and from which method invocations (or all of them)?

#

I'd say make the Metadata into a record, but my opinion merely

jagged rock
#

It's the same reason why I prefixed most classes on my skyblock plugin with Skyblock

#

SkyblockPlayer, SkyblockWhatever

desert ore
#

I don't think I ever did that

jagged rock
#

It doesn't really affect my thought process when I'm working on the plugin and it simplifies the thought process of anyone expanding on it

#

I didn't really care to do that at work and now we have a bunch of duplicate class names, especially with anything network related

#

Even in my skyblock plugin I had a bunch of issues when doing item meta because of it

random yacht
#

mfw packages

jagged rock
#

yeah but it's ugly

#

here's some old code

late smelt
junior holly
random yacht
# late smelt HypixelPlayer

I didn't say it was wrong to name things more specifically. I was implying it was wrong to prefix your whole API with some prefix because you don't want it to conflict with some imaginary type that may or may not exist

#

Imagine if everything in Bukkit were prefixed with Bukkit. i.e. BukkitPlayer, BukkitPig, BukkitBlock, BukkitItemStack, because we wanted to let plugins define a Player type without fear of accidentally importing the Bukkit type instead of their own

desert ore
#

imagine having BukkitBukkit

#

lmao

random yacht
#

There is CraftBukkit and it prefixes most of its types with Craft, but honestly that's at least an established practice for the type of API that CraftBukkit implements. Not to mention they're implementation types, not API types that the end user should be referencing anyways

#

My point being, the Z prefix in the user's code above is really unnecessary

#

Also also, you probably don't want the ZEvent working the way that it is. I mean maybe... but probably not. There's a reason Bukkit event types want you to have a static getHandlerList() method and getHandlers() implementation. The way your event hierarchy is structured means that if you call any of your child events, all of its parent event listeners are also getting called cause they share the same event handler

late smelt
#

BukkitStructure, BukkitStructure, and BukkitStructure

digital galleon
#

But thanks, I realy appredciat your input

winged blaze
random yacht
#

I don't think they needed to

#

Their API is independent from Bukkit

jagged rock
#

It's not necessary but it's a nice gesture imo

digital galleon
winged blaze
#

It's not fun when you mix WE and bukkit api

#

And you do that often

random yacht
#

oh no, I have to type an in-line package name

pine grail
junior holly
jagged rock
#

something that I'd improve on is using a pattern I call "delegating" to prevent your round class from becoming a god class

#

crap I just wrote a demo and pressed "new" instead of save fml

#

basically it consists of your regular interface, and a "delegate" interface that extends your interface but has a get method for an instance of it

#

It then adds a default method that calls to the delegate

#

You can inherit multiple delegate interfaces, make a getter for each instance and your class now has a million methods without becoming a god class

#

I then inherit DelegatePlaceholderContainer on every menu, menu slot, menu element and layer

#

meaning I can just do something like

menu.getSlot(13).addPlaceholder(MessagePlaceholder.of("foo", "bar"));
#

@junior holly ^^

#

Sure you can omit the delegate interface entirely and just call getPlaceholderContainer() but that results in a LOT more boilerplate

junior holly
junior holly
digital galleon
#

looks so fucking clean

junior holly
#

The defaults are all very coupled... but to be expected in this context I think

pine grail
#

create a round context object which has the various params inside of RoundImpl

#

this way if you need to pass it around you also get that for free

junior holly
pine grail
junior holly
#

Eh the only heavy loads round deals with is the invocation of stroke handling, ie: disc physics. I'm not too worried about performance and it feels a bit silly to have an extra composition layer when roundImpl is already serving that purpose yk

junior holly
#

Actual impls under /impl and /common

tall thunder
jagged rock
#

Why are your configs static

#

You can reuse the return value of getDataFolder and add some braces to make the code look consistent

image.png
#

keySet + get -> entrySet

image.png
#

nested if statements -> guard clauses

image.png
#

Your IgnoreManager's ignoreList should be private and the manager class should act as one

#

it skips all this tomfoolery

image.png
tall thunder
#

Oh

tall thunder
#

onEnable

jagged rock
#

that's not a reason to use static

tall thunder
#

Oh I mean onDisable

rancid fern
#

Still no reason to use static

tall thunder
#

It says it can’t be referenced from a static context

jagged rock
#

not an excuse

#

?di

rich fogBOT
tall thunder
#

What is that

rancid fern
#

A guide on how to pass data around

#

so you don't static abuse

tall thunder
#

Oh

#

But what is wrong with the config being static

jagged rock
#

read

random yacht
#

TL;DR (but really, please read lol)

  • When something is declared static, it belongs to the class itself. There is only one instance of it
  • When something is not static, it's a member and there is an instance for each instance of its enclosing class
#

At least when you're starting out, try to avoid static unless you're declaring a constant. There's probably a way to accomplish whatever you're doing without it.

#

I do think that DI page is a bit of an information overload for beginners though

rancid fern
random yacht
#

This is maybe a good explanation of how static things work using a visualization

image.png
tall thunder
#

Like this
private IgnoreManager ignoreManager;

public ReplyCommand() {
ignoreManager = new IgnoreManager();
}

tall thunder
#

@jagged rock

junior holly
# 
private IgnoreManager ignoreManager;

public ReplyCommand(IgnoreManager ingmoreManager) {
    this.ignoreManager = ignoreManager;
}```
tall thunder
#

Oh

digital galleon
#

poop

#

yum

arctic totem
#

Hello, I created NMS hologram library for my plugins. What do you thing about it? Is not completed yet. I'm mostly curios what do you think about abstraction and NMS implementation (it should be completed).
https://github.com/Marek2810/PersoLib

I didn't want to do it for multiple versions. I'd like to wait for feedback and then make other versions of minecraft based on that.

Thanks for any feedback!

GitHub
GitHub - Marek2810/PersoLib

Contribute to Marek2810/PersoLib development by creating an account on GitHub.

full dragon
late smelt
#

At least in FakeOpCommand I notice a lot of nesting of conditions

#

Ideally you should make use of early returns to make your code less of a staircase

digital galleon
#

IntelliJ flips nests for you I think

random yacht
#

Eclipse can do it too aPES_Sorry

late smelt
#

I bet it can’t flip your receding hairline though old man!

digital galleon
alpine anvil
#

Josh has a great hairline

sharp epoch
#

I was yak-shaving for the lack of motivation to actually work on the thing I had to work on, and made this class (or well, part of it at least, the other part was just gemini code lol). Do you guys believe it is superfluous: https://gist.github.com/JavierFlores09/995b7db78367be791cfb1da66c7a499a
expected usage is the following:

        this.navigationHandle = ReflectiveHandler.ofField("navigation")
                .from(CopperGolem.class)
                .type(PathNavigation.class)
                .privileged()
                .expect("Could not find 'navigation' field in CopperGolem. Incompatible server version?");

        this.getEntityHandle = ReflectiveHandler.ofMethod("getHandle")
                .fromCraftClass("entity.CraftCopperGolem")
                .type(CopperGolem.class)
                .expect("Could not find 'getHandle' method in CraftCopperGolem. Incompatible server version?");
# 
try {
    var craftbukkitPackage = Bukkit.getServer().getClass().getPackage().getName();
    var lookup = MethodHandles.lookup();
    var copperGolemCbClazz = lookup.findClass(craftbukkitPackage + ".entity.CraftCopperGolem");
    
    var privilegedLookup = MethodHandles.privateLookupIn(CopperGolem.class, lookup);
    
    this.navigationHandle = privilegedLookup.findVarHandle(CopperGolem.class, "navigation", PathNavigation.class);
    this.getEntityHandle = lookup.findVirtual(copperGolemCbClazz, "getHandle", MethodType.methodType(CopperGolem.class));

} catch (ReflectiveOperationException e) {
    throw new RuntimeException("Failed to initialize reflection handles.", e);
}

somewhat equivalent code would be this, I just made that class since I hated how cluttered it looked at a glance

round fossil
#

nice utility, how do you deal w obfuscation, that'd pretty much have to be reliant on some build tool transformator? but also whilst i personally wouldnt mind it, i do think it might be a bit of a nieche thing to over engineer. It does look cluttered but usually its not that big of a deal in those cases where you're writing reflection logic

sharp epoch
#

I am expecting to be writing quite a lot of reflection for this project so I just said eh, why not

sharp epoch
#

I could use jpenilla's reflection-remapper but I don't want to make a bunch of interfaces to get a single method

round fossil
#

i know forge used to have sth for it called ReflectionObfuscationHelper (a class) or sth, think their custom gradle plugin explicitly did some magic during compilation

sharp epoch
#

I am in the process of making a gradle plugin to simplify things like that, I want to go real fancy and have something similar to kotlin multiplatform with @Actual and @Expect annotations but instead of jvm/non-jvm, spigot/paper

#

that being said, it has proven to be harder than I initially thought

round fossil
round fossil
#

i could imagine that

sharp epoch
#

I just believe that right now making plugins for both platforms is kind of a pain in the ass, more so if you want things like testing or running a server with a gradle task, there's a lot of setup for what is usually a very simple plugin

#

I just have a template right now and that works, but now I got a lot of free time so I decided to give gradle API another go, though that never ends well lol

round fossil
#

yea idk

#

i yet till this day have a hard time with gradle dsl and gradle api

#

i suppose thats more of a gradle issue tho

sharp epoch
#

but of course, little of that applies to something niche like minecraft plugins

round fossil
flint kestrel
#

Hey!
I recently created a library called SpigotX to simplify GUI creation, event handling, and command management in Spigot plugin development.
This is my first time publishing a library, so I’m still figuring things out. And currently I’m the only one using it 😅

I’d really appreciate it if you could take a look, try the syntax, and let me know what you think, any feedback, suggestions, or critique would be super helpful!

🔗 GitHub Repository:
https://github.com/AdamTroyan/SpigotX

Thanks in advance! ❤️

muted sentinel
flint kestrel
#

I even added javadoc, I need to update it with all the comments and etc

round fossil
#

favor package names with singular naming

#

try-catch ReflectiveOperationException

#

for reflective operations ^ (over Exception)

#

Good that you cache the regex in validationsutil, u could use jetbrains annotations to describe the pattern in function parameters in said class

#

like @Pattern

#

you use a lot of static, you could arguably instance a lot of stuff here to make it reusable and testable etc

#

use a proper logger and not sysout

#

also SimpleDateFormat is not thread safe (PlaceholderManager, I saw that u were using ConcurrentHashMap, assuming u're accounting for multi-threading)

digital galleon
#

This feels very much like a sonic-screwdriver

#

Some of this just feels weird too:

@Plugin(
    name = "MyPlugin",
    version = "1.0.0",
    description = "Professional plugin using SpigotX"
)

Does this auto-generate the plugin.yml file? Otherwise what is its purpose here?

#

And please, attribute your commits. Everything cannot just be named "Commit"

flint kestrel
digital galleon
flint kestrel
digital galleon
#

🙏 thanks spigot, nice knowing you all, me and xpdz are becoming israeli intelligence operatives.

jade delta
#

🙏 we switching up on our day 1s

viral mantle
digital galleon
round fossil
# viral mantle https://github.com/YesNoBruhbruh/IdolPlugin Any suggestions/criticisms are grea...

I'd avoid using Random in IdolSuperProfitTask cuz its slower, has poor quality in terms of statistics and no equidistribution guarantee, if u need a fast random go for Xoroshiro (xor rotate shift rotate) for example, games tend to default to that algorithm (minecraft doesnt because of reasons I cba to explain)

Prefer Path + Files API over legacy File- tho i think kotlin got some other niceties to work w file io

You are using kotlin, no need to use Consumer, Function etc for ur own things, you have first class functions for example fun x(val y: String -> Int) (y is a first class function)

Don't forget @JvmStatic on things that need it.

I believe the server's plugin manager santizes after ur plugin, no need to cancel tasks (could be wrong though, pls fact check me some1).

Why are you using out String in Command? It can only be array type of String, so no need to care about that + you have declaration site variance compared to java's use site variance in terms of type projections. I'm pretty sure bukkits args from Command#execute(..., args) is String[] (Array<String> in kt).

I'm a bit skeptical to the design of MessagesConfig, I feel like you could engineer it a bit more sophisticatedly where it supports reload() from CustomConfig? (Unless I'm mistaken, you cache it once and then never allow for a cache flush in case of a reload).

viral mantle
viral mantle
storm zephyr
#

i heard if you post here you get a diss track written about you so guh

image.png
digital galleon
#

never heard of empty lines between ur code ffs?

storm zephyr
digital galleon
storm zephyr
hexed agate
#

This channel is for good faith requests for code review, if you're just going to troll go back to #general

cc @round fossil

digital galleon
#

Wasn’t trolling, I think he is. I was advising him not to use vernacular that may be oriented to mock African Americans.

storm zephyr
jagged rock
# storm zephyr i heard if you post here you get a diss track written about you so guh

Constant should be named LIKE_THIS
Static import icky but I'm noticing a bunch of static abuse in general

Add a few empty lines, at least between methods
Bit of a codestyle ick but add braces to your one-liner if statements and ideally nest them over to the next line
Varargs for message replacements are going to be error prone and non-specific. I've got no clue what the method does

Name your variables and methods properly

#

cooldown is a single word

storm zephyr
jagged rock
#

does coolDownHandler.handleCool make me cool

storm zephyr
jagged rock
#

smartass

#

what does playerNull(args[0]) do?

#

All of these questions could be fixed by just properly naming your stuff

storm zephyr
jagged rock
#

but I don't

#

even if I did

#

GString.playerNull

#

what does that tell me

storm zephyr
storm zephyr
jagged rock
#

now you're making me look at more code

#

What if it was named GenericMessages.invalidPlayer(String target)

#

But also sendToGulag as a method should not be responsible for sending the message

#

just for sending the player to gulag

storm zephyr
#

so instead of just importing the strings just use the classname.string?

jagged rock
#

Name the class properly

#

name the method properly

#

And instead of a static import you do the full call

#

Here's an example of something I wrote recently

storm zephyr
jagged rock
#

I'm still seeing a static import for GenericString

#

as well as one-liner if statements that are collapsed

#

if (whatever) someAction()

becomes

if (whatever) {
  someAction();
}
storm zephyr
jagged rock
#

But also your permission strings should just become constants up above

storm zephyr
jagged rock
# 
public class HealCommand extends WhateverCommand {

  private static final String HEAL_SELF_PERMISSION = "bim.bam";
  private static final String HEAL_OTHER_PERMISSION = "bim.bam.boom";

  ...

  @Override
  public boolean handle(CommandSender sender, Command command, String label, String[] args) {
    if (args.length == 0) {
      // Self heal
      if (!(sender instanceof Player player)) {
        // send invalid heal message
        return true;
      }

      if (!sender.hasPermission(HEAL_SELF_PERMISSION)) {
        // send no permission message
        return true;
      }

      this.heal(player);
      // send success message
      return true;
    }

    if (args.length != 1) {
      // send too many args message
      return true;
    }

    String targetName = args[0];
    Player target = Bukkit.getPlayer(targetName); // getPlayerExact if it isn't a paper only method

    if (target == null) {
      // Send invalid target message
      return true;
    }

    String permission = (target == sender) ? HEAL_SELF_PERMISSION : HEAL_OTHER_PERMISSION;

    if (!sender.hasPermission(permission)) {
      ...
      return true;
    }

    heal(sender);
    ...
  }
}
storm zephyr
#

compiled jar went from 14kb to 15kb 😭

jagged rock
#

dawg who cares about kb size

#

oo the plugin at work went from 32mb to 32.4 I'm deadd 💀

storm zephyr
#

i would be dead

#

i like the compacted ness of returning in one line

but anyway for the strings what determines if they are static or not since you said i was abusing static

jagged rock
#

that compact stuff is killing readability

#

I used to be all for it like 8 years ago but then I grew up

#

same for compact method and variables names

#

you're saving like 2 keystrokes and gaining 3 strokes in return

storm zephyr
#

i ot most of it

image.png
jagged rock
#

Good start but you can still make it all look nice

#

Not just the first half

storm zephyr
jagged rock
#

Almost

#

Your msg method still has a bad name

#

Also you could probably split this command into healSelf and healOther methods

storm zephyr
sly jackal
#

does it do a whisper?

#

if not its badly named as I instantly assumed it was a /msg

#

usually the method would be named sendMessage or something

winged blaze
#

Static abuse doesn't exist

sly jackal
#

thats what all abusers say

#

have you tried asking static how it feels?

digital galleon
#

@jade delta banger joke?

sly jackal
#

lmao

#

more like stoic

jade delta
gilded gate
#

https://github.com/Psikuvit/CashClash
I would appreciate the feedback for this ongoing project (code quality and readability).
Note: the classes and methods are documented by AI (for fast development) and modified by me.

GitHub
GitHub - Psikuvit/CashClash

Contribute to Psikuvit/CashClash development by creating an account on GitHub.

hard lantern
#

https://pastes.dev/JRqXrig42E
RepoSpcService#call does not support multiple parameters, so multiple parameters must be wrapped in a data class which might look like

data class Params(val uuid: UUID, val amount: Double)

to set Balance. Also the RpcType implementation gets a little more complex of course.

full code is here http://forge.kentoj.de/Tottori/TottoriNetworkCore

mostly looking for feedback on the API itself. feedback on the implementation is too mcuh to ask for anyways.
thx :)

#

oh and its Kotlin