[MEGA THREAD] Get Your Code Reviewed or Review Others

[woeful pasture]

    public Map<LivingEntity, Monster> getMonsters() {
        return monsters;
    }
``` don't return maps like this, you're giving direct access to your map which is the point of wrapping it in the first place

[jagged rock]

read the basics

#help-development message

[woeful pasture]

tbh for your manager if you're going to make it static, just make it a singleton, Some of these things shouldn't really be put in static methods though. Read through illusions post above

[civic shell]

Liskov Substitution Principle
Classes should be replaceable by their subclasses, without breaking the code. Aim to make your code as generic as possible, and avoid using instanceof in favor of a more polymorphic data structure

I don't understand this.

[jagged rock]

https://www.baeldung.com/java-liskov-substitution-principle

[civic shell]

So we pretty much just want to abstract everything we possibly can ?

[jagged rock]

Yep

[civic shell]

okay..

[round fossil]

That’s a really bad way of describing it

Or wel

Not bad but just

Not useful

[civic shell]

Yeah but i got the gist abstract everything you can, makes things a lot harder for me.

and I did publish more changes, if it looks any better, i removed some static methods, and made my Monster and MonsterType abstract.

[round fossil]

That’s not actually thw point

Abstract everything is a meaningless goal id say

[civic shell]

then what am i to improve on? cause I've just been abstracting my entire library for the past 3 hours.

not done, but getting there

[round fossil]

I think a good starting point is effective java

[civic shell]

What exactly of the basics should I go over again?

naming conventions i know thats a proble, that's something that is unlikely to change.

[round fossil]

There is a book called effective java

You don’t have to read the book

But its content is useful

[night knoll]

in the book it says you can use optionals

but I rarely see people use that in their apis

[round fossil]

Design choice just

Personally i dont like javas optional because its so clumsy and often you will find yourself just unwrapping it sooner or later anyway, additionally its quite limited as its just a null wrapper

[vivid reef]

I really like to use it with functions or callbacks, as you aren't able to use Nullable annotations there

[round fossil]

I mean you can do it like public @NonNull CompletableFuture<@Nullable Object> blah() {…}

[cobalt trellis]

And then there are cases where it is guaranteed null, such as in CompletableFuture<Void>, but I digress

[night knoll]

class FileConfigRepositoryTest {

    static Path configPathTest;
    static ConfigRepository configRepository;

    static {
        try {
            configPathTest = Files.createTempDirectory("config");
        } catch (IOException e) {
            throw new RuntimeException(e);
        }
        configRepository = new FileConfigRepository(configPathTest);
    }

    FileConfigRepositoryTest() throws IOException {
    }

    @Test
    void loadAndRegister() {
        assertDoesNotThrow( () -> configRepository.loadAndRegister(new OutdatedStartUpConfig()) );
        assertDoesNotThrow( () -> configRepository.loadAndRegister(new StartUpConfig()) );
        assertThrows(Exception.class, () -> configRepository.loadAndRegister(new IllegalConfig()) );
        assertThrows(Exception.class, () -> configRepository.loadAndRegister(null));
    }

    @Test
    void get() {
        assertDoesNotThrow( () -> configRepository.get(StartUpConfig.class));
        assertThrows(Exception.class, () -> configRepository.get(IllegalConfig.class));
        assertThrows(Exception.class, () -> configRepository.get(null));
    }

    @Test
    void save() {
        assertThrows(Exception.class, () -> configRepository.save(null));
        assertThrows(Exception.class, () -> configRepository.save(IllegalConfig.class));
        assertDoesNotThrow(() -> configRepository.save(StartUpConfig.class));

        StartUpConfig config = configRepository.get(StartUpConfig.class);
        config.setDiscordAboutMe("TEST");

        assertDoesNotThrow(() -> configRepository.save(StartUpConfig.class));
        assertDoesNotThrow(() -> configRepository.get(StartUpConfig.class));
        assertEquals("TEST", configRepository.get(StartUpConfig.class).getDiscordAboutMe());
    }

}```

first time testing

what do you guys recommend me learning

[round fossil]

I don’t know, are you trying to unit test?

[opaque plover]

        } catch (IOException e) {
            throw new RuntimeException(e);
        }

Why?

FileConfigRepositoryTest() throws IOException {
}

Why IOException and not, say, IllegalStateException?

Other than that, test function naming conventions are usually something like methodName_expectedBehavior_stateTested, e.g. isAdult_AgeLessThan18_False

https://dzone.com/articles/7-popular-unit-test-naming

[round fossil]

Yes well the test code written isn’t really that nice, and its unclear they’re trying to achieve

(I mean apart from the obvious, testing)

[opaque plover]

I didn't look into the actual code, just pointed out what was super obvious to me haha

[round fossil]

But apart from that I can strongly advice to look into the different types of functional testing

[opaque plover]

^

[night knoll]

Alr

[night knoll]

I forgot to delete that

But for the first one it's cause the Files.create

[night knoll]

U recommend any articles ?

[night knoll]

Am I allowed to post code not really related to spigot here

[round fossil]

I mean its for code reviews in general, if it regards other programming related issues then use #help-development

[night knoll]

me.skinnynoonie.rbw.
  config.
    StartUpConfig.class
    Config.annotation
  dao.
    ConfigDAO.interface
    FileConfigDAO.class impl ConfigDAO
  repository.
    ConfigRepository.interface
    ConfigRepositoryImpl.class impl ConfigRepository
  manager.
    ConfigManager.class
  parse.
    DynamicJsonParser.class
  Main.class

public class Main {
    
    public static void main(String[] args) {
        ConfigDAO configDAO = new FileConfigDAO(Path.of("..."));
        ConfigRepository configRepository = new ConfigRepositoryImpl();
        ConfigManager configManager = new ConfigManager(configDAO, configRepository);
        
        try {
            if (!configManager.isSavedInDatabase(StartUpConfig.class)) {
                configManager.registerToCache(new StartUpConfig());
                configManager.saveFromCache(StartUpConfig.class);
                return;
            }

            StartUpConfig startUpConfig = configManager.loadFromDatabase(StartUpConfig.class);
            if (hasNullFields(startUpConfig)) { //using reflection OR making a Config interface with a method like hasNullValues();
                configManager.saveToDatabase(startUpConfig);
                printNullFieldsError(startUpConfig);
                return;
            }
            configManager.registerToCache(startUpConfig);
        } catch (DatabaseException e) {
            e.printStackTrace();
            return;
        }
        
    }
  

Is this structure good or bad

example at the bottom for how I would probably load and register configs

And for multiple configurations, I'll make a list and loop through it

I'll also split this into a bunch of private methods

I made it on my phone so yeah

[frosty grove]

https://github.com/orbyfied/boredpvp/tree/master/boredpvp-core/src/main/java/boredpvp/core/gui gui library

[sly jackal]

why are you using generics, am i missing something?

talking about your factory

[frosty grove]

you call createGUI with a GUIKey<G extends GUI> which finds the factory in the gui manager

so you can swap out inventory gui impl

from like bukkit

to packets

easily

bc they both use InventoryGUI.Key

https://pastes.dev/FwqLbTD4Yf paginated random number shitty example gui

[sly jackal]

the page doesnt format on mobile

?paste

[rich fog]

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

[sly jackal]

but cant you just use an instance of a key to get an inventory

instead of using the class itself

[frosty grove]

wdym

it does

well

[sly jackal]

okay then i dont understand generics enough

[frosty grove]

it gets the implementation for the key

and then creates the gui

so i can swap out my packet inventory gui for a bukkit inventory gui without changing any code

[sly jackal]

well dont they both have different key classes

[frosty grove]

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

[frosty grove]

no

they both operate on the same key type

to produce different implementations

[sly jackal]

okay nvm this code looks to enterprise for me to comment on it xD

thats a long chain of inventory elements lol

[frosty grove]

true its kinda messy but i think its flexible

[sly jackal]

so this is for a gui framework

or just part of some plugin

[frosty grove]

i might make it a framework

[sly jackal]

I assume you wanna reuse this for everything else

yeah

[frosty grove]

but i coded it bc i was bored and needed a gui framework

for my server

[sly jackal]

lol

[frosty grove]

plugins

[sly jackal]

dont we all

[frosty grove]

lmao

idk why but i find it really hard to adapt to existing libraries so i very often just end up reinventing the wheel 9 times for a shitty project

i enjoy it tho

[sly jackal]

yes same

I have made my own gui stuff for my plugin too

not because I had to

but because its fun

[frosty grove]

nice

i had this rlly weird bug with bukkit inventories

where it just didnt update

which is why im now making a packet inventory gui impl

which also has some weird shit going on

bc its 1.8

probably

[sly jackal]

oh yeah

fr

[frosty grove]

https://cdn.discordapp.com/attachments/741875863271899136/1177395718995071006/Lunar_Client_1.8.9_v2.10.12-2346_2023-11-24_00-49-54.mp4?ex=65725a36&is=655fe536&hm=7834b32a668675ddade5bef1133c7881cb0f22b8dad0325152dd237c6cd0bf2f&

[night knoll]

u r not dewier 💀

[frosty grove]

i am dwieer

what are u talking abou

t

[night knoll]

i cant believe dewier is mentioned in a spigot dev server 💀

[frosty grove]

oh shit u play rbw too

nice

im bad and i dont play so nice 600 elo

[night knoll]

I looked at ur code and it looks kinda nice

(I didnt rly process just skimmed)

[frosty grove]

thx

[night knoll]

but idk if theres a convention

[frosty grove]

true

[night knoll]

but do default methods in an interface go on the bottom

[frosty grove]

ig that could work i like to keep the default implementations close to the abstract methods themselves tho

its more i realized my method names are all over the place

[night knoll]

idk it just looks so weird

[frosty grove]

i do it a lot idk

ig i see what you mean yeah

[night knoll]

oh its prob cause u were not consisent with your doc format

[frosty grove]

its nice when coding and reading it with the intent of analyzing it tho

[frosty grove]

yeah i feel for methods like that u can just view the impl

its kinda logical what it does

[night knoll]

nah i think that's bad when u can view the impl

[frosty grove]

i might add docs later but its not really necessary for myself to get the intent

[frosty grove]

theyre really not needed tho just helper methods

[night knoll]

oh ur talking abt the default

[frosty grove]

so it's not really part of the gui concept

yeah

[night knoll]

nah I do that too sometimes

ik what u mean

[frosty grove]

oh k yh

[stone dagger]

This is still in development tho: https://github.com/aparx/bufig-library

[round fossil]

You’re not using reflections though, you seem to use reflection

[night knoll]

https://github.com/skinnynoonie/NoonieManagement/blob/master/src/main/java/me/skinnynoonie/nooniemanagement/database/ManagementDatabase.java

is this a good or bad way to handle a database

I made it a while ago

[sly jackal]

I think the naming is a little weird

I assume impl stands for implementation

but its an interface

[pine grail]

^

[night knoll]

That's just me copying luckperms naming

[sly jackal]

well its shitty xD

[cobalt trellis]

Just prefix an I and problem solved

[stone dagger]

I am a bit confused with the way it's structured since it's self recursive

Is that after a specific design pattern?

[night knoll]

it provides completable future

[stone dagger]

I see

[stone dagger]

But is there any specific reason you don't directly define it within the implementation tho?

[night knoll]

yes

there is

Single responsibility principle

[stone dagger]

Oh wait what

I was confused for a second, your Impl class is not actually the implementation?

[stone dagger]

Yeah I get that, but I thought Impl was a class extending the management class thus I was perplex lmao

[night knoll]

Oh

[stone dagger]

In the end I'm just confused by the naming scheme

[night knoll]

I thought u read a little bit of thr conversation

[stone dagger]

Yeah I skipped through it must've missed a word

Yeah it is kinda weird since another class of yours which is suffixed with Impl is an actual implementation haha so kinda confusing

[sly jackal]

I think you just need to rename managementDatabase to managementDatabaseAsync and remove impl from the interface or something

[night knoll]

This was a while ago so

would

ManagementDatabase - interface
ManagementDatabaseManager

Make more sense

[sly jackal]

yes but managementdatabasemanager is a disaster name too

[round fossil]

lol

[night knoll]

ManagementDatabase should be database type like local, mysql, sqlite.

"Manager" Does not explain what it does

[night knoll]

then would Async work

[woeful pasture]

you'll probably be fine with DatabaseManager async should be implied via CompletelableFuture

[round fossil]

Its called humble object principle, (i mean it overlaps with SRP)

[sly jackal]

or calling it a wrapper would also work I guess

since you are just wrapping your database interface into completables

[round fossil]

Yes but wrapper/delegation is a more general term, we call things like adapter, proxy etc to be more specific s.t. one can address more specific design points

[sly jackal]

right I guess so

what would you call it

[round fossil]

As said one of the principles when you come to multithreading is called humble objects, not to be confused with elegant objects.

Humble objects means we try to isolate the general logic from the concurrency logic, meaning we try to decouple the 2, the best we can ofc. That should make it easier to test the general logic and the concurrency logic. Additionally it should make the design of things a bit more niche, which can be a big deal in some situations.

[sly jackal]

okay that makes sense

[round fossil]

yeah and you know like its really awkward to sometimes properly write and test concurrent/multithreaded code

[sly jackal]

yes that I know haha

[night knoll]

would this even be thread safe tho

like the way I did it

[round fossil]

depends on what u're doing concurrently lol

the principle is there to help u, wont automatically solve ur problems, sadly...

[night knoll]

would it be ok to do stuff with the future in the storage's thread

[frosty grove]

https://github.com/slatepowered/inset

renamed it from "store" to "inset"

less boring i hope

[frosty grove]

https://pastes.dev/vAGXftAn5p

exampel

[round fossil]

just looked at the first class I could access, little bit curious why u ignore interruptedexception like that and just return false

[frosty grove]

idk bc it didnt await the operations

thats the point of the return value

[round fossil]

well its indeterministic in that case

like u actually dont know if termination is succesful or not if one of those is thrown

[frosty grove]

its safer to assume it didnt

basically if it returns false u might want to sleep or call it again

cant hurt

[round fossil]

well ur api would be lying in some cases then

i mean its an odd way of handling it

thats all I wanted to point out

[frosty grove]

if you throw an exception itll just end the program and youll lose data

[round fossil]

id be surprised at least if I were to read this code in some api I used

[frosty grove]

oh nvm its checked

ig

[round fossil]

otherwise, good stuff

I dont like lombok, but thats justme yk :P

[frosty grove]

thx, dont have records in java 8 so i thought id use lombok

kinda like it now

[round fossil]

Ah fair, anyway the two standard ways of dealing with interrupted exception esp if u write an api where u’re not the end consumer is to either

just propagate the exception upwards (it can be unchecked, just that u re throw and pass the interrupted exception as the cause)

second is to interrupt the current thread

[frosty grove]

ah k

thanks

[night knoll]

https://github.com/KarmaDeb/Channeling
https://github.com/KarmaDeb/NettyChanneling/tree/master
[WIP] A simple netty API which aims to allow secure communication directly between clients

[frosty grove]

looks cool, dont like the interface names prefixed with I, also goes against java convention but yeah overall nice project, couldnt find much implementation code tho with i assume is bc its a wip

also why are you using a custom future class instead of something like CompletableFuture

and if the execution behind a future happens on another thread from the listeners you will need to make your ConnectionFuture and other implementations thread-safe

which CompletableFuture already does by default

[night knoll]

Idk if it's against java conventions

[round fossil]

That future class is so bad yeah lol

Just go with CF

[stone dagger]

I went through the sources a bit and there's a small things I would change that could have a good amount of impact to your overall source quality and performance, such as:

• actually preallocating memory when the final size is known
• using runtime and static assertions to ensure preconditions
• only documenting when it's needed, and then actually documenting what a method or class is supposed to do, what the implementation details are etc.

[frosty grove]

this on my project or their project?

[stone dagger]

wrong tag sorry

[frosty grove]

oh ok

[stone dagger]

@night knoll

[frosty grove]

noone does it so its at the very least not a java convention

[round fossil]

It is against

Because in java we have extends and implements to tell them apart

[frosty grove]

yeah

i alr thought

[round fossil]

IDEs also help to distinguish the 2

[round fossil]

I mean what worries me the most is how the whole thing aint thread safe while the project is written to sustain a highly concurrent environment (or well a subsequence of using netty)

[frosty grove]

https://github.com/slatepowered/inset/blob/master/inset-core/src/main/java/slatepowered/inset/query/FoundItem.java

i dont really like the fact that its mutable and can be 'qualified'/adopted to/by a datastore but i also dont want to create 2 mirror objects per item returned by a findAll query

for context, the database abstraction uses this class to abstract a result in a result set/iterable but the datastore component also uses the same instances for datastore-related functionality which are not technically related, i decided against seperating them though bc of memory/gc usage

[night knoll]

If im going to have a huge utils api that mainly generates locations, how should I structure it

for example:

SphericalLocationCalculator
LineLocationCalculator

etc.

I just feel like it is too different to abstract them into one

cause one calculator can have
calc(radius), then the line one can also have calc(destinationLocation) etc

and do I even use objects or should I just make it all static lol

[round fossil]

Always nice to avoid static for future extendability, maintainability etc

Esp if its an api

[woeful pasture]

meh as far as utility methods go which is what I'm pretty sure is being explained here static should be used. You can probably throw them all into one class too e.g. LocationCalculator class or LocationUtils class

[round fossil]

Depends

I mean sure the concretion can be a static method if its pure or well, “stateless”, but then again depending on the structure of things it may be highly appropriate to not let the consumer directly have to call that impl, or general concretion. Let’s say you define some sort of polymorphic api where a certain subtype does x thing stateless and purely, but another subtype might be doing x thing imperatively

[woeful pasture]

ig it depends what would be going on inside of the method. What does calc do? Does it even need to be wrapped in the objects are probably questions you should ask yourself before doing something along those lines

[round fossil]

I mean its always nice to go high level with some layer of abstraction if you’re defining some api that’s specific to ur plugin or module w/e, different if it were to be like a geometry library where the shape, formuls and measurement of things are well known

[night knoll]

Let's say

like a LocationUtils.calcSphere(radius, increments, blah blah) returns a list of locations

this brings my next question of wether I should use vectors

The purpose of this api is to display particles in cool shapes and effects

[alpine anvil]

the sphere would probably return locations if its used at runtime or vectors if its calculated once

[night knoll]

Idk

it's just very yk like

Situational

like for example, I could cache a list of vectors that form a sphere

But then, for something like a line with a start and end

I can't cache that because two variables change

[jagged rock]

I'm honestly thinking of a shape system first

So you could have shapes surch as cylinders, capsules, triangles, cubes

From there you can draw particles, generate random positions

[stone dagger]

yup agreed

it's the perfect OOP example haha

Also I'd primarily focus on creating iterators or streams, instead of creating lists just because of memory overhead (which is important with many particles!)

[jagged rock]

Particles can be done async so CPU is not the issue but yeah

You can probably render the whole thing with one object

[stone dagger]

Yeah CPU ain't the issue, but memory is with thousands of particles especially in Minecraft

[jagged rock]

I've experienced bandwidth issues

as well

Mostly just ppl at work sending 80k particles every tick and accidentally DOS'ing the player

[stone dagger]

Lmao

I mean that's when batching comes in handy

But for that you manually have to write out the netty channels I think

[jagged rock]

Eh, the problem wasn't batching

The problem was that we were sending 100mbps in particles

[stone dagger]

What

Why's that 😂

[jagged rock]

The sheer volume

Pretty sure my solution for that was just using a lossy location hash and culling particles

[stone dagger]

Ah yeah makes sense

But why so many particles

Per tick

[jagged rock]

we had a vfx guy ¯_(ツ)_/¯

[stone dagger]

Dfq

[jagged rock]

And we had them split up in frames

But we were rendering the whole frame rather than the positions that changed

[stone dagger]

... with particles

Damn

[night knoll]

What about a single line

I feel like if I'm going to display lines it's going to be a throw away object each time

[jagged rock]

It's still a shape

[frosty grove]

i think the GC will collect those temporary objects pretty quickly/efficiently

besides, location, vector, list, etc are all temporary objects as well so dont think it would matter much

[round fossil]

Depends on gc implementation but essentially yes

[cobalt trellis]

It only is an issue once you allocate millions of objects a second, but chances are you don't.

[sly jackal]

if you do you have other problems probably

[night knoll]

Where do you guys recommend I learn how the compiler, gc, etc all works

[cobalt trellis]

Chances are you don't need to know how the JIT compiler, GC, etc works. But fyi this is offtopic and more belongs in #help-development or the general channels

[round fossil]

Yeah as geo said, gc and jvm stuff is a deep rabbit hole

[night knoll]

public interface ShapeTemplate {
    List<Vector> getPositions();
}
public class LineTemplate implements ShapeTemplate {
    public LineTemplate(Vector start, Vector end, int maxIntervals) {
    }
    @Override
    public List<Vector> getPositions() {}
}
``` is this what you guys meant

i could also rename to calcualtePositions

[stone dagger]

you might want to consider using iterators / streams over lists to have less memory allocation when there's no demand

it's a good practice I would say, cause with streams the caller can decide what terminating action they want to execute (as in collecting it into a list or iterating over it [=> less memory overhead for many objects])

but it'll depend on the size of your list, intervals etc. and what your target environment is like. Tiny (< 100 entries) lists shouldn't ever be a problem.

[pine grail]

I'd just do getNextPosition(Vector)

which sets the position in the vector

max efficiency

[night knoll]

hm

well then dont I have to store like current states

[pine grail]

you only have to store x y z

but that's like 48 bytes + created object size instead of creating many short-lived objects

like

24 + object address size + padding per vector (3 vectors)
24 bytes for the last xyz
4 bytes for max intervals

[night knoll]

well most of these are probably going to be async

so idk if I really need to be doing all that

[frosty grove]

is a vector mutable

[pine grail]

yes

[pine grail]

ah

[jagged rock]

Still has a memory footprint

public interface Shape2D {
  Iterator<Vector> getPositions();
}

type deal

[stone dagger]

yeah, but it's more efficient because the JVM doesn't need to find N consecutive free positions in memory to allocate an array (... of pointers I should add)

[stone dagger]

But this is the best implementation, what you can do use use a custom iterator that internally modifies the vector and returns it (only works for sync code)

For async code you create new vectors and it should be fine

[frosty grove]

or if order doesnt matter divide into n sections and allocate n mutable vectors for those sections for n number of threads and then have each thread have its own iterator for a different section

[viral mantle]

optimization to the max

[night knoll]

so yall mean smthing like this ig?

i still think the list thing is nice tho

cause it basically does all the calculations first and then you can do whatever

[stone dagger]

you could, but just use the Iterator interface

And use a method within ShapeTemplate that returns that iterator

No need for a reset if you just return a new iterator

[stone dagger]

Again, if you have a method that returns an iterator and one that returns a stream you can easily collect the iterator into a list if the caller wants that

But imo no need to force the list behaviour for when it's not needed

It's just making things more flexible

Just a warning for when you go with the mutable vector implementation: clone the vector each iteration before collecting through e.g.: shape.getShapeStream().map(Vector::clone).collect(Collectors.toList());

[round fossil]

Also remember streams are expensive, so only use it if it doesn’t impact too much, else go with a normal loop

[night knoll]

I decided to use the list impl

[stone dagger]

Streams ain't that expensive, unless you talk memory overhead then yeah totally. But streams don't allocate something new or execute anything until your termination is called, thus should be O(n) when collecting

[stone dagger]

understandable, makes things easier to implement and potentially to maintain if you have a lot of shapes

[round fossil]

Yes but its exactly that, there is mem overhead and much more ops for every n, again, time complexity just gives you a hint about an algorithms scalability, a good example is merge sort, at the smaller sublists you rarely divide it again, but just sort the sublists even tho it might be O(n) at that point using some other sorting algo

[stone dagger]

yeah I totally agree, it's not that I disagree it's just that the memory overhead of streams won't have a really noticable impact on a plugin. I was just talking from an API standpoint, whereas myself as a library author I would not force a list behaviour but rather would provide options to stream and iterate (separately, exactly because streams are more memory intensive, so it's up to the caller's decision)

the nice thing about this is, it gives flexibility to future contributors and other external dependants. If you notice a bottleneck with your streams, then go ahead and create a separate method for collecting using the iterator over the stream. It just comes down to premature optimization imho (at least for a minecraft plugin).

[round fossil]

well, yes but at the end even on lists u do have the stream conversion api, more so you also if you store a vector of elements you usually do that in terms of some array or some collection data structure in which streams are fine as they are api on those interfaces

[stone dagger]

yes but at the end even on lists u do have the stream conversion api
Yeah true, but it is more about giving the enduser the ability to avoid unnecessary extra allocation of a list when you as the caller just want to iterate over it and maybe filter something, it's a minute detail that I think leverages overall API code quality

[cobalt trellis]

Wishing to reduce allocations is actually a very niche API design concern

[round fossil]

no I mean u store it as a list anyway?

like the impl has an underlying list it relies on (or maybe array) but what I said earlier still holds

and also allocating just a list can actually be way less expensive than to invoke the ReferencePipeline

[round fossil]

this in fact is extremely fine

u have an underlying list and u just return an immutable view on getPositions()

List::stream still works fine

(and in the case of an array, u just use Arrays::stream)

[stone dagger]

Erm, do you have any reference to this? Afaik you supply it something iterable, such as a spliterator. Nothing happens until you do your terminal action. As soon as you invoke your terminal action, the elements are gone through 1 by 1 without allocating new memory. It's like doing a loop - but with more actions and evaluation happening.

[stone dagger]

yup, like I said very minute (as in very tiny) detail. Nothing that is necessarily required for small APIs

[round fossil]

its a redundant detail tho

it makes sense if u're the one impelementing collection or iterable

but they're not

so

[stone dagger]

I mean that's what I've been talking about

If I was to implement it, it would be a custom iterator without a list

[round fossil]

this is perfectly fine tho?

[stone dagger]

If you supply it a list, then yeah sure then it makes no sense to add an additional stream method ^^

[round fossil]

maybe they dont want to commit to the list or iterable api

[stone dagger]

yeah of course

It is totally valid

[round fossil]

i meant that in regard to his impl

[stone dagger]

But if you scroll up in the conversion I've been like talking about how we would implement this. But this thread is a few days old already so nws haha

[round fossil]

not the ReferencePipeline impl

[stone dagger]

Yeah I was confused for a sec there

Classic misunderstanding here

[round fossil]

yeah its fair, tho I think its nieche having a lose interface that is just designed to encourage composition if u want a replica of the internal list structure

like he does rn

[stone dagger]

yeah

But it happens

[round fossil]

Yeah

[night knoll]

It can't possibly be that deep

[cobalt trellis]

what I was going at is that it is so niche that it even is irrelevant

[stone dagger]

I see where you are coming from but I don't think it's totally irrelevant. It depends on your use-case.

For most plugins it may be, yeah

But I was like kinda talking outside the scope of just plugins and more on general, for how I would do it. I don't wanna force anything onto anyone ofc haha

[cobalt trellis]

I personally have been making stuff with 0 allocation, but it only is worth it if you are going to call it thousands of times to millions of times a second. This plainly will nearly never happen.

However, such APIs ought not be designed before the allocation was actively identified as an issue - having it be "hm, performance - am I rite?" is a noble concern, but will result in convoluted or even incomprehensible APIs that require a lot of documentation and may even be error-prone if used in a way that wasn't intended.

For example, designing an API to effectively get rid of the overhead of Iterators can result in complications with not being able to use foreach loops (See Java's XML API). And if you try to be clever and reuse the iterator you may get problems in concurrent environments or even in nested loops.

There is a good reason you probably never used the Enumeration interface, or if you did it wasn't because you wanted to.

[stone dagger]

Erm that's why I suggested to have more than one method that just returns an iterator. Such as a stream, implement iterable yourself etc. Minecraft, as an example, also uses the iterator principle a lot in their source code for collisions, entities and even for ints that represent a 3D range of blocks and what not. They're assured to be called a thousand times a second. When you design a general purpose API you should cover a certain spectrum of environments. In this case servers with a lot of memory and not a lot of memory. And all this is not just about performance, its about the consistency and design patterns you want to use (consistently).

Most plugins may not need this detail, but providing it opens a world to people who want to use it. Code reusability and easy access to the object are now simplified. Again, this is an opinion of mine since I've dealt with a ton of APIs before and is a consideration I would want from a library (or specifically a framework if you build one) - this is out of the context of just Minecraft

But it's totally valid if you are disagreeing with my opinion, it comes down to scalability and usability in the end. Complexity also comes with a little cost, so sometimes a more simplistic approach is enough 👍

[round fossil]

Iterable is also implemented mainly because of the syntactic suger u get w it

then yes, nicely enough ifu rly want u can always ise StreamSupport::stream and pass the spliterator from Iterable::spliterator

which is just, yess... streams!

[stone dagger]

😆👍

[night knoll]

Yeah I just ended up doing the

getNext(Location location) thing

Cause working with a list of vectors was rly awkward

[stone dagger]

Honestly that's when you don't use getNext but use an iterator instead

Cause that definition of getNext is literally an iterator, so you should use the right interface

[night knoll]

what interface

has getNext(T thing)

[sly jackal]

probably Iterable

[stone dagger]

Iterator

has next()

getNext() is basically the equivalent

  public Iterator<Vector> getPathIterator() {
    return new Iterator<>() {

      @Override
      public boolean hasNext() {
        /* Check if we can supply another item */
      }

      @Override
      public Vector next() {
        /* Calculations (probably using a cursor) */
      }
    };
  }

I'd probably not use an anonymous iterator, but a class that implements iterator

[round fossil]

That’s fine

[stone dagger]

so for the ShapeTemplate thing I probably would've done this and implementations might override:

public interface ShapeTemplate {
  
  Iterator<Vector> getPathIterator();
  
  Stream<Vector> getPathStream();
  
  default List<Vector> getPathList() {
    // use this if you want to avoid reference pipeline
    ArrayList<Vector> vectors = new ArrayList<>(); 
    // ^ possible optimization: preallocate instead of default initial capacity of 10
    Iterator<Vector> pathIterator = getPathIterator();
    while (pathIterator.hasNext())
      vectors.add(pathIterator.next());
    return vectors; 
  }
  
}

[jagged rock]

I'd do something to precompile that list

[stone dagger]

i mean yeah, that's what I meant with preallocation 🙂

[jagged rock]

Not just the initial capacity but the entire list itself

[stone dagger]

yeah if your shape is not mutable

[jagged rock]

I wonder if anyone has ever used an iterator to read byte arrays

[stone dagger]

possibly

i mean

foreach is nothing else than looping through an iterator

[stone dagger]

btw. a code improvement might be to extend this with Iterable, so you have iterator() as a method and can easily use a foreach upon the shape template itself

[round fossil]

I’d either change getPathIterator() to return an iterable (and refactor the name), or yeah, derive Iterable directly

that syntactic sugar is rly nice

[stone dagger]

yup

Doing

Circle circle = new Circle(/* ... */); // implements ShapeTemplate
for (Vector position : circle)
  // ...

is just really nice I think

[round fossil]

Yeah altho for circle you probably represent it with a 1d vector as the radius and then one 3d vector for origin location of it

[stone dagger]

yeah

think of a Sphere then :3

[round fossil]

Not much different tho? :>

I mean if you want it to have a weird shape or sth then yeah mayhaps

The shapetemplate interface is rather poor anyway unless the author added more stuff to it

[night knoll]

so is it ok

if I make like

a new reference to a location every time

[night knoll]

i love this idea i cant lie

do you think I should have each vector as the next step, or actual position?

so
Positions: 0, 0, 0 -> 0, 2, 0 -> 0, 4, 0 ...
Steps: 0, 0, 0 -> 0, 2, 0 -> 0, 2, 0 ...

[round fossil]

Position

[night knoll]

so it's alr if I use .clone() a bunch of times for my starting pos right

[round fossil]

Well avoid clone

If you can, go with the prototype design pattern instead

[night knoll]

why?

aren't they the same thing

[woeful pasture]

Clone in Java is so weird I wish they'd improve it. No clone is not the same thing. I couldn't specify how the impenetation of clone works but their is quite a distinction

[stone dagger]

Nah you can use clone, it is recommended to be avoided by most people due to it's unpreditability and way it works underneath. But it is totally fine since we know clone exists on vector and only contains primitives. In some ways clone is even more efficient than allocating a new object, since the memory block is simply copied without the invocation of the constructor

(that's why it always be a shallow copy)

I personally think clone should be avoided most of the time, but when you want to modify mutable instances, without actually modifying them (thus copying them), clone is totally fine as long as you ensure preconditions

@night knoll

[round fossil]

the entire semantic design around Cloneable and clone() sucks

[stone dagger]

It does indeed

But Bukkit uses it, so we kinda should play along I think

[round fossil]

i dont think thats a good approach, like, itemstack suffered terribly from all the clone calls (as an example)

i mean sure if you need to clone something and said class supports clone(), but I generally would avoid it

[night knoll]

Ok so...

[night knoll]

goning back to this

Circle circle = new Circle(/* ... */); // implements ShapeTemplate
for (Vector position : circle)
  Location newLoc = start.clone().add(position);

I would need to do this right

[stone dagger]

yep, if you represent each vector as an offset

A more efficient way would probably be a mutable vector within the iterator. The iterator already contains a copy of the center, and it's coords, which is now only mutated with each next() invocation without the clone call

This would perfectly work for synchronous code

[stone dagger]

But this is probably more fail safe and future proof. Also you can "precompile" a list of vectors if you really want that

[night knoll]

for the getPathList() should I make an AbstractShapeTemplate for that or nah

[stone dagger]

Depends on what you want to achieve, what your conditions are (is a shape depending on a location? Etc.)

If you want to precompile a list then yeah

[night knoll]

nah for like

like

encap

sylism

[stone dagger]

You would need to memoize the result of getPathList after the first invocstion

[night knoll]

or however u spell that word

kinda like

List -> AbstractList -> impl

[stone dagger]

encapsulation?

[night knoll]

for this structure

[stone dagger]

As an example

[night knoll]

oh wait

[stone dagger]

Mhn I mean yeah

[night knoll]

correct me if im wrong

but how java does that list abstractlist arraylist thing, is it makes arraylist impl all the complicated methods

[stone dagger]

But you could also just use default unless you rely on attributes (=> further implementation)

[night knoll]

and it fills in for the "simple" methods

[stone dagger]

AbstractList extends AbstractCollection

It contains stuff like addAll

remove all and stuff

single lookup and mutations are done by the farthest non abstract implementation

Idk if that's necessary for ShapeTemplate

[night knoll]

okie dokie

[night knoll]

public interface ShapeTemplate extends Iterator<Vector> {
}```

```java
public final class ConcreteLine implements ShapeTemplate {

    private final int totalIterations;
    private int currentIteration;

    private final double incrementX;
    private final double incrementY;
    private final double incrementZ;

    public ConcreteLine(@NotNull Location start, @NotNull Location end, double incrementLength) {
        Preconditions.checkNotNull(start, "Parameter start is null.");
        Preconditions.checkNotNull(end, "Parameter end is null.");
        Preconditions.checkState(incrementLength > 0, "Parameter incrementLength must be greater than 0.");
        double displacementLength = end.length();
        this.totalIterations = (int) Math.floor(displacementLength / incrementLength);
        this.incrementX = end.getX() / displacementLength * incrementLength;
        this.incrementY = end.getY() / displacementLength * incrementLength;
        this.incrementZ = end.getZ() / displacementLength * incrementLength;
        this.currentIteration = 0;
    }

    @Override
    public boolean hasNext() {
        return this.currentIteration < totalIterations;
    }

    @Override
    public Vector next() {
        if (!this.hasNext()) {
            throw new NoSuchElementException();
        }

        Vector nextPosition = new Vector();
        nextPosition.setX(this.incrementX * this.currentIteration);
        nextPosition.setY(this.incrementY * this.currentIteration);
        nextPosition.setZ(this.incrementZ * this.currentIteration);

        this.currentIteration++;
        return nextPosition;
    }

}```

is that better or nah

or

public interface ShapeTemplate {

    Iterator<Vector> getPositionIterator();

}```

public final class ConcreteLine implements ShapeTemplate {

    private final Vector endVector;
    private final double incrementLength;

    public ConcreteLine(@NotNull Location start, @NotNull Location end, double incrementLength) {
        Preconditions.checkNotNull(start, "Parameter start is null.");
        Preconditions.checkNotNull(end, "Parameter end is null.");
        Preconditions.checkState(incrementLength > 0, "Parameter incrementLength must be greater than 0.");
        this.endVector = start.clone().subtract(end).toVector();
        this.incrementLength = incrementLength;
    }

    @Override
    public Iterator<Vector> getPositionIterator() {
        return new ConcreteLinePositionIterator(this.endVector, this.incrementLength);
    }```

    private static class ConcreteLinePositionIterator implements Iterator<Vector> {
        private final int totalIterations;
        private int currentIteration;

        private final double incrementX;
        private final double incrementY;
        private final double incrementZ;

        private ConcreteLinePositionIterator(Vector end, double incrementLength) {
            double displacementLength = end.length();
            this.totalIterations = (int) Math.floor(displacementLength / incrementLength);
            this.incrementX = end.getX() / displacementLength * incrementLength;
            this.incrementY = end.getY() / displacementLength * incrementLength;
            this.incrementZ = end.getZ() / displacementLength * incrementLength;
            this.currentIteration = 0;
        }

        @Override
        public boolean hasNext() {
            return this.currentIteration < totalIterations;
        }

        @Override
        public Vector next() {
            if (!this.hasNext()) {
                throw new NoSuchElementException();
            }

            Vector nextPosition = new Vector();
            nextPosition.setX(this.incrementX * this.currentIteration);
            nextPosition.setY(this.incrementY * this.currentIteration);
            nextPosition.setZ(this.incrementZ * this.currentIteration);

            this.currentIteration++;
            return nextPosition;
        }
    }

}```

idk why the class is final lol

let me change that

oh that's cause it was originally a record

[stone dagger]

Both is valid, but you can simply use a mutable vector. So you allocate a Vector once as a private attribute that you modify and return with each next()

But this might mess with synchronicity so it's up to you

Btw: I would probably go with the second variant and make the ShapeTemplate iterable. I would add more methods to the ShapeTemplate, such as streams, lists, and other stuff. Also you would always have to reallocate a ShapeTemplate if it is an iterator, by design, which doesn't quite fit with the meaning of your class. Also you'd force that behavior down the inheritance tree, which ain't that great

[night knoll]

Alr

[jagged rock]

I decided to work on a similar thing for fun

And am doing some cursed things with interfaces

Here's an idea :)

[stone dagger]

This is too scuffed lmao

[night knoll]

Copy cat

[stone dagger]

on-topic, I basically just something similiar from your concrete line thing:
https://github.com/aparx/skywarz/blob/master/src/main/java/io/github/aparx/skywarz/game/item/items/idle/kit/KitInventory.java#L211
https://github.com/aparx/skywarz/blob/master/src/main/java/io/github/aparx/skywarz/game/inventory/InventoryPositionInterpolator.java
@night knoll

[night knoll]

i cant acess skinnynoonie @stone dagger but that's interesting

[stone dagger]

yeah it's all just syntactic sugar

even tho what i made above isn't quite optimized, a few object allocations due to immutability but that's okay for this application until I see a bottleneck

[pine grail]

why an iterator though?

couldn't you just not make a new object

and use a for-loop in the object itself

basically making a fancy wrapper for a for loop

[stone dagger]

you could yeah, but the iterator won't make much of a difference in the end, also I will probably utilize the iterator directly in the future, so I already have the implementation for that

when I go over code review in the stable release I might take a look what to improve upon, and might optimize this, but for now this is fine i guess

[pine grail]

you'd be surprised how much difference "little"/"cheap" objects can cause

https://github.com/malte0811/FerriteCore/blob/1.20.0/summary.md

Look at #1, just moving the way that the optional is gotten reuced ram usage by ~100MB

[rancid fern]

Well that's because they're stored for a long time

That mod doesn't remove them it just makes sure they're gc'd

[pine grail]

Yes, but still, ~100MB were just in the size of the Optional class

[pine grail]

yep yep I know

[rancid fern]

That will happen if you store millions of objects

[stone dagger]

i can allocate 100 mb of objects and still have 100 mb allocated

but i never allocate 100 mb of objects for LTS

because that would be millions

[stone dagger]

^^ this

[night knoll]

Is there a reason why java's framework uses default instead of implementing it in an abstract class?

And their static constants aren't at the top?

In the class HashSet, they extend AbstractSet, but also impl Set again

[round fossil]

Which static constants are we talking about?

But yes, because default methods from interfaces gives behavior inheritance

Which is fine

If its in an abstract class it pretty much becomes state inheritance (almost) which is basically implementation inheritance

[round fossil]

Well that’s just explicitness

But yeah then also remember AbstractSet AbstractCollection etc are meant to simply be skeletal implementations

[night knoll]

@round fossil

What is state inheritance

And behavior inheritance

[round fossil]

behavior inheritance is when classes inherit methods (like implementing an interface with default methods)

state inheritance is when classes inherit variables as well

like extending abstract classes or classes

then the subclass inherits the state of the superclas

[night knoll]

Alright well by how the framework is structured, no variables are being inherited

[round fossil]

exactly

which is called a skeletal implementation

[night knoll]

I'm still confused lol

[round fossil]

about what?

[night knoll]

why they don't just put the method into the abstract

Skeleton impll

[round fossil]

because multiple inheritance

[night knoll]

wdym

[round fossil]

a class can derive multiple interfaces

because interfaces only allow type and behavior inheritance

[night knoll]

Oh shi wait

[round fossil]

so thats why its often preferred to stick with an interface

[night knoll]

now I see

Kind of

[round fossil]

the reason AbstractSet is an abstract class is because its a skeletal implementation so extending it also implies u commit to it strictly

:)

well its a bit messy so its fine

[night knoll]

@round fossil

Is there a specific format convention for the framework

[round fossil]

well not rly

[night knoll]

like I'm seeing formats I've never seen before in different classes

[round fossil]

which classes?

[night knoll]

    public static <T> 
     int binarySearch(List<? extends Comparable<? super T>> list, T key) { 
         if (list instanceof RandomAccess || list.size()<BINARYSEARCH_THRESHOLD) 
             return Collections.indexedBinarySearch(list, key); 
         else 
             return Collections.iteratorBinarySearch(list, key); 
     }

like I understand why they do it

but I just feel like it's random lol

https://github.com/wupeixuan/JDKSourceCode1.8/blob/master/src/java/util/Collections.java#L210-L216

Here

[round fossil]

oh

[woeful pasture]

what's wrong with this code?

[round fossil]

its just line wrapping

like they obv dont want the code lines to be too long

[woeful pasture]

wow JDK doesn't use braces on if statements

disgusting. anyways Yeah you don't need braces after if statements and such. Though its convention to do so

[round fossil]

well

its mainly because it adds clutter according to some

or well "verbosity"

[night knoll]

yeah

I'm in like a constant war with myself

On the cosmetic look of my code 💀

[round fossil]

lol

same

[night knoll]

Because like

here wait

Let me ask u which looks bettwr

public class Dog {
    private final String
    private final int
    
    public Dog(...) {
    }
}

public class Dog {

    private final String
    private final int
    
    public Dog(...) {
    }

}

public class Dog {

    private final String
    private final int
    
    public Dog(...) {
    }
}

[round fossil]

rnt they all the same

[night knoll]

Yeah but

What looks more appealing

[round fossil]

oh

well it doesnt matter

[night knoll]

To me it does

[round fossil]

I believe last is best

[night knoll]

what about the empty line before the fields

like u gotta balance that out somehow

[round fossil]

i mean it depends

[night knoll]

yeah it does

I mean I'm fine with something like

}

}

but not rly

    }
}

}

[round fossil]

the thing is, if the space between fields, constructors etc is needed then have it there

[night knoll]

https://github.com/skinnynoonie/BukkitLocationUtils/blob/master/src/main/java/me/skinnynoonie/bukkit/locationutils/shape/ConcreteLine.java

Is it OK to have a private Class for the iterator here

[sly jackal]

just have your shape implement iterator

[round fossil]

sure

u wna write code defensively, but u wna make sure ur code is open for extension, closed for modification (open-closed principle)

[opaque plover]

I go with 1 but it should be what you like most

[stone dagger]

What do you guys think of a structure like this (for game phases)? Whereas we separate the chain of phases from the actual phase implementation themselves and use a "cycler" instead
https://github.com/aparx/skywarz/tree/master/src/main/java/io/github/aparx/skywarz/game/phase

[round fossil]

Yes the idea is good. Its what most high player base networks use. Tho lombok there is goofy, but yours is a bit under engineered

[stone dagger]

I only use Lombok for getters and setter really, sometimes for constructors but I never use anything else just for more control

[round fossil]

Lombok is trash imo, I’d never touch a project or use sth that is involving lombok, but thats just me

lets say at the end of the day, if it makes u happy, then thats still good for u

Anyway makes no sense how you use volatile as well

[stone dagger]

well I don't agree on that since it's mostly just like code completion ... but at compile time

[round fossil]

Its not rly code completion

Its just code generation

which at runtime doesn’t change a bit

[stone dagger]

*generation that's what I meant

[round fossil]

Also, i personally really encourage to use this and super and ClassName when calling fields or invoking instance methods to add clarification

Esp on for instance github, it becomes harder to read if the method is static or not (or inherited) if u just write methodName(); on invocation

[stone dagger]

fairs, I used to do it all the time but have gotten away from that pattern due heavy programming in typescript

[round fossil]

Absolutely no need to clutter every field and every method with @NonNull, we have annots like @DefaultQualifier

[round fossil]

Yeah thats fine

Im just trying to give some feedback, take it how u want, if u dont like it thats fine

I personally dont like Optional

So I’d avoid that

This is because optional is not like Kotlins nullability, its not like rusts Option enum, its not like Haskels Maybe

just a null wrapper

[stone dagger]

I use it to especially show developers that there's no possibility of this value being null. In my style when a value does not have nonnull it may be null but isn't encouraged. To the optional part, well that's something I understand, I use this null wrapper throughout the code base purposely so I'm trying to be consistent

[round fossil]

which is well…. fine when u return it from methods but else shouldnt be used according to the java convention

[round fossil]

You didnt understand the first part

Its not needed to paint everything with @NotNull because there are annotations that define implicit contracts like @DefaultQualifier

[stone dagger]

That would say that no nullability annotation is implicitly NonNull, no?

(if I used NonNull as a default qualifier)

[round fossil]

yes

[stone dagger]

That's exactly what I don't want

[round fossil]

Which in general is what java is leaning towards

Unless something is documented to be nullable, its not null

[stone dagger]

I want three states:

• Nullability (encouraged)
• Nullability (discouraged)
• NonNull
  It's just for me personally really

[round fossil]

There is even a project running about this

Its not about states really

And then again, you double commit to Optional and @NonNull + @zealous stoneable

Also your utility class is not a utility class

EventPriority.NORMAL is by default, no need to explicate that

[stone dagger]

for me it's about static analysis & showing the developer (once they know the system behind that) when to use null values and when not to use them, so I would like to use three "states" to describe my nullability: one that ensures no nullability, and two that signal a possible null value. And I don't think these annotations and optionals are mutual exclusives

[stone dagger]

I know, this stems from a refactor

I used HIGH on these before

and then had to refactor to NORMAL

for reasons I changed two priorities: HIGH and HIGHEST to NORMAL and HIGH

[round fossil]

Definitely not but you overuse Optional, and then again, you’re like half committed to @NotNull, like the code you’ve written is super inconsistent with it

Also some of your code is really tightly coupled

I’d encourage using static factory methods more for instance and other means to address that

[stone dagger]

alright thanks for the feedback 👍

[stone dagger]

I'll have a look into that, my motto is consistency so maybe I've overseen things

[round fossil]

Yeah allg

Also

The thing with utility classes

Is that they are considered real ones when they only have utility methods that are pure and/or stateless

Math is one of those

Collections partially, but it has some static helpers as well (and some static factories and static singletons)

[stone dagger]

I used the utility annotation as a lazy way of avoiding the constructor to be called (and if so, e.g. through reflections, it would throw an exception), I'll refactor some of them since I am not keen into using anymore of @Utility anyways (I think it is kinda ugly, it makes non-static methods static and it's a mess)

[round fossil]

Myeah, I think static helper methods usually becomes a mess when you keep them in the longer run

Altho it can make sense

Look at LockSupport or StreamSupport for instance

Its not always bad, but it definitely needs to be reasoned for

[round fossil]

Anyway my point with this was that you’d rather write a code base where we assume things are not null unless they are nullable, but if you think about it the opposite way, that code base would be a living terror to maintain; that is we assume things to be nullable unless they are not null

I rate your code 7.8/10 @stone dagger

[stone dagger]

alright yeah, I mean fair point. I've been using this scheme (this "3 different mutability") for quite a while and it worked well, even in large code bases, but I know many companies also think of objects to not be null first which makes sense

[stone dagger]

alright thanks for looking into it, oddly specific number tho haha

[round fossil]

[stone dagger]

if I fix this inconsistencies

is it going to be an 8+?

[round fossil]

Well its an interesting idea of discouraged vs encouraged nullability

[stone dagger]

( I dont think I get to remove lombok this quick tho 😉 )

[round fossil]

You’re the first one to ever sort of explicate on that

[round fossil]

sure lol

[stone dagger]

I just think it's important to signal the developer exactly what the code does, makes things easier. Sometimes you want to allow nullability, but only in edge cases, whereas (explicit or implicit) @NonNull would always deny a null value. @zealous stoneable signals the value can always be null, but doesn't specify how intrusive or in what cases this can occurr. So when you mix this with proper documentation it can work well

I guess if you're really fancy you could do a separate annotation for that, that you somehow implement in a static analyzer

I just stuck with using this schematic

[round fossil]

I definitely think its cool to have some sort of discouragement/ encouragement but what exactly defines it

Since us mathematicians and programmers love strictness

[stone dagger]

yeah, well ain't we all loving some strictness sometimes

[round fossil]

Sorta feel bad for @ Null but at the same time, not :>

[stone dagger]

lmao this guy wakes up from his nightmares thinking about nullability being tagged in a conversation about nullability

[round fossil]

lol yea

[frosty grove]

why

[round fossil]

Because its meaningless imo, unlike most other third party libs which usually address some sort of functionality, lombok does nothing, it just reduces some compile time semantics, but truth be told it adds more complexity than what it addresses, in my experience it 9/10 cases it belongs to the problem set

[night knoll]

Guys

When are abstract classes supposed to store data

[round fossil]

I mean its hard, I generally avoid touching abstract classes if I can

And if I do, I like to keep the tight coupling to a minimum

That is to have as few abstract methods as possible, and as little state as possible

[night knoll]

@round fossil

Remember my character abstract class

[round fossil]

Nope

[night knoll]

well i was using it to store data and yall were kinda harsh abt it

so i switched to a non abstract class and extended it lol

[cobalt trellis]

Behold! Geolykt reinvented the wheel again (or well in this case it is <insert dependency injection framework of choice here>): https://github.com/Starloader-project/smatterDI

[woeful pasture]

https://github.com/Y2K-Media-Creations/Pineapple/tree/dev/pineapple-chat. Trying to make this a pretty decently optimized MiniMessage clone. Not all the syntax follows exactly, but 🤷‍♂️ this is my first attempt at a parser much appreciate feedback

[jagged rock]

I'd make a dedicated DummyBaseComponent class

The thing with adventure is that it's platform-agnostic, this seems like a restriction

[woeful pasture]

goal isn't to be platform agnostic

only to work BungeeChat. I could care less about paper atm. Seeing as paper has Adventure and MiniMessage built in

[woeful pasture]

also my ComponentBuilder is copied from BungeeChat (with tweaks), but good point I think I'll do that

[jagged rock]

I mean yeah but what if you went beyond minecraft

but tbh what's the point

[woeful pasture]

yeah that'd take abstraction way to far

anything you think I could do to speed up my tagging and tree parsing system?

currently my speeds (atleast on my machine) are pretty good

[jagged rock]

mmm I don't like how StyleStack is formed

[woeful pasture]

me either, but I wasn't sure what else to do

[jagged rock]

TBH I need to learn more about parsers before I start criticizing

[woeful pasture]

I took most of my parsing knowledge from a few articles and videos on the basics are parsing languages into trees

and then wrote the bridge to BungeeChat myself without any other information

these are my timings atm for the jmh (given this is ofc my machine and not totally useful seeing as my cpu is powerful)

[jagged rock]

I might write my own parser just to learn stuff

You should try comparing it to adventure's parser

[woeful pasture]

its faster, well atleast using their tests

probably because mine is much less abstracted

as far as structure the abstraction makes comparrisson impossible

I tried using it for reference initially and it was 0 help

[stone dagger]

https://github.com/aparx/bukkit-gui

Still in development tho

Ain't optimized

[sly jackal]

"most advanced" now that is a bold claim

[stone dagger]

Very*

I didn't want to claim it to be the best, i meant very advanced

[round fossil]

There are quite a few things I noticed by just scrolling through the classes, ill lyk later :^)

[cobalt trellis]

Just looking at your POM I noticed the following defects:

You'd want to remove JSR305 from your transitive deps (specifically, guava and your bommons lib) and declare it as provided, since annotations need not be shaded.
Guava can also be provided (maybe even removed entirely), as long as you do not want to risk issues with API breakages (it is provided by spigot). Otherwise, you MUST shade and relocate it.
Commons-lang3 should be shaded and relocated to prevent version conflicts with other libraries and plugins

Also after some point I'd stop relying on -SNAPSHOT versions, they can be nasty when building from source

[stone dagger]

fairs, will do so

[pine grail]

one thing I just noticed is that "interval" could be renamed to "updateEvery" or "updateInterval"

lil nitpick

[stone dagger]

hey yeah, I just implemented updateInterval which will follow with the next release, it is still safe to use interval tho for now but is deprecated and marked for removal

if you spot any issues or have more feedback, feel free to open an issue on github tho! 🙂

[torpid lion]

first time making a plugin, main issue is it just doesn't register as a plugin
https://github.com/Exoterminator/DifficultyTweaks/tree/main

[round fossil]

Have you ever worked with .gitignore files, else you should learn how to exclude certain folders and files from being pushed to upstream (github in this case)

[torpid lion]

I have not

[rancid fern]

Also use git instead of file upload

[round fossil]

Well that’s one thing, another is that usually you’d flatten out the "diff_tweak" folder and make “src” be in stored in the top directory (folder=directory) along with the readme and the other files

[rancid fern]

Plugin should be named DifficultyTweaks or DifficultyTweaksPlugin

[round fossil]

and in plugin.yml it should match the class that extends JavaPlugin (ur aka main class), such that its “package.name**.**MainClassNameThatExtendsJavaPugin”

(w/o quotations ofc)

and its case sensitive ofc

[rancid fern]

with quotations would work too

it's just a string so 🤷‍♂️

[round fossil]

Yeah just that I have the iPhone quotation marks

So it would mess it up, but thats ofc true

[rancid fern]

ah didn't notice it being a different character

[sly jackal]

cursed

[torpid lion]

ok I think I changed it all

oop nvm

[torpid lion]

which files/folders should I be excluding?

[round fossil]

target is one them

.idea

If intellij

(Usually) sometimes u dont wna exclude .idea

[torpid lion]

ok that should be fixed now

[cobalt trellis]

You want to ignore any directory or file beginning with a dot (though you need to forcefully add the gitignore via git add -f .gitignore" or similar)

[stone dagger]

An exception to this case is when you use GitHub actions the .GitHub directory

[night knoll]

https://github.com/KarmaDeb/GameLib/blob/master/gamelib-plugin/src/main/java/es/karmadev/gamelib/plugin/impl/entity/GameEntity.java#L129
Entity data serialization/deserialization

[jagged rock]

uhm

Yeah idk this looks pretty restrictive

[night knoll]

What would you suggest?

I've been thinking a while about it, but can't think about an alternative

[jagged rock]

For this switch block.. you can do better than this cmon

[night knoll]

😔

[jagged rock]

As for entity fields you can switch to a more data driven approach

[night knoll]

lol

[jagged rock]

it's a bit complex so I won't give an example right now

[night knoll]

I will something in google probably

[jagged rock]

But the idea is to make "entity data" fields where each field has its own version, get/set methods and filters

So for example, a HEALTH field would be applicable to livingentities, store a double and call LivingEntity::getHealth and LivingEntity::setHealth

[night knoll]

I think I understand what you mean. Like instead of using a "map" model, with key-value, I should use a "data" field which has a getter and a setter

[jagged rock]

So you could do something like

public class EntityField<E extends Entity, V> {

  public static EntityField create(Class<E> entityRestriction, Class<V> valueType, Function<E, V> serializer, BiConsumer<E, V> deserializer) {
    return ...
  }

...
}

and maybe even a MinecraftVersion restriction to not serialize fields that don't exist

You'd end up with something like

public final class EntityFields {

  private EntityFields() {}

  public static final EntityField<LivingEntity, Double> HEALTH = EntityField.create(LivingEntity.class, Double.class, LivingEntity::getHealth, (entity, health) -> entity.setHealth(health));

[night knoll]

Gotta admit, this looks way better than I currently have

[jagged rock]

This is just an example

You'd then have a registry of some kind

And a serializer that loops over all fields, filters to see if it's applicable to the entity and version and does magic

[night knoll]

I like it

[jagged rock]

@night knoll I was looking over your utils and stuff

This is not how you make a concurrenthashset

[night knoll]

What's doing that here?

I know

I usually use ConcurrentHashMap.newKeySet

[jagged rock]

While the backing map may be concurrent, that newSetFromMap's structure is not concurrent

[night knoll]

Where is that at?

[jagged rock]

globalplaceholderengine

this is also not smart

Just make a Unit[]

[night knoll]

All this is from KarmaAPI right?

[jagged rock]

Yeah

[night knoll]

KarmaAPI or KarmaAPI2?

[jagged rock]

KarmaAPI

[night knoll]

Yeah, don't look at that code, it's all messed up

[jagged rock]

well

it's public

[night knoll]

That's why I started KarmaAPI2

[jagged rock]

Set it to archive mode or sumn

[night knoll]

Yeah, I'll be archiving it soon

[jagged rock]

it's also in your pinned

[night knoll]

Thought I've change those to the new project versions (2)

Anyway, thanks for letting me know

[night knoll]

https://github.com/skinnynoonie/NoonieConfigs

It's untested and docs aren't finished but I want an opinion on how it looks so far

What it does is it saves an Object into a form (json yml etc). But let's say if you update an object, this should auto update the "raw" form

[sly jackal]

im having a hard time finding the right files xD

so this will update the files everytime a change is made to the json object for example?

isnt that kinda slow?

also when do you want to keep a raw form and when do you want to keep an object form, maybe im just not seeing it?

[stone dagger]

I think this is a better solution, not perfect but better
https://github.com/aparx/skywarz/blob/master/src/main/java/io/github/aparx/skywarz/language/VariablePopulator.java#L243

[round fossil]

TemporalAccessor, TemporalAmount, DateTimeFormatter, DecimalFormat: “am I a joke to you?”

[stone dagger]

I aint using this shite

Hehhehe

[round fossil]

Yeah yours look quite duct taped, scary stuff

But on a serious note, you should seriously consider using java’s framework unless you have some very specific reason not to

[stone dagger]

Cause it is duct taped

That's why I said not a perfect solution 😂

But it works I guess

[round fossil]

A lot of stuff does work in fact

:>

[stone dagger]

And I didn't know you could modify the unit names depending on their respective amount :p

Using javas default time utilities

[round fossil]

Yeah, well javas date time formatter is powerful, tho I assume ppl just presume SimpleDateFormat (terrible class) is the only thing Java got and just yikes it away

Oh yeah btw I think TemporalAmount derivatives have some nice api methods like that allows u to take respective temporal field and normalize it for u by taking modulo on it

[night knoll]

Here's how I pictured it, dividing it into layers

ConfigService - Converts the @Config annotation to config names
RawConfigDao - interacts with the storage, but it only parses data into a raw config form
RawFormConverter - this actually converts the raw config form into objects
RawFallbackAppender - this appends any missing fallback values to the saved config

Now that I think of it, I might be able to combine the converter and fallback appender

I did this cause let's say:

class DogConfig:
private String name

Saved in storage would be:
{"name":"something"}

now if I released a new version, and DogConfig is updated:

class DogConfig:
private String name
private int age

It will append the age field into the storage's current data

[round fossil]

Why have you named it dao lol

You rarely ever name something DAO or DTO

Usually you give a proper suffix or prefix, like RawConfigRepository etc

Also Max, Appender is a goofy suffix for what it does, id go with sth like Resolver instead

[night knoll]

Should I combine the appender and converter

[cobalt trellis]

what even is the purpose of instances of that class?

[night knoll]

i explained it here

[sly jackal]

these all look like things that can be done with functions

like whats the lifetime of these objects

[night knoll]

wdym functions

oh like the interface

[night knoll]

what object

[sly jackal]

for example rawfallbackappender

looks like it could just be a function on the converter class or whatever

[sly jackal]

yes I think you should, they both just modify the object data

[night knoll]

it has 2 methods tho

[night knoll]

for that

[sly jackal]

yeah maybe the converter isnt the right place

idk

[night knoll]

whats a good name

for the converter and updater

or appender

[sly jackal]

idk

[night knoll]

cause they do kinda do different things

[sly jackal]

the issue with the fallbackappender is that it doesnt really append?

[night knoll]

it appends missing values

cause if you update a class, the current saved config would have missing values

[sly jackal]

hmm

if I was to make this kinda system I would just have an interface/class for like JsonConfigData or something

and then have the functions like load, save

and then load would just do the fallback thingy

[night knoll]

functions or methods

lol

[sly jackal]

so its kinda hard o come up with names when I would do it separately

they mean the same to me

[night knoll]

ok cause there's a function interface and I was getting confused with that

[sly jackal]

methods in this case since they are on the object of JsonConfigData

oh no

I just mean a method then

[cobalt trellis]

I mean then converter makes little sense. DefaultValueResolver or FallbackValueProvider are good example names

[night knoll]

should I include "raw" to inform that raw data forms are being used

[cobalt trellis]

I haven't looked too deeply to know the diff between raw and non-raw

[night knoll]

non-raw is an Object, like DogConfig

raw is what the object is represented with

like Json or YML

[sly jackal]

will there also be a non-raw version of the method?

[night knoll]

yes

wait wdym

[sly jackal]

well, a fallback provider that parses non-raw configs

like DogConfig

[night knoll]

yeah

did u look at the class

oh fall back

uh no

[sly jackal]

yeah so then I dont think it needs the prefix

[night knoll]

yeah actually ur right

i feel like it couples the purposes too much with configs

but im gonna keep the rawconfigdao, just rename it to repository

[woeful pasture]

https://github.com/Y2K-Media-Creations/Pineapple/tree/dev/pineapple-chat Please keep thoughts surrounding this specific module please

[alpine anvil]

if you say "Why not use minimessage" im sending you to pluto

[sly jackal]

I wonder what that would be like

also thats a pretty expensive trip

[alpine anvil]

bc its huge

theres no way to get minimessage on spigot without shading 13 mb or adding it as a library to every single plugin that uses it

cant upload a plugin thats 13mb big on spigot

[rancid fern]

It's not that large

BlueSlimeCore does contain MiniMessage

[alpine anvil]

also minimessage doesnt really give you a good way to bungee component it either

[rancid fern]

And I know you're using it

[alpine anvil]

kekw

[jagged rock]

I wonder if my skyblockcore project will get so big to the point where I can't upload stuff to spigot

[rancid fern]

Wdym? You can convert between the two?

[alpine anvil]

iirc only the old seperated minimessage had bungee conversion

[rancid fern]

https://docs.advntr.dev/platform/bungeecord.html#component-serializers

[night knoll]

What do yall mean too big for spigot

Like the plugin marketplace

[jagged rock]

hm it's only 300kb as of right now so I might be safe

it also has 0 features other than just being its own lil platform and database

[jagged rock]

limit is like 4mb

[woeful pasture]

yes because we need to do more Serialization than is already required

I preferably want feedback on the code itself not whether or not making what I already did was a sound decision

for me it was which is why I created it

[woeful pasture]

.

[night knoll]

Tf

Enums can have abstract methods

Whaaaat

[rancid fern]

I was just talking to Ebic

[sly jackal]

can you even extend enums? xD

that would kinda defeat the point

[night knoll]

No like

Look

[alpine anvil]

you would override it in the definition

VALUE() { @override } iirc

[woeful pasture]

https://github.com/Y2K-Media-Creations/Pineapple/blob/dev/pineapple-chat/src/main/java/sh/miles/pineapple/chat/style/Decoration.java

[night knoll]

https://github.com/Y2K-Media-Creations/Pineapple/blob/dev/pineapple-chat/src/main/java/sh/miles/pineapple/chat/style/Decoration.java

Oh

k

[sly jackal]

oh right

funny

[jagged rock]

I feel like it should be made the opposite way

Instead of setThis setThat the methods would set an internal field

well maybe that's what his is lmao I'm stupid

[night knoll]

https://github.com/NazarbekAld/SpigotExamplePlugin

Gradle(kts) + Kotlin

[round fossil]

Nice

But what happened to the gradle wrapper lol

[night knoll]

Wdym?

The gradle folder?

[round fossil]

yes lol, and the bat one is gone in ur upstream

[night knoll]

Why?

Need that

I excluded useless stuff that ide generated

[rancid fern]

They're not useless

they should be there in every gradle project

[round fossil]

^

Different wrapper versions have different gradle apis

So not being consistent with it may mess things up

[night knoll]

Ok

[round fossil]

The only folder iirc is .gradle which you can exclude since thats just intellijs

[cobalt trellis]

nah, .gradle is just a cache folder (this is the folder where the gradle jar is downloaded to). Doesn't hurt to include it but doesn't make sense to do so

[round fossil]

Hmm, i thought intellij dumped of its stuff into it as well on top of it all

[cobalt trellis]

That could be the case, but obviously as an eclipse user I cannot tell

[round fossil]

Yeah fair enough

[night knoll]

{
  "item": {
    "material": "DIAMOND_SWORD",
    "name": "<!italic><red>Cool sword",
    "lore": [
      "hi<blue>hi <black>dark...",
      "<!italic><blue>hilol"
    ],
    "unbreakable": true
  }
}

This a good way to configure items?

@Config(name = "MyFirstCustomItemConfig")
public class MyCustomItemConfig extends CustomItemConfig {

    public MyCustomItemConfig() {
        super(new ItemBuilder(Material.DIAMOND_SWORD)
                .setName("<red>Cool sword")
                .setLore("<blue>hi <black>dark...", "<purple>lol")
                .setUnbreakable(true)
                .build()
        );
    }

}

[rancid fern]

Is that MiniMessage

[woeful pasture]

nah its PineappleChat 💪 /j

[rancid fern]

if so do make sure to provide builder methods that accepts components too

[woeful pasture]

yeah as long as you're consistent. Just make sure your ItemBuilder can handle components as Olivo said

[night knoll]

lol

yeah I will

[woeful pasture]

are you using Paper API?

[night knoll]

ye

[woeful pasture]

alr was gonna say if not you're in for a lot of pain

[rancid fern]

You can always shade and provide adventure

That's what I do

[woeful pasture]

true but its useless as hell

[night knoll]

isnt it like

10mb

tho

[woeful pasture]

you can't add things to items

[rancid fern]

300kb

[woeful pasture]

nor inventory titles

[night knoll]

wtf who said it was 10mb

I like this tho cause users can't really break this config (unless its a missing bracket or something)

[woeful pasture]

most good text editors provide checking for json

[night knoll]

like if I had this:

{
  "item": {
    "material": "DIAMOND_SWORD",
    "name": "<!italic><red>Cool sword",
    "lore": [
      "<!italic><red>My Lore!",
      "5"
    ],
    "unbreakable": "not a boolean"
  }
}

it would automatically replace the value with a fallback one

[woeful pasture]

any experience server admin would use notepad++ or an equivalent which provides syntax highlighting

[rancid fern]

Not a big fan of json configs

yaml >

[night knoll]

im just used to gson

i like their api

[rancid fern]

Configurate best config api

https://github.com/SpongePowered/Configurate/

Easy to works with an has support for many different formats

[night knoll]

hm

im just using one that I made myself

[rancid fern]

sadly yaml comment support has been W.I.P for a long time now

[night knoll]

https://github.com/skinnynoonie/NoonieConfigs

pretty small api

[woeful pasture]

@night knoll back on topic though just make sure you're consistent and you doccument how to construct items

doccumentation is key

[night knoll]

wdym

[sly jackal]

then you are being too naive xD

[woeful pasture]

for items

[sly jackal]

users can break anything with user input

[night knoll]

oh in the json format?

[woeful pasture]

yes

[night knoll]

uh

if they put any like

redundant values

it just removes them lmao