[MEGA THREAD] Get Your Code Reviewed or Review Others

[dense leaf]

you actively hurt me

[pine grail]

skill issue

greatest of all time eating disorder

[dense leaf]

u2

[desert ore]

yes

[late smelt]

C# is just Microsoft java

[sleek shard]

yep

[late smelt]

With a bunch of fancy features sprinkled on top

[sleek shard]

with bad syntax

Capital Methods, braces on different lines

[vital jasper]

agreed

[night knoll]

Capital methods and using _variableName for internal variable is crazy.

[dense leaf]

i mean

i use _ for mutable collections all the time when i just wanna expose immutable ones

[desert ore]

you are just weird

[dense leaf]

how else am i supposed to do it

private val _universes = mutableListOf<Universe>()
val universes get() = _universes.toList()

this is what 95% of people do (kotlin users at least)

[desert ore]

now imagine if only you could write getters...

like getUniverses

[dense leaf]

wait ig

val universes = mutableListOf()
    private get
    public get() = universes.toList()

i think this works in kotlin

@surreal peak can you confirm

[hybrid osprey]

did you mean private set

[dense leaf]

what

[hybrid osprey]

private get // public get

[dense leaf]

no that's intentional

[hybrid osprey]

oh

[dense leaf]

private get = mutable
public get = immutable

[desert ore]

I believe that wouldn't work

[dense leaf]

maybe private get() = universes

that can work i believe

[desert ore]

how can you have two methods of the same name in one class

[dense leaf]

nope

[desert ore]

kek

[dense leaf]

this literally does the exact same thing

[desert ore]

why did kotlin not thing of some immutable-on-get thingy

[dense leaf]

there still needs to be java compat

[desert ore]

it will just change the get methods code

[wintry fern]

https://tenor.com/view/pointing-weird-individual-weird-jordan-you-are-weird-gif-17690426

[surreal peak]

i usually do it as

    private val _uiState = MutableStateFlow<YourFeedState>(YourFeedState.Loading)
    val uiState = _uiState.asStateFlow()

[dense leaf]

you would need a getter for state tho

because that way you would only have the initial state

[surreal peak]

implicit getter for second line

[dense leaf]

wrong channel buddy

[gilded gate]

oh sorry

[junior holly]

https://github.com/NukeCaps/PlayerTitles/tree/master/src/main/java/nuclearkat/playertitles

Feedback ❤️

[hybrid osprey]

some configuration would be nice, readme explaning what this should be doing

[dense leaf]

https://github.com/NukeCaps/PlayerTitles/blob/master/src%2Fmain%2Fjava%2Fnuclearkat%2Fplayertitles%2Flistener%2FTitlesGuiListener.java you should be using the pdc here

and not comparing by item names

[hybrid osprey]

and I feel like getPageIndex is hacky

on a more broader scope

[dense leaf]

this bit specifically since you're using the pdc in a diff else case..?

        if ("Next Page".equalsIgnoreCase(clickedItemName)) {
            int currentPage = getPageIndex(event.getView().getTitle());
            plugin.getTitlesGUI().openNameTagGUI(player, currentPage + 1);

        } else if ("Previous Page".equalsIgnoreCase(clickedItemName)) {
            int currentPage = getPageIndex(event.getView().getTitle());
            plugin.getTitlesGUI().openNameTagGUI(player, currentPage - 1);

        } else {

[junior holly]

I just never gave those two items any containers

[junior holly]

yeah a little bit haha but it works

[hybrid osprey]

you asked for code review

it works != good enough

[junior holly]

Wait I asked for a code review?

[hybrid osprey]

this is that thread

[junior holly]

That was sarcasm

[dense leaf]

...

then don't be asking here

[junior holly]

?

Go away kotlin nerd

[dense leaf]

i swear my discord client hides some fucking messages

[junior holly]

modified client?

[jagged rock]

ew

Make layers!

[surreal peak]

someone is gonna rename their chest and have fun

[hasty lotus]

Uh oh

[woeful pasture]

?gui

[rich fog]

https://www.spigotmc.org/threads/a-modern-approach-to-inventory-guis.594005/

[surreal peak]

didnt need to tag me for that

[woeful pasture]

It's on by default I cbf to turn it off

[round fossil]

Use Path and Files (nio), not File

[desert ore]

does that make any difference tho

last time I used nio I got some weird exception that disappeared by using io

[surreal peak]

modern api

[cobalt trellis]

I personally prefer nio for file IO, too - especially when making APIs

[random yacht]

Weird. I've never really liked NIO. Maybe I just haven't used it enough to find a liking to it like I did with java.time

[cobalt trellis]

The thing with NIO is that it allows use of non-file:// protocols. So jar:// is completely a-okay. You can also do custom ones, as is done by jimfs.
This makes NIO much more versatile compared to java.io.File. The fact that you also have helper methods in Files for common operations such as opening a buffered reader, reading all bytes into a byte-array and more is also quite helpful.

[sly jackal]

I feel like this is just very subjective

like surely both can work depending on your goals

if not one would be deprecated surely

[woeful pasture]

I pretty much only use nio

[cobalt trellis]

io is fine for applications, not fine for libraries

[round fossil]

Depends, I mean they’re using File#listFiles for example, which essentially causes DOS given a large enough directory (the return type is File[] so you can imagine), here the Path + Files nio is more or less mandatory if u want to operate successfully w/o a DOS. There are other issues like exceptions being somewhat poorly designed (a lot of methods did not throw properly when failing), renaming works inconsistently depending on OS iirc, the attribute and permission system is better in NIO (think its supposed to be more performant [additionally] but I have no real source of that being the case), recursive file walking was also somewhat unreliable esp w symlinks. (Though symlinks is a general problem w legacy file io)

I still think its a wonder how it hasnt even been deprecated yet, but I suppose the time will come sooner or later

[junior holly]

Does nio give me problems such as denying access to a file I literally created? That was an issue yesterday

[round fossil]

I think that might be more of a PEBCAK issue

[cobalt trellis]

that ordinarily isn't a problem

At least I never encountered it - but then I'm also exclusively using one OS (linux)

[junior holly]

Haha didn’t think so, was just something I’ve never encountered before

[round fossil]

yea I mean maybe NIO would yield a more helpful error message

otherwise there are some other obscure ways in java to deal with “access denied” actions (doubts that what u’re looking for tho lol)

[junior holly]

Well it was just so odd because I didn’t manipulate the permissions of the direct at all after creating it

I don’t even know how to do this tbf

[cobalt trellis]

Maybe the permissions of the directory the file way located in had a play in here?

[desert ore]

Back when I made my gradle plugin, I was unable to use Files.newOutputStream on a certain file as it gave me some exception, instead I had to use new FileOutputStream. Any idea why?

java.nio.file.AccessDeniedException

[junior holly]

First time testing, on the local host access was denied, my account is a super user account so still why I’m confused (since it’s held locally)

[cobalt trellis]

superusers are ALWAYS a bit finicky when it comes to file access perms

[junior holly]

Maybe a windows thing then?

It only ever happened once

[round fossil]

Got an error message?

[desert ore]

java.nio.file.AccessDeniedException: build\mappings\mappings-1.19.4-intermediary.tiny

[cobalt trellis]

Gradle transforms your class files for whatever reason - sometimes causing bullshit errors.

Although more often than not the results of it's transformation is that you'll get NPEs where in theory NPEs should be impossible because gradle for whatever reason replaced a null-safe method with a null-unsafe one.

[desert ore]

Lol

[round fossil]

wait u got a stacktrace as well?

[desert ore]

not rn

[round fossil]

but yea sometimes u just need to unalive the gradle DEMONS

like running the unalive command

[desert ore]

why did new FileOutputStream work

[desert ore]

Inside of a gradle task? Bad idea

[round fossil]

eh I mean gradle is stupid sometimes

[cobalt trellis]

nah, unalive -9 pid where as pid can be obtained from the jps command

[round fossil]

lol

[desert ore]

If I kill gradle I will kill my gradle plugin so I should probably not do that inside of the plugin lol

[junior holly]

Also did you guys look at the TitleCreationConversation class? I had no idea conversations were a thing until now, and tbf I'm only using it as a chat prompt for multiple lore lines but still

Wanted some notes on that too since it's the first time I've used em

[round fossil]

I feel like every new gradle version treats us as labrats, but apart from that its pretty great being an elephant enjoyer, anyway there is a command to terminate active gradle daemons if it fucks up w file watchers or file locks etc

thats what I meant before ^^

[junior holly]

I feel like it may be considered a bit overkill considering I'm just using it to gather lore

[desert ore]

I think I even restarted my laptop back when I had that issue and it persisted

[muted sentinel]

how bad is this? https://paste.md-5.net/azenepecun.java
scale 1-10 (1 being absolute shit - 10 its good)

[junior holly]

Didnt really look through it but first off put all your getters on the bottom

or at least organize them in a way

[muted sentinel]

my issue is this class is based off everything. i can organize it but thats not the problem. i just think i could've done it better

[alpine anvil]

just having 1 set of homes seems a bit weird, id expect it to be a uuid to a list string or a user map to differentiate between homes

[junior holly]

Also it seems this impl is going to be a manager instance per player doesnt it?

[muted sentinel]

yes

[junior holly]

It should handle homes of all players not just one

[muted sentinel]

What do you mean? it handles whomever uses the command in a sense

[junior holly]

Well you pass a player object to the constructor of the manager meaning when you create a new instance of the manager, that is the player you are dealing with. Granted the player object is not final so it can change but I still feel this isn't needed

The player object can be gathered from when they use the command and passed this way

[muted sentinel]

so i should pass the player through every method instead?

[junior holly]

For example, addAllowed should take (Player player, int amount) because otherwise who are you adding the homes to? The original player object you instantiated the class with

[muted sentinel]

So in other words the way i did it by passing the Player through the contructor was stupid

[woeful pasture]

• Follow Java Code Conventions https://www.oracle.com/technetwork/java/codeconventions-150003.pdf
• Avoid start imports they suck import java.util.*;
• import static me.arkallic.mineadventures.managers.UserManager.*; its very unlikely you use UserManager enough to justify this start import
• Make sure your home keys are immutable. as of now its likely that it returns an immutable set, but if you're returning it directly without wrapping it you really should just wrap it yourself either using GSON's ImmutableSet or just using Set factory methods.
• I can't think of a single reason you need the sets size getSize method is useless
• getCurrent is redundant it literally just calls getSize choose one or get rid of both
• Don't cast getLocation getX use getBlockX and so on
• Don't pass the Player Object, this can cause memory leaks. You should instead pass a UUID. Its likely that the fundamental design of UserManager is flawed too.
• This isn't a manager class its pretty much just a Data class wrapper. A more fitting name would be HomesData. Why homes data would have to create a new UserData is beyond me again points to flawed structure.
• Don't hard code messages especially messages with legacy color codes. User proper contstants or a proper translation system.
• Send chat as a method makes 0 sense. If you have a chat utilitity you can simply do player.sendMessage(Util.color(message)); Your IDE allows you to type like 3 letters before tab completing it. If you have ML detection on you can even complete the entire thing in one tab. You could also setup a snippet to do it for you.

•  public void addAllowed(int amount) {
       user.getConfig().set("Allowed-homes", getAllowed() + amount);
       user.saveConfig();
       sendChat(player, String.format("&7You're now allowed &a%s &7homes. &7Allowed homes: &a%s", amount, getAllowed()));
   }``` This method is trash. Don't save the config every time this is updated. You're doing file IO every time this is called. That will slow down the server if this method is executed too many times at once. `removeAllowed` is also trash for the same
  

• Any method that saves the config should be done lazily on server shutdown or periodically Not every time an action is done. Good way to freeze the server.
• Use consistent method naming your names are all over hte place. ou have set but getHome choose one syntax and stick with it.

•       if (!user.getConfig().contains("Homes")) {
            user.getConfig().createSection("Homes");
            user.saveConfig();
        }``` this is trash bad as well, you can do this lazily. You are also doing File IO again every time this object is constructed. You could keep createSection in there and remove the save and it'd slightly better

that's about everything I have

[junior holly]

A little bit, theres no need to because all the methods in that class would have to deal with a specific player not just one

Specific in terms of multiple players but a specific at a time

[muted sentinel]

Damn Y2k, that's a lot. So you're saying Start over. xD

This whole plugin is pretty much like this.

[hybrid osprey]

Good review though, valuable

[wintry fern]

Not all applications run with super user permissions in windows

Especially the command line

You need to specifically run the command line as admin if you want everything ran inside it to have super user perms

[muted sentinel]

?paste

[rich fog]

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

[muted sentinel]

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

Is this any good atleast?

[woeful pasture]

you need to stop abusing classes

these can pretty much just be static methods ( MY OPINION)

I see little to no benefit in wrapping things here

even if you stick with your current structure, which is fine. Cut down atleast on the useless getters

in what case do you need to get the file. It seems like you expose it for pretty much no reason

[random yacht]

I mean tbh a config wrapper like that is probably fine. It won't work because you're instantiating the File object inline before name has been instantiated, but it's fine to do a wrapper like that

Respecially because things like reloadConfig() aren't really provided by MemoryConfiguration. It's provided by JavaPlugin

[random yacht]

Something I've taken a liking to is writing config wrappers that actually have methods to get the info you need in the types you need

https://github.com/2008Choco/VeinMiner/blob/master/veinminer-bukkit/src/main/java/wtf/choco/veinminer/config/VeinMinerConfiguration.java

Then implementing it basically as a wrapper around plugin.getConfig() calls https://github.com/2008Choco/VeinMiner/blob/master/veinminer-bukkit/src/main/java/wtf/choco/veinminer/config/impl/StandardVeinMinerConfiguration.java

Simple, effective, makes your code a lot clearer, keeps all your string constants in one place so you don't have to redefine them in a dozen different places

[hybrid osprey]

VeinMinerConfiguration extends VeinMiningConfiguration that's not confusing at all /s

[random yacht]

Think of it kind of like how YamlConfiguration extends ConfigurationSection but also has a getConfigurationSection()

You can get a VeinMiningConfiguration for each individual tool, but there's also one in the root which acts as the default fallback. So you can getVeinMiningConfiguration(ToolCategory) but also the root itself acts as a vein mining configuration

Things like max vein size and whatnot

[random yacht]

There is. There's UnmodifiableView as well

They're just marker annotations. IDEs can use them to show you warnings that a returned collection is immutable

[jagged rock]

The one thing I'm not a fan of is constantly spamming configSection.getThis getThat

It makes reloading simple however

[random yacht]

Then make a convenience config() method or something :p

[jagged rock]

Not that either

I like to get the values from the section and cache them as fields

My usual config workflow goes a little like this

public class SettingsFile extends AbstractConfiguration {

  private final int maxPlayers;

  public SettingsFile(JavaPlugin plugin) {
    super(plugin, "settings.yml");

    this.maxPlayers = getConfiguration().getInt("max-players", 10);
  }

  public int getMaxPlayers() {
    return this.maxPlayers;
  }
}

[random yacht]

Yeah but then reloading becomes a bit annoying. Plus you're just adding an additional layer of fields for really no benefit

YamlConfiguration is a MemoryConfiguration. It's all held in a Map already

[jagged rock]

Well yeah but some values are more complex (list of materials, for example)

[hasty lotus]

We should make /reload delete the server lol

[jagged rock]

omw to use bytebuddy to make /reload hard crash

[hasty lotus]

It’s not great to use it for so much stuff

[random yacht]

I mean config reloads

Not plugin reloads

[hasty lotus]

Oh ic

[junior holly]

If your plugin breaks on a /reload that's just a skill issue on the owner's part

Imagine not making your plugin /reload safe

[late smelt]

This is why I load my config values into fields using reflection

Sure it might make reloading a bit slower but it handles all the conversions needed

[round fossil]

For configurations, object mapping is really neat, i think there are good examples of this in LuckPerms src code, Minecrafts code and Sponges code

[hasty lotus]

it's not always about that tho, sometimes you can't really as a developer

examples are like using reflection to modify the command map (brigad), creating custom enchants

also NMS use, packet stuff that was only designed to be loaded once or done only on server load up

its not just about reloading configurations into memory but other stuff too

[round fossil]

Back in the days classloading sometimes bricked during reloads as well

so it was just somewhat unstable and unreliable

I think Plib used to suffer from something like this making it heavily “unreloadable”

[muted sentinel]

I'm obviously not getting it, may just give up dev lol

[round fossil]

Na don’t compare urself, just keep coding and u get better day by day

[desert ore]

On some hybrid servers, back when I used those, it was common for the reload command to completely break minecraft, chests became empty, commands stopped working, crafting tables didn't work, etc.

[round fossil]

Only god knows how those hybrid servers were implemented

[hasty lotus]

that's so funny

i wanna try that

[desert ore]

I believe I might have a backup of that server somewhere

[hasty lotus]

Do you remember what server software it was lol

Was it mohist

[desert ore]

I believe it was magma 1.12.2 but I'm unsure

[hasty lotus]

I swear every time a hybrid dies some of the old devs then work together and create a new hybrid that just dies again

Loop continues forever

[desert ore]

Lol

[round fossil]

Didnt we have like yatopia at some point also lol

[desert ore]

Yk how the fabric loader can technically wrap any java program? Can we wrap spigot? And then use it as the ultimate fabric-spigot hybrid?

[hasty lotus]

lol

Magma also died in a very bad way too

https://pixelmonmod.com/viewtopic.php?f=5&t=29799

Lmao

[round fossil]

💀

[desert ore]

Fuck the magma people

[junior holly]

Gee it’s almost like I was being sarcastic

[muted sentinel]

So what does abusing classes mean

[junior holly]

Mainly just means you're splitting up too much logic that could be consolidated to a single class

[muted sentinel]

So by me having my homesmanager outside of my player class is technically class abuse

[woeful pasture]

It's not really a term I literally made it up. But put simply you're making bad abstraction imho. It's great you're already comfortable with making classes but you have a misunderstanding of manager classes.

I can't really give you examples of what I mean till I get off work

[muted sentinel]

I'm at work to dw

[woeful pasture]

I'll send some later to hopefully illustrate my point better

[muted sentinel]

I get I'm not doing stuff wrong, still learning. That's why I come here

[junior holly]

Term you made up or not it still applies to a lot of people kek

[woeful pasture]

Yeah knowing when to and when not to abstract comes with time and experience you'll eventually get the hang of it

[woeful pasture]

It'd be nice if you could get your source on github when you have time it'd be cool to see the overall look of your project

[muted sentinel]

I'll put it up when I get home

https://youtube.com/playlist?list=PL2OrQJM8zmZ1c5jG5jeXkjlgQoLkaL2T6&si=Tq8jq8F6Aol3iH14 been listening to this

But I'm gonna be changing a lot when I get home, the sendchat is a big one

[round fossil]

level up :0

[muted sentinel]

I may redo this entire project

[night knoll]

💀

[dense leaf]

💀

[muted sentinel]

I just listen to what all the variables mean and how to use it

Which is teaching me what @woeful pasture is telling me

[woeful pasture]

basically a manager pattern is really controlling some set of data.

Below I'll give a simple example of a data class that a manager could control

public record Car(String name, String model){}

public final class CarManager {
  // this map contians "name" -> CarObject
  private final Map<String, Car> map;

  public CarManager() {
    this.map = new HashMap<>();
  }

  @Nullable
  public Car getCar(@NotNull final String key) {
    return map.get(key);
  }

  public void register(@NotNull final Car car) {
    this.map.put(car.getName(), car);
  }
  
}

A Manager classes manages some set of data. It can be thought of as a very simple Registry of sorts.

Now lets kind of expound this to your data set. We have a situation where each user has some list of homes so setting up a basic data class is pretty trivial. It is important now that we put our eggs all in one basket. First thing we need to make a list of what our PlayerData entails.

PlayerData

• Player's Homes Config
• The player's unique ID
• maximum homes the player is allowed to set

Now we can actually model this as a data object

public class PlayerData {
  private final UUID playerUUID;
  private final MyConfigWrapper playerConfig;
  private final Set<String> playerHomes;
  
  // this is the minimum data we need to set up
  public PlayerData(UUID playerUUID, MyConfigWrapper playerConfig) {
    this.playerUUID = playerUUID;
    this.playerConfig = playerConfig;
    this.playerHomes = playerConfig.getConfigurationSection("homes").getKeys(false); // you should make this a mutable set
  }

  // you can add other methods to modify player data just don't save!!!

  // this method does File IO as such you should never run synchronously unless the server is shutting down.
  public void save() {
    // you should save your config here
  }
}

public class PlayerDataManager {
  private final Map<UUID, PlayerData> playerData = new HashMap<>();
  
  // you can now write methods to add and remove etc or any other methods you'd need to make it easy to obtain and modify whatever PlayerData you want.
}

Basically K.I.S.S policy is best here. Don't over-complicate things with too much abstraction or too many objects everything you want to do can be achieved in 2 objects.

You could and you may receive recommendations from others here to abstract your data more, but currently I'd disagree to say its beneficial for you to do so. Keep it simple you can add complexity later.

@muted sentinel

[muted sentinel]

https://github.com/MrArcane/MineAdventures

@woeful pasture

The fileUtils is probably the only thing im saving

[muted sentinel]

damn this is confusing me

[woeful pasture]

what is confusing youi

[muted sentinel]

I'm using my original fileUtils class as my wrapper extended YamlConfiguration but how do I pass the constructor in?

[woeful pasture]

add parameters to the method

[muted sentinel]

In my fileUtils?

My like CreateConfig?

Sorry currently in car

[muted sentinel]

public class PlayerDataManager {
    private final HashMap<UUID, PlayerData> playerData = new HashMap<>();

    public boolean hasPlayerData(UUID playerUUID) {
        Player p = Bukkit.getPlayer(playerUUID);
        if (playerData.containsKey(p.getUniqueId())) {
            Core.getInstance().getLogger().info(String.format("Player file: %s exists!", p.getName()));
            return true;
        }
        
        playerData.put(p.getUniqueId(), new PlayerData(p.getUniqueId(), new FileUtils()));
        Core.getInstance().getLogger().info(String.format("Player file: %s doesn't exist!"));
        return true;
    }
}```

so PlayerDataManager just does stuff like this?

[remote kiln]

ig you could use computeIfAbsent

[woeful pasture]

Pretty much

[woeful pasture]

Looking for ways I could improve this dependency loading system. I'm not liking how many times I have to look at the graph of each dependency to check for circles etc, but idk how else I'd ensure no circles occur
https://paste.md-5.net/nofecagipi.java
https://paste.md-5.net/zaporiduqo.java

Data Classes incase you want context
https://paste.md-5.net/umedajazeg.css
https://paste.md-5.net/tobewahiqo.css

[clever crag]

SimpleItem, SimpleInventory, SimpleRandom, Scheduler, KeyGenerator, Database, Config
https://github.com/MinesonTeam/MinesonAPI/blob/development/bukkit/src/main/java/kz/hxncus/mc/minesonapi/bukkit/inventory/SimpleInventory.java
https://github.com/MinesonTeam/MinesonAPI/blob/development/bukkit/src/main/java/kz/hxncus/mc/minesonapi/bukkit/item/SimpleItem.java
https://github.com/MinesonTeam/MinesonAPI/blob/development/bukkit/src/main/java/kz/hxncus/mc/minesonapi/bukkit/scheduler/Scheduler.java
https://github.com/MinesonTeam/MinesonAPI/blob/development/bukkit/src/main/java/kz/hxncus/mc/minesonapi/random/SimpleRandom.java
https://github.com/MinesonTeam/MinesonAPI/blob/development/bukkit/src/main/java/kz/hxncus/mc/minesonapi/random/KeyGenerator.java
https://github.com/MinesonTeam/MinesonAPI/blob/development/bukkit/src/main/java/kz/hxncus/mc/minesonapi/database/Database.java
https://github.com/MinesonTeam/MinesonAPI/blob/development/bukkit/src/main/java/kz/hxncus/mc/minesonapi/config/Config.java

[jagged rock]

extending itemstack yike

[clever crag]

why not

[woeful pasture]

Don't extend api

Youll live to regret it

And no we won't introduce compatibility for that

[junior holly]

Ith most people just make wrappers for itemstack creation w/o the use of extending it?

[woeful pasture]

If you want a little hint of why it's stupid look at internals

Your data will be lost on top of extending api not being supported

[clever crag]

what

my simple item that extends item stack dont require any compatibility

[woeful pasture]

It might in the future

What happens if ItemStack becomes an interface

[clever crag]

i change my code

if itemstack become interface

anything can happen, why i cant extend itemstack

[jagged rock]

fun fact

[woeful pasture]

If your fine with dealing with the consequences continuing to make stupid decisions is okay I suppose

[jagged rock]

paper on 1.21+ now reinvented the itemstack impl

and you can no longer create itemstacks for non-item types

(I had to deal with this on Pikamug's quests by literally rewriting a couple hundred lines of code)

[clever crag]

for example you use World class and itcan change a name

or structure

[woeful pasture]

Items are on my chopping block after inventories

[dense leaf]

Wow you're not extending inventory anymore

good job

[jagged rock]

they delegate everything to a CraftItemStack reference

which delegates to the nms itemstack

[dense leaf]

after having 10 people tell you the entire time

[clever crag]

XDDDDD

[junior holly]

They switched to extending itemstack lol

[clever crag]

i just tried

[woeful pasture]

As it should be tbh

[clever crag]

extending itemstack is very comfortable

[woeful pasture]

Unfortunately making 20 big prs at once isn't fun

[dense leaf]

No

[woeful pasture]

Extending any API unless specified otherwise in its javadocs is extremely stupid

[junior holly]

I think you’re the only spigot dev to have ever said this

[clever crag]

ok

[clever crag]

using api, extending api, who cares?

[junior holly]

Kek

[woeful pasture]

The people who maintain the API

[junior holly]

It’s almost as if structure / architectural design exists

[woeful pasture]

Id actually be more likely to encourage you to use NMS

Make a custom item api implementation

And use methods to translate to bukkit

[clever crag]

According to you an itemstack can become an interface, same with any classes you just use in code. If the developer makes any changes, you have to keep up with it.

[woeful pasture]

Yes but also it's quite common that there are complaints about changing things that people shouldn't be using or doing with API becuase they don't heed the warning

If you understand this isn't safe cross version that's good enough for me

But when it breaks I don't want to hear anything

[clever crag]

i can just change my code if it happens

[dense leaf]

Yea but that makes no sense to do

[junior holly]

Why get your code reviewed if you won’t even listen to the suggestions

[dense leaf]

It's just unnecessary

[clever crag]

you dont know what happens in future

[dense leaf]

wha

[woeful pasture]

I work towards the future so I'm a bit more inclined to be saying things

[junior holly]

I’m md5s brother and I can say with confidence we are always moving towards the future

Here at spigot mc

[clever crag]

ok

[dense leaf]

Miles IS the future

[woeful pasture]

I am the future

[dense leaf]

He works for OrbiteMC which quite literally is the future

[remote kiln]

тщ

[clever crag]

sorry

[remote kiln]

I work with 1.8 most of my time

[dense leaf]

💀

[clever crag]

i need to replace it with builder?

[remote kiln]

hire me I will bring some balance

[clever crag]

i need to replace extends with builder?

[woeful pasture]

Yes you should

[clever crag]

okay done

[late smelt]

When you don’t get your extended type back from any methods then you’ll be having a bad time

[dense leaf]

(SimpleItemStack) stack

[late smelt]

ClassCastException

[woeful pasture]

wait till they figure out about CraftItemStack

[clever crag]

whaat

[remote kiln]

SO, what's the way to redesign it? If it has to be redesigned ofc, looking for opinions

https://github.com/fodded/Minigame-Network/blob/master/Common/src/main/java/me/fodded/common/ServerCommon.java

[surreal peak]

why have a builder that simply delegates to the server common

[remote kiln]

change variables values directly with setters instead of builder?

[surreal peak]

ig

[round fossil]

Builders are good for configuration stages/phases, objects with many immutable references to components (thus no setters) and for nested and layered objects in general ^

[surreal peak]

there is no concept of actually building something

youre just modifying a server obj and returning it

why think in terms of builders

well in this exact code

[round fossil]

yea I mean there is a debate whether void methods should just return their receiver, cuz its more convenient

but eh, it makes it less expressive and yada yada

[sly jackal]

yikes, that would make setters actually useful

[junior holly]

I meant to post it in help dev lol

[junior holly]

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

Felt pressured by steaf kek

[jagged rock]

what's the problem with making ARegistry non-abstract

It would allow for the creation of registries without a concrete implementation and not force inheritance for something like a facade

[woeful pasture]

Real registries look like
https://github.com/PineappleDevelopmentGroup/Pineapple/tree/dev/pineapple-utils/src/main/java/sh/miles/pineapple/collection/registry

[sly jackal]

Complicated* registries

ig its cool to be able to freeze it though

[woeful pasture]

Theyre pretty easy the only difference between the two is whether you can write to them

[sly jackal]

right

but like the key class seems kinda pointless

and once again I fail to see why its even useful to have the interface if everything is gonna use the abstract class anyways

[woeful pasture]

There for specific cases

There have been projects where I actually implement Registry

Nice to have a contract and standard just laying there

[dense leaf]

nms registries ❤️

[random yacht]

Sometimes it's better to hide implementation details. You can very easily, at a quick glance, tell what that Registry class is capable of doing

In that same realm, you can hide methods you don't want the API consumer to see. Maybe AbstractRegistry has a method called clearEntries() that you don't want to be visible to the Registry interface because it's meant to be used for internal reasons. The Registry stuff Y2K shared above doesn't do this, but it's certainly not uncommon. CraftEntity has a public getHandle() method but you don't ever see that from Entity because it's not exposed, the API doesn't need to see that

but there's only one implementation of Entity and it's CraftEntity

[sly jackal]

hmm

i see, that makes a little more sense

especially from like an API perspective

[random yacht]

It's part of the reason I appreciate C++'s header files vs implementation files

[sly jackal]

yes because that kinda forces you into that kinda design

[random yacht]

Granted you can do some ugly shit and implement things in the header, but you can also use them as contracts and type definitions which is great. Easy to quickly overview what you're working with

[dense leaf]

kind of matching meme i just found

[surreal peak]

oop 🤮

[desert ore]

I believe registries are Registry and SimpleRegistry in NMS

[surreal peak]

but they are not actually simple

[dense leaf]

Yes

[dense leaf]

They are :)

[desert ore]

Aren't registries just fancy hashmaps tho

[dense leaf]

yea

[dense leaf]

https://github.com/mcbrawls/blueprint/blob/fdbbc001c31a7dadab45788f58cbd13c34d807a6/src/main/kotlin/net/mcbrawls/blueprint/region/serialization/SerializableRegionTypes.kt How is this not simple?

[dense leaf]

cc @desert ore ^

[surreal peak]

just wanted to say that most of the time just prepending Simple isnt actually what makes it simple

[desert ore]

Who even thought of using that Simple-prefix instead of the Impl-suffix

[dense leaf]

[desert ore]

I'm gonna be fair with ya, I myself use the Impl-suffix and the I-prefix for interfaces with one single impl

[dense leaf]

IPlayer when

[desert ore]

Not like Craft-prefix is the fanciest thing

I wanna write a Bukkit impl on top of forge one day

[surreal peak]

just prepend Standard, like choco does it

StandardBukkit

[dense leaf]

i hope you step on a lego brick

[surreal peak]

wasnt ike working on some fabric/bukkit thing?

[desert ore]

Like Choco does it?

[desert ore]

Ye

[sly jackal]

I-prefix for interface are for people without imagination

[surreal peak]

i prefer no interfaces at all

[hasty lotus]

☠️

[sly jackal]

i use interfaces when I for sure have multiple things inheriting from it

[hasty lotus]

I use interface, then an abstract class and then normal classes

[sly jackal]

always?

[hasty lotus]

Not always

[desert ore]

Brother has never heard of the bukkit services system or forge capabilities

With bukkit services you can definitely have two different implementations, even tho it depends on the usecase, with forge capabilities you will almost never meet two classes implementing one interface

[sly jackal]

from the sounds of it I dont wanna either

[desert ore]

Bukkit services are actually a really great thing

[surreal peak]

is the java service loader a joke?

[hasty lotus]

Why do u think that

[desert ore]

I use it for my library, I don't want to force users to install it as a plugin, so I let people (myself and nobody else) shade it and call the init method, then if the lib hasn't been initialized before it registers itself to the service manager, otherwise it just returns. That way I can make sure that only one instance of the lib is loaded, even tho I believe relocation could cause Issues

And generally it's a pretty flawed system

Version mismatch

[round fossil]

modules!!!

[junior holly]

Hey con do you wanna put your 2 cents in as well?

[junior holly]

.

[round fossil]

IRegistry and ARegistry, get rid of the one letter prefixes

also im not sure if u wna create a bunch of subclasses of the registry class

isn't it better to utilize composition here?

and yeah feels like the abstract class is a bit redundant (someone said that alr I believe)

[junior holly]

Yes been said before

Idk I just felt like the option is nice to provide

[round fossil]

ah well, im sorry to crush ur spirit... but its not

[junior holly]

A registry is quite simple, but that doesn’t mean someone else might not have a different approach

[round fossil]

subclass-superclass relationships are most prone to become tightly coupled

[sleek shard]

hi conclube

[round fossil]

that is why any usage of abstract classes must be done w care and consideration

[round fossil]

helo

[junior holly]

Hmm

So it’s either drop the interface or drop the abstraction basically?

[round fossil]

this is why for example AbstractMap, AbstractCollection etc try to enforce AS FEW METHODS as possible on subclasses

for example AbstractMap::entrySet iirc being like the only mandatory method u need to override

[round fossil]

i mean the interface could be handy

if u wna define mutable registries as well as immutable ones

or concurrent vs non thread safe ones etc

but the abstract rn serves little purpose

i kind of always go by having interfaces over classes unless there's a good reason not to

ofc if u work in specific ecosystems it may prove to be a poor decision, lets say mc modding

and ofc u dont need to enforce an interface on a record for example

but it often comes quite handy

[sleek shard]

https://github.com/ignPurple/seasons

@junior holly if you want to see my current code for seasons

[surreal peak]

i prefer adding a Plugin suffix to the main class

and why so many methods in your main class, those can be all in onEnable

remember i am biased by using c

and try to make your try scope as small as possible

[desert ore]

what's a scope

[surreal peak]

they learn that at first year programming

[desert ore]

no hables ingles

[surreal peak]

area covered by try block

[cobalt trellis]

but in this specific case?

[desert ore]

ah, wait, scope?

[cobalt trellis]

I'd honestly not define scope as that, but whatever - I'm self-taught anyways

[desert ore]

I remembered what it is

[sly jackal]

well thats just one scope

but scope in general in java is anything between 2 {} badboys

[desert ore]

ye I remembered

[cobalt trellis]

I've done too much maven that I first think of the scopes used in maven artifact resolution heh

[desert ore]

lol

I don't like maven

[cobalt trellis]

Figure what: Gradle uses the same concept.
Sure, it calls them configurations, but it's the same thing.

[surreal peak]

Pattern

[surreal peak]

actually about everywhere

[junior holly]

I will look later on a date today

[surreal peak]

if Modifier.isFinal(field.getModifiers())

[sleek shard]

Fairs yeah forgot to do that

[surreal peak]

PlayerEvent?

[sleek shard]

Custom EventBus

[surreal peak]

you tell me whats wrong here

[sleek shard]

go ahead

Plugin plugin?

ConcurrentHashMap?

[surreal peak]

why inject a plugin when all you need is a logger

[sleek shard]

Fair

[surreal peak]

consider a situation where youre instantiaating one such object and you have no plugin anywhere close

[sleek shard]

Yea fair enough

[sly jackal]

but what if you have no logger either

[surreal peak]

and i would inject config here as another parameter just to decouple from that specific class, but whatever

[surreal peak]

then youre dead

[cobalt trellis]

(just use SLF4J but sssh)

[surreal peak]

Bukkit.getWorlds().get(0)

[sly jackal]

just have a static logger 😄

[surreal peak]

jokes aside, wtf is useUnusualXRepeatedCharacterHexFormat() from adventure

[sleek shard]

real

[surreal peak]

what real

[sleek shard]

Oh yeah testing purposes

[sleek shard]

like wtf is that naming

should be default imo

[cobalt trellis]

suuure.

[sleek shard]

^

[junior holly]

We love adventure conventions

[alpine anvil]

its the weird x format bungee hex code

[surreal peak]

which goes like?

[alpine anvil]

its like §x§r§r§g§g§b§b

[surreal peak]

wtf

people havent heard of ways to translate that?

[junior holly]

That’s why we useUnusualXRepeatedCharacterHexFormat duh

[surreal peak]

why is it calling itself unusual, is there another way to translate it then?

[dense leaf]

minimessage

[sly jackal]

thats the only way to represent hex colors in legacy strings

[sleek shard]

^

[junior holly]

But minimessage doesn’t support legacy formatting, so I mean you kind of need both don’t you?

[dense leaf]

components

[junior holly]

I should look more into components

[winged blaze]

what do u guys thin about this? 🥵

ill make build return optional probably so that if wrong serialization data is provided it wont throw anything and just safely return empty value

[random yacht]

Map<String, Object>

[random yacht]

I think it's better to throw an exception and catch it in the deserializer tbf

It's unexpected input. That's exceptional

[late smelt]

Hey look it’s the config api

[random yacht]

At least with an exception you can also pass through a nicer message to show to the end user. Remember that you don't always have to print the stacktrace. You can choose to do exception handling that just prints out an error message. Notably, an error message that you specified in the constructor of the exception

Or if some other consumer of that API wants to handle the exception differently, it can

[woeful pasture]

that input was quite exceptional 🧐 ☕

[random yacht]

Not that kind of exceptional lol

[woeful pasture]

well that's not very exceptional

[winged blaze]

❓ thats a spigot configuration api requirement

[random yacht]

Well sure but you could at least use a MemoryConfiguration as a wrapper

Then invoke getValues() on it when you need a Map<String, Object>

[winged blaze]

yea ill do that actually, i dont wanna replace the map constructor with a static method just to get optionals

[random yacht]

At least that way you have some nice getInt() values and whatnot. Unless all you're doing is set() calls in which case it doesn't matter I guess

[winged blaze]

ive never heard of MemoryConfiguration actually

ive used the config api like once in my life

[random yacht]

MemoryConfiguration is a base of things like YamlConfiguration. It's a Configuration spec that just holds its values in memory

[late smelt]

Its a fairly deep tree

[random yacht]

ConfigurationSection -> Configuration -> MemoryConfiguration -> FileConfiguration -> YamlConfiguration

But you don't need a FileConfiguration. You can just use a MemoryConfiguration. Comes with all the benefits of ConfigurationSection though, including its getters and serialization properties

[winged blaze]

why does JavaPlugin have a FileConfiguration getter then

[random yacht]

Because it uses a YAML file

[late smelt]

Don’t forget MemorySection!

[random yacht]

Its configuration is an actual file configuration on disk

[winged blaze]

so like its not cached in memory as a map?

[random yacht]

It is!

YamlConfiguration extends MemoryConfiguration

[winged blaze]

oh makes sense now

i thought it was the opposite

[random yacht]

All FileConfigurations are MemoryConfigurations, not all MemoryConfigurations are FileConfigurations

Ye

[surreal peak]

storing function pointers in an enum will always come back at you

because at some point youll need constructor params that arent available

[woeful pasture]

Looking for ways I could improve this dependency loading system. I'm not liking how many times I have to look at the graph of each dependency to check for circles etc, but idk how else I'd ensure no circles occur
https://paste.md-5.net/nofecagipi.java
https://paste.md-5.net/zaporiduqo.java

Data Classes incase you want context
https://paste.md-5.net/umedajazeg.css
https://paste.md-5.net/tobewahiqo.css

[surreal peak]

first thought: bitset for all dependencies

havent looked at the code

maybe some explenation instead of dropping two files? 👀

[woeful pasture]

It's all files related to dependency loading I didn't wanna give more so it was more of a focused critique, pero whatever

https://github.com/OrbiteMC/OrbitMC/tree/1.21-cutting/orbitmc-server/src/main/java/io/github/orbitemc/impl/plugin

[winged blaze]

This follows the configuration API spec so there's always one constructor that takes a map

But I've got rid of that whole system anyway, I can just deserialize directly

[muted sentinel]

Is this looking better?

package me.arkallic.core.managers;

import me.arkallic.core.User;
import me.arkallic.core.handlers.FileHandler;

import java.util.HashMap;
import java.util.UUID;

public class UserManager {
    private static final HashMap<UUID, User> playerData = new HashMap<>();
    private final UUID playerUUID;
    private final User user;

    public UserManager(UUID playerUUID) {
        this.playerUUID = playerUUID;
        user = playerData.computeIfAbsent(playerUUID, _ -> new User(playerUUID, new FileHandler("players", playerUUID + ".yml")));
    }

    public void playerJoin() {
        if (!getUser(playerUUID).hasData()) {
            getUser(playerUUID).setPlayerRank("Player");
        }
        getUser(playerUUID);
    }

    public void playerQuit(UUID playerUUID) {
        getUser(playerUUID).save();
        playerData.remove(playerUUID);
    }
    
    public static User getUser(UUID playerUUID) {
        return playerData.get(playerUUID);
    }
    
    public boolean hasUserData() {
        return playerData.get(playerUUID).hasData();
    }
}```

just fixed the static. dont mind that lol

[late smelt]

Why would each user have its own manager

It isn’t a manager then

[round fossil]

I mean it can make sense ehrmm

[woeful pasture]

Feel like my suggestion of manager pattern was ignored

This is weird amalgamation of manager pattern

[muted sentinel]

no y2k. i just didn't know how to do it lol

i was trying to do it myself

[round fossil]

Yours is somewhat of a failure thats true, but its not too bad

im sure y2k is willing to put u on the right path again

[muted sentinel]

hes always busy. im redoing it.

[round fossil]

Well I think you’re asking a bit too generic as well, if you can pinpoint what you’re unsure about and don’t understand, its easier for people that don’t have as much time to just answer it ^^

Best of luck on ur overhaul

[woeful pasture]

I gave an example of manager pattern earlier I believe

I'll see if I can find it

[woeful pasture]

.

[muted sentinel]

User.java

package me.arkallic.core;

import me.arkallic.core.handlers.FileHandler;

import java.io.IOException;
import java.util.Set;
import java.util.UUID;

public class User {

    private final UUID playerUUID;
    private final FileHandler fileHandler;
    private Set<String> playerHomes;
    private String playerRank;

    public User(UUID playerUUID, FileHandler fileHandler) {
        this.playerUUID = playerUUID;
        this.fileHandler = fileHandler;
        if (fileHandler.getFile().exists()) {
            this.playerRank = fileHandler.getConfig().getString("Rank");
            this.playerHomes = fileHandler.getConfig().getConfigurationSection("Homes").getKeys(false);
        }
    }

    public void save() {
        try {
            fileHandler.getConfig().save(fileHandler.getFile());
        } catch (IOException e) {
            throw new RuntimeException(e);
        }
    }

    /**
     * Getters
     */

    public Set<String> getPlayerHomes() {
        return playerHomes;
    }

    public String getPlayerRank() {
        return playerRank;
    }


    /**
     * Setters
     */
    public void setPlayerHomes(Set<String> playerHomes) {
        this.playerHomes = playerHomes;
    }
}```

UserManager.java

package me.arkallic.core.managers;

import me.arkallic.core.Core;
import me.arkallic.core.User;
import me.arkallic.core.handlers.FileHandler;
import org.bukkit.Location;
import org.bukkit.World;

import java.util.HashMap;
import java.util.UUID;

public class UserManager {
    private final HashMap<UUID, User> playerData = new HashMap<>();

    // you can now write methods to add and remove etc or any other methods you'd need to make it easy to obtain and modify whatever PlayerData you want.

    public void registerUser(UUID playerUUID) {
        User user = new User(playerUUID, new FileHandler("players", playerUUID + ".yml"));
        playerData.put(playerUUID, user);
        Core.getInstance().getLogger().info("Registered UUID: " + playerUUID);
    }

    public void unregisterUser(UUID playerUUID) {
        playerData.get(playerUUID).save();
        playerData.remove(playerUUID);
        Core.getInstance().getLogger().info("Unregistered UUID: " + playerUUID);
    }

    //Get the user data
    private User getUser(UUID playerUUID) {
        return playerData.get(playerUUID);
    }

    
    public void addHome(UUID playerUUID, String home, Location location) {
        double x = location.getX();
        double y = location.getY();
        double z = location.getZ();
        World world = location.getWorld();

        getUser(playerUUID).getPlayerHomes().add(home);
        getUser(playerUUID).getPlayerHomes();
    }

    public void removeHome(UUID playerUUID, String home) {
        getUser(playerUUID).getPlayerHomes().remove(home);
    }

}```

i think i get what you're saying now

I just took the // you can now write methods to add and remove etc or any other methods you'd need to make it easy to obtain and modify whatever PlayerData you want.
and kept reading it over and over

[junior holly]

For the player homes field in User, wouldn't it make more sense to store a set of locations?

Oh I see

the string is basically a reference to the location right?

[muted sentinel]

I haven't gotten to that yet, i was trying to figure out how to work UserManager work.. i was over complicating it again

yes

Because if i register the user then i can use the methods for stuff, that wasn't working in my head for some reason

[hasty lotus]

Wouldn’t it make more sense to put the remove and add homes into the User class

And then have like a lookup player function in the manager class

[surreal peak]

use dependency injection

also it doesnt make sense a user stores a file manager, filemanager (or any other kind of storage) should be aware of how to save users, now the other way around

imo, the user manager should be aware of a storage abstraction, which on its turn saves users

[hasty lotus]

I agree. The user should just store attributes and nothing like super functional if u know what I mean

[woeful pasture]

I fundamentally disagree about a manager using dependency injection to be passed around. But that ends up being personal choice so take your pick

[surreal peak]

i already forgot what i said that for

[woeful pasture]

Lol

Real

[woeful pasture]

real

[surreal peak]

yes buddy

[terse spoke]

https://github.com/hostadam/timers/blob/master/src/main/java/me/hostadam/timers/Timer.java

Improvements? And I’d love to be completely humiliated in terms of wrongdoings

Hit me

It’s an old project fyi

Also it’s never been tested lol its just the start of an API I never ended up using

[surreal peak]

dont expose a list, dont expose the task, dont expose a consumer

[terse spoke]

fair enough

[muted sentinel]

Scale 1-10 (10 being worst)

    public void setDefaults(UUID uuid) {
        FileConfiguration cfg = getUser(uuid).getFileHandler().getConfig();

        //Add default data to users data
        if (!cfg.contains("Settings.Rank")) {
            getUser(uuid).setPlayerRank("Player");
        }
        if (!cfg.contains("Settings.Allowed-homes")) {
            getUser(uuid).getFileHandler().getConfig().set("Settings.Allowed-homes", Configuration.getInstance().allowedHomes);
        }
        if (!cfg.contains("Homes")) {
            getUser(uuid).setSection("Homes");
        }
    }```

[round fossil]

4

[lunar vale]

0

[round fossil]

imo

[surreal peak]

2

[muted sentinel]

I have no idea what else i can do...

[surreal peak]

1 variables
2 why is a user aware of its config handler
3 why does the config handler expose its internal config
4 why is there another Configuration
5 hardcoded string literals

[muted sentinel]

The UserManager should be changing the User correct?

[surreal peak]

wdym by changing

[muted sentinel]

User is a player data file. UserManager is the manager file for User, We want to change the values in the user file from userManager?

this is in the Manager file btw.

[surreal peak]

why dont you call it UserData then?

[covert marsh]

About... a 6 i would say. There is room for improvement, to the point where this method might even completely vanish.

Nvm, more like a 7. I just realized that you are mixing properties being stored in fields and fileconfigurations.

[round fossil]

yea I dont think its that bad

[covert marsh]

Maybe a 6 because its readable

[round fossil]

its more like smile said, room for improvement :>

[covert marsh]

Conclube being diplomatic again. But yea, room for improvement is probably the right desc.

[round fossil]

haha well smile so are u, alittle :^)

[late smelt]

Who uses 10 as the worst

Just sayin

[round fossil]

wait

fuck I saw that now as well

[winged blaze]

I do what I was taught to do when working with databases:
I have a UserData class with all user properties and User class which contains an unique identifier in addition to that

I don't guarantee this is the best solution maybe others have different ideas

[woeful pasture]

idk if this is "proper" design but personally I would query the maximum punishment ID on startup and store it in memory

and increment it as the ID every time a punishment is created

that is a fair critique.

personally I wouldn't do everything off an int ID though

I would use the player UUID and increment a ID number for each player punishment

any method of incrementing this ID could cause issues though in which if two punishments were made at the same time on multiple different servers and sent off to the DB in one go you'd have a 2 punishments with the same ID

[surreal peak]

why return a saved punishment if you just saved it?

think about it

[hasty lotus]

Looking for some suggestions for my project https://github.com/PulseBeat02/MurderRun. One thing I was wondering was I wanted to find a way to make the Command classes neater, maybe by using some Holder<T> class to hold/check null values? Besides that, I think checking on the other code would be great to have another pair of eyes.

[sly jackal]

wdym by hold and check null values

do you mean Optional?

[hasty lotus]

No, you shouldn't pass Optionals around so i was thinking of creating my own holder class with its own null logic, etc

[sly jackal]

also your onEnable and onDisable dont have to be this giant list of function calls, especially if you only use it once and its just one line of code

[sly jackal]

what would be the difference

[hasty lotus]

could possibly do some chained stuff, for ex if null send a message to audience, etc

[sly jackal]

you shouldnt not pass optionals around because they are implemented poorly, but because the principle

the way you are describing sounds like Optional.orElse()

[hasty lotus]

I guess I could send the error message to the audience and return null, but idk

[sly jackal]

well this might be more personal preference but the issue I have with Optional is that it doesnt change anything

like you are either returning null or you arent

you will have to check either way, if the result can be nullable

maybe if you have a more concrete idea about what a holder is, if not just a pointer, then we would have something to talk about regarding that part

[woeful pasture]

Think the main idea is forcing the exception to be handled but still optional in Java are poorly implemented and don't even do well at that

[sly jackal]

yeah

I guess I would have wanted the nullable annotations to be non arguable

and I guess optionals are a way to do that

but I find it to be cluttery

[woeful pasture]

They still aren't perfect but better to beable to implement in kotlin.

val result = when(option) {
 is Some -> option.some()
 is None -> throw Exception
}

[sly jackal]

yes

which is kinda reducing it to a glorified null check anyways

but you have that compiler backing you up

[woeful pasture]

I don't think you need optional.in kotlin anyways

Because of how well they handle null safety

[sly jackal]

yeah i wouldnt know

never touched it in my life

[woeful pasture]

As far as java goes for optional to make sense it'd need to essentially compile down into a nullcheck

Or some other implementation as a direct language feature

[sly jackal]

for me to see any improvements it needs to be a direct language feature

with some sort of syntax or whatever

because otherwise its not more reliable than null checks and annotations and not less verbose than optionals

[hasty lotus]

Yeah I wanted to not use optional cause of its limitations

But are there any other suggestions u guys think

[sly jackal]

im not a fan of the "pointless" interfaces but people let me believe its the good thing to do

also whats a config manager vs a config mapper

[hasty lotus]

Wait why lol

[hasty lotus]

I named it badly. But the config manager is the plugin configuration. The config mapper is for storing data in JSON

[sly jackal]

because why do you have an interface if it only has one logical implementation

[hasty lotus]

It’s useful for other purposes, like creating a mocking implementation

[sly jackal]

I mean I guess the other way to see it is that your abstract is poorly named

[hasty lotus]

Ya

[surreal peak]

why is everything final

[sly jackal]

because it seems to be used for writing json to file

[hasty lotus]

Final good 😊

I have an IntelliJ plugin to auto add it

[surreal peak]

sure buddy

why arent your functions final

[sly jackal]

ah I had not considered such a usecase too..

[hasty lotus]

Making your functions final causes them to not be overrided, which is a completely different purpose of final

[sly jackal]

why arent your classes final 🤓

[hasty lotus]

It’s not like variables where you don’t reassign them

[hasty lotus]

lol

[sly jackal]

final interface when

[desert ore]

pretty much the same thing tho?

[hasty lotus]

Wdym

[desert ore]

you can't reassign the method

[surreal peak]

tell me the real reason why youre using final

https://tenor.com/view/popcorn-gossip-listening-suspense-drama-gif-13117366258359790345

[hasty lotus]

Cause obviously its 0.00001% faster lmao

Jk

[sly jackal]

final in constructor arguments seems pointless

[surreal peak]

it isnt

[sly jackal]

well it seems like it

[surreal peak]

didnt reply to your comment btw

[hasty lotus]

Sure but usually the variables are enough for most developers

[sly jackal]

oh

[hasty lotus]

Yea and same for exceptions

[sly jackal]

also abstract classes without any abstract methods seems a little criminal

but I guess maybe there are usecases for it

I suppose its just more about the principal of being abstract

[hasty lotus]

Maybe for implementing interfaces and then not defining some of their method implementations

Idk

[sly jackal]

oh right

thats a thing

[round fossil]

abstract classes are amongst the hardest tools to use correctly

[sly jackal]

why

because they are very vaguely in between classes and interfaces?

[hasty lotus]

It’s like a hybrid almost. When I use them I define variables within the class so I can “give more implementation detail” to my sub-classes if that make sense. Now that I have access to my variables, I have more freedom to do stuff and implement some methods. But maybe leave some for the children classes to implement

That way my children classes can just extend the abstract class and get the advantages of having access to those variables, etc

[round fossil]

I mean yes sort of, abstract classes usually imply tightly coupled parent-child class relationships, they also imply state and behavior inheritance

differs from interfaces that primarily promotes type inheritance but not implantation inheritance

[desert ore]

abstract classes are like interfaces but different

imagine if we had abstract variables

[hasty lotus]

How would that work

[desert ore]

no idea lol

[sly jackal]

we have that they are called getters and setters

[desert ore]

no no no

that's boilerplate

[sly jackal]

depends on if some class overrides it

[round fossil]

Hows that supposed to work

Sounds like you’re explaining property semantics right now

[desert ore]

public final abstract variable

[round fossil]

how would that work

[desert ore]

it's a constant that has to be defined by all classes

[winged blaze]

It simplifies the experience

Just like streams

[dense leaf]

abstract class KotlinIsGreat {
    abstract val isKotlinGreat: Boolean
        get() = true // it's even got defaults
}

hehe

[sly jackal]

what experience

[winged blaze]

And nullability annotations aren't a part of java

[round fossil]

what sounds so contradictory

[surreal peak]

whats the point of that

[dense leaf]

what

[surreal peak]

what what

[dense leaf]

what what what

[round fossil]

legit what I said, property semantics 🗣️

[winged blaze]

Less boilerplate and neat functions like orElse

Easier to read

And optionals force you to handle them

[sly jackal]

yeah

thats the only part I agree with

and maybe less boilerplate too

[surreal peak]

just use kotlin and you dont have those issues

[dense leaf]

real

[sly jackal]

or not

[surreal peak]

just use scala and you dont have those issues

[winged blaze]

Kotlin is a mess

[dense leaf]

ew

[winged blaze]

I don't like 5 megabyte jars

[dense leaf]

my jars are 40mb

[surreal peak]

we are 2024, who cares about a few megabytes

[round fossil]

sure, but its still yanky, there's no static subtyping for presence/absence of values

[winged blaze]

Can u elaborate in human words

[dense leaf]

premium plugins

can only be like 4m

[surreal peak]

lol

[round fossil]

well imagine
Optional<T>
PresentOptional<T> extends Optional<T>
AbsentOptional<T> extends Optional<T>

present ensure a value is there, absent ensures a value is not there

[surreal peak]

whats the point

[dense leaf]

average java boilerplate

[round fossil]

kotlin has this built in for example with T and T?

[dense leaf]

when you can just T? in kotlin

[round fossil]

yep

[winged blaze]

But if it's present you don't need optional at all tho?

[desert ore]

I call them Nullable<T> and NotNull<T>

[round fossil]

sure

[dense leaf]

bro got da annotations

[round fossil]

thats not the point

[winged blaze]

Can you show me a code example perhaps

How'd you see that

[round fossil]

lets say u have a method

Optional<R> someGetter()

and some subclass

@Override
PresentOptional<R> someGetter()

where like, a certain subclass will always yield a value

then u dont have to at runtime evaluate the optional

u can just invoke unwrap() or get() w/e

this is one example

[hasty lotus]

Whyyyyyy there are already annotations with the same names lol

[desert ore]

for that exact reasno

[round fossil]

but it should demonstrate why Java's optional is inferior to kotlin nullability semantics

[surreal peak]

keep in mind that dynamic dispatch is more expensive than a static call

[dense leaf]

factual

null chaining my beloved

[round fossil]

I mean yea you're trading off memory and speed by using Optional

[hasty lotus]

Ya optional dookie that’s why if u want smthing good u have to make your own Holder class whatever

[round fossil]

unlike kotlin :>

[round fossil]

🥲

well if they made Optional fast, giving it some jit boosts maybe then, and reworked it completely :^)

[surreal peak]

🤔

[hasty lotus]

Yea

Do you know a library that has good optional substitute

[round fossil]

I mean there are these functional java libs

but they ehm... slow

[surreal peak]

for what 👀

[round fossil]

I just use @Nullable @NotNull I pray to jesus

or well maybe not jesus, but someone

[hasty lotus]

Yea true

[hasty lotus]

You used checker at one point

But did you stop cause it was too much

[round fossil]

checker?

checkerqual ?

yea I mean they have some good ones

I just shove in googles findbugs, jetbrains annots and checkers

DefaultQualifier is nice from checkers, and Language from jetbrains is nice

[surreal peak]

looks like its a good thing yall didnt have to use c

just imagine all those fancy things there

at some point you could ask yourself why you even need them

[round fossil]

ah yes, well I had one assignment in it

c and assembly

never again

Rust is the lowest ill go

[hasty lotus]

Yes

[surreal peak]

odin is fun

its like c but with go style semantics

anyways some are you are really diehards for java

[hasty lotus]

I dont think any of us code Java 24/7 lol

But we just want to adhere to good practices and stuff just like how we would for any other language

[round fossil]

I prefer haskell

Or well rust, go and javascript are nice

[hasty lotus]

C++ cool

[surreal peak]

WAIT

you are one of steels kind

[round fossil]

yeaaa truee i just havent touched it a lot :(

[surreal peak]

c++ is awful imo

[round fossil]

Well its mainly cuz I studied category theory, group theory and discrete mathematics that branches into abstract algebraic structures

[surreal peak]

brrr

[terse spoke]

https://pastebin.com/W1xBQ5jF

roast my code please

i've written this with no testing

in 45 minutes

[surreal peak]

(int) Math.ceil is useless

[terse spoke]

why

[surreal peak]

already happens with an int cast

uh wait

mb, thats floor, its fine

[terse spoke]

ah

For explanation

The Menu system is designed to have these key features:

• Pagination
• Permanent items that are persistent across pages.
• Dynamically adjusted slots. If you have a list of objects to display, it automatically adjusts the slots they are placed in to assure that they cover only air slots, and then adds more pages if there are more items than there are air slots.

In the example here, I have the border & book be permanent items.

And air slots in the middle that are "available" to dynamic items, in this case, it's a list of the first 36 materials in the Material list

And it distributes it automatically across the (6) pages that it calculates is required

[jagged rock]

You're doing too much in this class

(and using a static map as a registry is kind of disgusting)

In short my approach to this problem is to break a menu down into layers

We have a "base layer" which holds the border and all the permanent items, and a "paginable layer" which handles all of the pagination logic

Like for example if the page is 0, it doesn't render an item on the "previous page" slot

I go a step further and split a "paginable layer" into multiple "paginable areas", each with its own list of slots and items to match

So that for example if you want a battle pass menu where you have a row for each tier, you have 3 separate areas under the same layer

[terse spoke]

@jagged rock can i dm you?

[hasty lotus]

Post your new code here

[terse spoke]

yeah idk just easy to reach xd wasnt bothered making handler class when making this

[terse spoke]

Isn’t that basically what I’m doing except maybe not directly specifiying them as layers?

[jagged rock]

Problem is if you want multiple "paginable layers"

[terse spoke]

I’ve added in an option to set a fallback item that is in place of e.g. the previous page item if there is no prev page

[jagged rock]

like 2 independent sections

[terse spoke]

i get what ur saying, but also not XD

[terse spoke]

U have a pic of the menu?

[jagged rock]

I've had to use it before, no concrete example but sometimes you'll need 2 different pages in the same menu :)

[terse spoke]

hm

ok well thank you

[sly jackal]

I usually work on those kind of abstractions when I need them because it's simply faster and easier to not worry about it beforehand

but ideally you have to find some middleground where actually converting the code to use the new stuff is not too hard to do

this could simply be the step of just splitting the stationary items from the moving ones

[clever crag]

Non abstract method

i mean that it doing more than method name

You understand me?

[sly jackal]

also who tf scales with 10 being the worst

that scaling is 0/5

[remote kiln]

brother not just bullied his code but his scaling system too

f

[muted sentinel]

That's why I ask 🤷

[viral mantle]

?paste

[rich fog]

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

[untold jay]

When I depend on my plugin's main class, should I dependency inject my plugin or just use a static getter

I'm asking because I don't understand the point if what you're depending on will not change

[woeful pasture]

Either works

I prefer the static getter others will die by Depenendency Injection

Do what you like and ignore whoever will try to argue the other way

Because there are merits to both

[winged blaze]

Preference

Singleton is ok for plugin instance cuz there's one per program

[untold jay]

I also have the tendency to prefix a class name like: MyPluginConfigManager if it's tightly coupled to MyPlugin

is that fine?

[woeful pasture]

Personal preference

I always just name it Config or ConfigManager

[jagged rock]

If I expect the plugin to be hooked into, or other plugins to share a common name, I suffix most things with the plugin name

SkyblockLocation, SkyblockPlayer, SkyblockPlayerTracker, SkyblockFileManager

[untold jay]

What if I have a very long name already

And my plugin name is long

NoonieManagementPostgreSqlPlayerMutePunishmentRepository

[dense leaf]

need some abstract factories for that

[untold jay]

Yes I do

Just a factory, not an abstract one

[dense leaf]

nah you need an AbstractNoonieManagenentPostgreSqlPlayerMutePunishmentRepositoryFactoryConversionStrategy

[untold jay]

😂

[sly jackal]

I hope this is 100% serious

so we can train gpt5 on it

[dense leaf]

gpt4o got this: NoonieManagementPostgreSqlPlayerMutePunishmentRepositoryHistoricalViolationTrackingAndArchivalServiceIntegrationManagerDelegateFacadeImpl

[sly jackal]

lol

what prompt

lmao the Impl at the end xD

which implies this is normally an interface that could be implemented multiple ways

[sly jackal]

right

I meant more like train it in a way where it thinks this is a genuine good suggestion

[dense leaf]

lmao

[sly jackal]

thats why we need more good class name like those ones

[gilded gate]

https://github.com/Psikuvit/BetterStats

[pine grail]

💀

what even IS that

[gilded gate]

?

wdym??

is quite clear

[rancid fern]

https://github.com/Psikuvit/BetterStats/blob/master/src/main/java/me/psikuvit/betterStats/Utils.java#L20 Modify the armor stand in the consumer argument

https://github.com/Psikuvit/BetterStats/blob/master/src/main/java/me/psikuvit/betterStats/Utils.java#L31 int text is missleading

[gilded gate]

fixed both

[rancid fern]

https://github.com/Psikuvit/BetterStats/blob/master/src/main/java/me/psikuvit/betterStats/Utils.java#L37 use a for i loop instead

[gilded gate]

I am more concerned about the PlayerStats class should I use it in the enum class?

[rancid fern]

enum class?

[gilded gate]

Stat

btw fixed

nvm nvm

is the static class PlayerStats good?

[rancid fern]

could work on the name a bit

[gilded gate]

StatsUtils?

[rancid fern]

also you really don't need a get/set health/max health etc

just a getter and setter accepting the stat type

[gilded gate]

yeah I know forgot to remove them

[rancid fern]

also utility classes should not have an accessible constructor

and they should be final

[gilded gate]

there is no constructors in my utility classes

[rancid fern]

There is always an implicit no args one

[pine grail]

I forgot I am in the code review channel

[sly jackal]

so then you just do a private MyUtils() {}?

or what

[woeful pasture]

yeah pretty much

I usually thow an exception in there

but really private MyUtils(){} works

[sly jackal]

what does that look like with an exception

sounds like extra work

unless you are inside of that class and forgot about your own private constructor

in which case skill issue

[rancid fern]

yes

[surreal peak]

or just dont bother

[untold jay]

private UtilClass {
throw new UnsupportedOperationException("can not instantiate utility class");
}

In my opinion it is extra work

If I remember correctly it was in an Effective Java book as good practice

[random yacht]

It's because you can still reflectively access and construct the class if the constructor is private

but really... if someone (or even you, as the maintainer) as reflectively instantiating your util class that contains nothing but static methods...

Your code is the problem, not the exception being thrown lol

[clever crag]

@lombok.UtilityClass

Automatically creates constructor and set static to all fields and methods

[dense leaf]

[clever crag]

Xd

[dense leaf]

is extending a bukkit/nms entity class using kotlin delegation acceptable for providing custom functionality which cannot be achieved with events? e.g.:

class CustomTextDisplay(craft: CraftTextDisplay) : TextDisplay by craft {}

[surreal peak]

probably

[clever crag]

I think no

[dense leaf]

would be nice getting some definitive answers

[rancid fern]

yesn't

[dense leaf]

istg

[alpine anvil]

Maybe

Also I'm gonna add

?xy

[rich fog]

https://xyproblem.info/

[dense leaf]

fairs

making an item display which is effectively a bullet flying in a straight line which needs to do something on every tick and change some behaviour

the ticking thing can be done with a scheduler but other stuff can't really

wait displays don't tick nvm 💀

[alpine anvil]

Transform it in a 1 tick scheduler

[woeful pasture]

just go and extends NMS

as long you accept things will break

[dense leaf]

i have a lotta things dependent on nms already, which break every other update

so, why not add another one

[sly jackal]

yikes

[late smelt]

Displays do actually tick

The government lied to you

[woeful pasture]

mfw people didn't know displays tick

[junior holly]

Do they tick the same way like a mob does for example?

[woeful pasture]

They tick the same as all entities

But their tick is just 2 if statements

[floral inlet]

https://github.com/Dqmino/HomeSystem

harsh criticism needed

[random yacht]

Your plugin name is shit 😠

Sorry

[floral inlet]

😭

[random yacht]

Too mean

[floral inlet]

seriously tho

I mean

I should have a repository module right

but the plugin's idea is far too simple for that

so I feel like a package is enough

[woeful pasture]

Only thing that is really worth improving is slightly better caching

I personally would save player data when the player logs out

    public boolean loadCache(PlayerHomeDatabase keyValueDatabase) {
        for (UUID player : keyValueDatabase.getKeys().join()) {
            Location homeLocation = keyValueDatabase.getValue(player).join();
            cacheHomeLocation(player, homeLocation);
        }
        return true;
    }

    public boolean saveCache(PlayerHomeDatabase keyValueDatabase) {
        for (Map.Entry<UUID, Location> playerHomeEntry : playerHomes.entrySet()) {
            keyValueDatabase.setValue(playerHomeEntry.getKey(), playerHomeEntry.getValue()).join();
        }
        return true;
    }``` methods like this that always return true are pointless

remove the return type

I also wouldn't hard code messages like you do, but personal preference 🤷‍♂️

[floral inlet]

I dismissed that idea earlier because the players could maybe organize to all leave the server at once which would maybe lag the server?

but that's kinda an absurd idea, so I'll do what you said

[woeful pasture]

that is the part about intelligent caching

if you use something like Caffeine you don't expire as soon as they leave

but a while after they leave

[floral inlet]

o

[woeful pasture]

if they rejoin they end up not being expired

usually I set the timeout to like 2 minutes or so. That way you can't really "lag" the server perse and also if a player quickly disconnects and rejoins for whatever reason you can just reach back to the cache instead of the DB

[floral inlet]

Yeah that makes sense

[random yacht]

The only true issue with that way of caching is in a multi-server setup

If I'm being honest, changes to data should be deployed to the database immediately. /sethome? Database call

Rename your home? Database call

You can (and should) rate limit these commands to only be performed once every 0.5 - 1 second or so, but you're always going to benefit from immediate data updating

[woeful pasture]

that's another great way to do it

I also would add more database options than just Redis. Idk what your exact goal is with this, but personally I would atleast add SQLite

[random yacht]

That was also going to be one of my concerns was Redis as a means of persistent storage. Yes Redis can be used as a persistent storage database, it just often isn't. It's in-memory so it's typically used for either efficient state storage or message brokering