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

1 messages · Page 2 of 1

lunar vale
#

split your anvilutils into more classes

mystic solstice
lunar vale
#

i hate the word util

#

maybe have 3 classes

cobalt trellis
#

The main issue I have with that source file is that you have multiple package-private classes in the same file without them being inner classes

#

Also the AnvilUtils class should be package-private, there are no non-package-private methods on there

mystic solstice
#

We really be programming in Java like it's Rust

#

Does every class really need its own file?

cobalt trellis
cobalt trellis
mystic solstice
#

I have run into a need for this pattern more than once - is there a data structure that contains multiple enum variants, but prevents duplicates and can be set to prevent mutually exclusive options?

cobalt trellis
#

https://github.com/lthoerner/betteranvils/blob/cd1d226ea238f595df06cfb5204b0b7c45ad0b0b/src/main/java/com/lthoerner/betteranvils/AnvilUtils.java#L169-L170 there you have a static non-final field be all uppercase. Another issue with that line is that you access the JavaPlugin instance in a clinit ("static initializer") block wich can be dangerous if the class is loaded too early.

https://github.com/lthoerner/betteranvils/blob/cd1d226ea238f595df06cfb5204b0b7c45ad0b0b/src/main/java/com/lthoerner/betteranvils/AnvilUtils.java#L268-L271 Nullable integer assumed not null (useless auto-boxing, given that according to GH this is the only reference to this method).

https://github.com/lthoerner/betteranvils/blob/cd1d226ea238f595df06cfb5204b0b7c45ad0b0b/src/main/java/com/lthoerner/betteranvils/AnvilUtils.java#L285 Expensive #getItemMeta call that should be cached

https://github.com/lthoerner/betteranvils/blob/cd1d226ea238f595df06cfb5204b0b7c45ad0b0b/src/main/java/com/lthoerner/betteranvils/AnvilUtils.java#L285 Same here.

https://github.com/lthoerner/betteranvils/blob/cd1d226ea238f595df06cfb5204b0b7c45ad0b0b/src/main/java/com/lthoerner/betteranvils/AnvilUtils.java#L320-L346 this list can be pre-baked for improved performance

https://github.com/lthoerner/betteranvils/blob/cd1d226ea238f595df06cfb5204b0b7c45ad0b0b/src/main/java/com/lthoerner/betteranvils/AnvilUtils.java#L350-L378 This should be a field in the enum constants for improved lookup speeds

https://github.com/lthoerner/betteranvils/blob/cd1d226ea238f595df06cfb5204b0b7c45ad0b0b/src/main/java/com/lthoerner/betteranvils/AnvilUtils.java#L384 Reliance on Enum#name for a Material. Use namespaced keys instead

https://github.com/lthoerner/betteranvils/blob/cd1d226ea238f595df06cfb5204b0b7c45ad0b0b/src/main/java/com/lthoerner/betteranvils/EnchantUtils.java#L20 This should be a Map<NamespacedKey, Enchantment[]> for improved lookup speeds (double-nested for loops are usually a code smell)

cobalt trellis
mystic solstice
#

Nullable integer assumed not null (useless auto-boxing, given that according to GH this is the only reference to this method).

I wanted to indicate that the item should always have a repair material if it got to that point, but I didn't want to return some magic value from repairMaterialCost, what is the proper way to do this? @cobalt trellis

cobalt trellis
#

throw an exception

mystic solstice
#

Good point

#

I think I'm using nulls too much when I should be using exceptions

#

I'm used to Option<T> and Result<T>

#

Should I be writing custom exception types?

round fossil
#

Yes

#

You can return Optional

#

Or OptionalInt

#

But yeah else just exception or null

cobalt trellis
#

I generally prefer throw assertion errors for severe programmer errors, IllegalStateException for API timing issues and IllegalArgumentException for invalid/improper arguments

round fossil
#

Yeah those are a best practice in Java

mystic solstice
#

Expensive #getItemMeta call that should be cached

Where/how would I cache this? And where would be the other place that the cached value is used?

round fossil
#

Its cached lazily automatically

#

iirc

#

Nvm it isnt

#

But yeah it does clone the itemmeta every time you invoke getItemMeta

#

And clone() is expensive

#

No clue why spigot haven’t decided to go with the custom prototype pattern instead

mystic solstice
#

Another issue with that line is that you access the JavaPlugin instance in a clinit ("static initializer") block wich can be dangerous if the class is loaded too early.

How do I fix this? I don't want to use DI because the whole class is just static methods

mystic solstice
round fossil
#

Yeah its kind of a factory pattern

mystic solstice
#

Its mod API uses prototypes

round fossil
#

Idk lol

#

There’s a design pattern called it

mystic solstice
#

For this exact sort of thing

round fossil
#

“Prototype pattern”

mystic solstice
#

I think that the idea of having ItemMeta be an abstract class is a little ridiculous

#

But maybe I'm just not familiar enough with Java

#

It seems like it would be more like an interface but idk

round fossil
#

No its good

#

Because it defines a type inheritable hierarchy

#

Cuz item meta is like, the nbt shit that gives items properties

mystic solstice
# 
    // Determines the material used to repair the given item, or null if the item cannot be repaired with a material
    static DamageableMaterial getRepairMaterial(@NotNull ItemStack item) {
        // Only damageable items can be repaired
        if (!isDamageable(item)) {
            return null;
        }

        Material damageableItemType = item.getType();
        // Special cases for tools that are not made of the standard materials, or do not have a material prefix
        if (damageableItemType == Material.SHIELD) {
            return DamageableMaterial.WOOD;
        } else if (damageableItemType == Material.TURTLE_HELMET) {
            return DamageableMaterial.SCUTE;
        } else if (damageableItemType == Material.ELYTRA) {
            return DamageableMaterial.PHANTOM_MEMBRANE;
        }

        // General cases for most tools, weapons, and armor
        String damageableItemName = damageableItemType.name();
        if (damageableItemName.startsWith("LEATHER_")) {
            return DamageableMaterial.LEATHER;
        } else if (damageableItemName.startsWith("WOODEN_")) {
            return DamageableMaterial.WOOD;
        } else if (damageableItemName.startsWith("STONE_")) {
            return DamageableMaterial.STONE;
        } else if (damageableItemName.startsWith("IRON_") || damageableItemName.startsWith("CHAINMAIL_")) {
            return DamageableMaterial.IRON;
        } else if (damageableItemName.startsWith("GOLDEN_")) {
            return DamageableMaterial.GOLD;
        } else if (damageableItemName.startsWith("DIAMOND_")) {
            return DamageableMaterial.DIAMOND;
        } else if (damageableItemName.startsWith("NETHERITE_")) {
            return DamageableMaterial.NETHERITE;
        } else {
            return null;
        }
    }

How do I "prebake" this list?

round fossil
#

I think they want you to use a set

#

A HashSet or EnumSet specifically

#

Since the lookup is at best and at average O(1)

#

Anyway

#

Ur utility class isn’t really just a utility class

#

Since it depends on state

#

(The static variable which is used in some of ur static helper functions)

mystic solstice
#

🫠

mystic solstice
round fossil
#

?

#

The proper pattern here is singleton if you wanna have global state

mystic solstice
#

Well, realistically it's doing almost all static things, but because it has to send logs and stuff it needs some state

mystic solstice
round fossil
#

I’d say you initialize the singleton instance in ur plugin main class in onEnable

#

And then yeah just a normal static singleton

mystic solstice
#

Well I really have no idea what a singleton pattern even looks like tbh

# 
public final class BetterAnvils extends JavaPlugin implements Listener {
    static BetterAnvils instance;

    public static BetterAnvils getInstance() {
        return instance;
    }
#

Is this it

round fossil
#

Pretty much

mystic solstice
round fossil
#

Ignore the static block

#

Or static initializer

#

W/E

mystic solstice
#

Is it bad style to use static singleton for util classes and DI for other classes

round fossil
#

No

#

But util classes are stateless, and often pure

#

If ur util class does not achieve that then its not a util class but rather just a helper class with helper methods etc

#

DI is essential

#

Along with interface segregation principle, dependency inversion principle and liskov’s substitution principle

sly jackal
#

they arent really singletons in my case

#

its just classes with static methods

round fossil
#

Then its a sucky helper class

sly jackal
#

purely because I cant put a static method in its own file

#

why is it sucky xD

round fossil
#

Yea

#

Well its fine sometimes

cobalt trellis
#

Hm, I just realized that I should cache Instant.EPOCH.toEpochMilli() properly in that class.
Whatever, not as if that method is called that often anyways

Yeah there is a bunch of stuff that is wrong with that class now that I look further into it.

round fossil
#

That’s technically just a non canonical multiton pattern

mystic solstice
#

Well my plugin had a fatal logic error which caused it to not do its main job

low apex
#

I want feedback

#

please

rancid fern
#

No code

low apex
#

ah do we have to set sourcecode ?

rancid fern
#

Yeah or share it

low apex
#

ok

woeful pasture
low apex
#

no ??

#

I just don't know how to do it

woeful pasture
low apex
woeful pasture
#

I trust fireship though I've never watched this tutorial if you have any dev questions go to #help-development I think we should try to keep this not cluttered and stick to code reviews as much as possible

mystic solstice
woeful pasture
#

Don't even need command line knowledge anymore you just click 1 or 2 buttons and you're done. Though it can definitely be overwhelming given if you're brand new. But once your comfortable writing code in your IDE learning git next would be logical

mystic solstice
woeful pasture
mystic solstice
sly jackal
#

or use github desktop

#

I never had to use the command line for git ever and its part of my job

mystic solstice
#

I'm just pointing out that it is faster for me, YMMV

#

I like being able to do quick things like git commit --amend --no-edit -a && git push -f

sly jackal
#

yeah

#

i dont even know what that does xD

#

I guess you push some commit?

cobalt trellis
#

The --no-edit -a part is new to me. Disregarding it the command would edit the last commit and force-push it to the remote repository

#

That being said you should really really rarely make use of force pushes. i.e. only if you pushed stuff that should never be pushed and you realize it within a few minutes after getting pushed.
Wait a bit too long with the force-push and you have a garbled history

river estuary
#

(ignore the readme its outdated)

jagged rock
#

I don't see the need for your subscription annotation system

#

I'd just make an interface

#

I also don't see the need for UUIDs to describe packet types

#

I'd preferrably just pass the class name instead so you don't ever need to do any registering

#

There's no javadoc, no comments

river estuary
jagged rock
#

That's the difference between getName and getSimpleName though

#

When you call Class#getName it returns the full package and class name

river estuary
#

Ah I forgot about that when I updated today

#

It used to be get simple name

#

I thought uuids would use less bytes

jagged rock
#

Also this is a bit icky

image.png
#

GIven you're printing to the console

#

It allows the possibility of DOS / DDOS attacks if I just spam connections to your socket

river estuary
#

Yeah

round fossil
#

Another thing, learn what to include/exclude in .idea

#

Rn u just dumped all ur idea config files upstream

#

Ur StringUtils class is missleading

#

It almost has nothing to do with strings

#

Also name the package util, not utils, more generally prefer singular

jagged rock
#

Either stick to singular or plural, I personally prefer plural

#

But don't do both

round fossil
#

If possible go with singular that’s a convention

winged blaze
#

@river estuary util classes shouldn't have a public construction

#

Constructor*

lunar nacelle
winged blaze
#

I always create a private constructor

lunar nacelle
#

I mean why :l

#

It shouldnot create new instance so why you create a constructor?

cobalt trellis
#

To prevent the instance from existing

lunar nacelle
#

yes It is unable to access with private but with final you cannot even call it :l

cobalt trellis
#

No?

#

Final just means you cannot extend the class

lunar nacelle
#

:D? isn't that what util class should have?

cobalt trellis
#

Yes, alongside the private ctor

lunar nacelle
#

oh yeh right :d

#

it does abled to create but there will some error throwing isn't it?

night knoll
sly jackal
#

does you console have the minecraft font?

#

thats pretty funny

rancid fern
sly jackal
#

oh yeah u right

#

now it makes sense

#

i mean the minecraft font is fun and all but I wouldnt wanna code in it

#

atleast it seems to be a monospaced version

opaque plover
#

please

night knoll
opaque plover
stark scarab
late smelt
#

Even if I’m still working on it, why not

jagged rock
#

I'd like to see constants for your int magic values

#

And guard clauses

#

index++ == 0 is also icky as you can have overflow issues if you run it enough times

late smelt
#

True

#

Should probably just swap a Boolean

woeful pasture
#

https://paste.md-5.net/jerehicuci.java any suggestions on improving this. My main goal is to add component support to inventories. Wondering if implementing IInventory was a bit overkill here, but if I don't and I use CraftBukkits MinecraftInventory what do I put in as the String title

midnight spade
#

And add getTitleComponent

woeful pasture
midnight spade
#

Or use a spigot method

#

Like Inventory#spigot

woeful pasture
#

there isn't one for inventories

midnight spade
#

Ah damn

woeful pasture
#

choco pr incoming hopefully soonish

#

but yeah that's why I'm using NMS

#

currently components on spigot only cover Chat messages maybe some other edge cases

midnight spade
#

Also can you add a setTitle to inv open event?

#

To customize the title sent for that player only?

woeful pasture
#

yeah

midnight spade
#

I had this idea a few months ago

#

That you're implementing rn

#

But got lazy

woeful pasture
#

you don't need to use NMS

#

I pr'd it a while back

#

I believe paper has component methods for it now too

midnight spade
#

oo

woeful pasture
#

?jd-s

rich fogBOT
midnight spade
#

Didn't know

woeful pasture
midnight spade
#

If you do setTitle, does it update for open invs?

woeful pasture
midnight spade
#

Lovely

woeful pasture
#

setTitle sets the title for a view which is specific to a player

#

so the setTitle name makes more sense though it may be less intuitive to think about logically

woeful pasture
#

once you set it

midnight spade
#

Nice👍

woeful pasture
#

anyways thanks for the reccomendation

woeful pasture
#

its not even public

midnight spade
#

RIP

#

That's stupid

woeful pasture
sly jackal
#

does the materialvaluemanager create the materials beforehand?

#

or like lazily creates them

#

of not I would recommend that

woeful pasture
#

so its all in memory

#

its just a Set call

#

MaterialValueManager#material(Material) loops through a set and finds all matching materials. It can't be done with a map as certain materials can contain extra data. Right now its only spawners, but it could change going forward

night knoll
round fossil
#

I mean yeah no

#

SRP says that a class must only change for a single major reason

#

so lets say that u have a class that parses strings into some other type of objects, then whenever you have to modify this class, the reason for modifying the class ought to be that the parsing had to be changed and not some other reason

#

now, that class looks like a facade class

#

so im unsure whether u actually want to chop it up into smaller classes

civic shell
jagged rock
#

mHM there's a lot of room for improvement

#

I'd start by following naming conventions

#

Instead of this, make a PlayerClass interface with a method to apply to a player

image.png
civic shell
#

Alrighty. done.

dusty axle
#

You need to follow naming conventions, both in proper capitalisation but in the fact that everything is confusingly named (e.g. why does the “entities” package contain event listeners? Why are so many things vaguely labelled as a “manager”?)
This is an issue in many areas, but just using your EconomyManager class as an example it appears to be a singleton but you then have a constructor and create an instance of it in your main function (which leads to you unintentionally calling loadBalancesFromFile twice)
There’s just generally a lot of repetition and areas which could be better abstracted, and a lot of error prone code from my cursory scan of a few classes

rancid fern
#

Keep your example code in the main method rather than in the constructor

#

Use variables?

#

Don't catch Exception

vivid reef
#

At this point you may as well code in a functional programming language

spring reef
#

id say his static use is fine so far

#

the contexts are only overlapping by having public statics

alpine anvil
#

the static use is fine in what ive just looked at

spring reef
#

f.e. in the EnkaNetworkAPI

#

i wouldnt give out base ui url or base url and handle them inside

#

yeah

#

encapsulate them

alpine anvil
#

could make it package private technically

spring reef
#

if you want do achieve that i would make a dao out of it

#

which one exactly

#

the cache?

#

you could also make it non static and instantiate it inside the api class

vivid reef
spring reef
#

other than that, why is CachedData and EnkaUserInformation in the util package

#

also i wouldnt have the CachedData manage its own expiration time

#

put the cacheddata closer to its purpose

#

like the cache or wherever its used

#

util is in most cases only a bunch of classes which you cant classify

#

the method is too long and complex

#

everyone looking over it doesnt want to look over it anymore

#

but inside the constructor?

#

rather make a load method and call it on startup or a specific event

#

everything else looks fine

#

in what specifically

#

looks redundant to me

#

you could leave it away as it seems

#

i mean the locale stuff since you already pass it in

#

or you could extract the localization parts from the caches

#

and make them static

#

no need to sacrifice the caches for the localization (in regards to static)

#

the localization part will be outside the caches

#

make an extra class which can be static

#

not QoL but rather overoptimizing

#

you could but for what

#

in what way

#

a few object creations less doesnt make it run better

#

it still has to read the jsons

#

without the I prefix and im in

alpine anvil
#

if you have a class called Nameable the I prefix is fine imo

spring reef
#

we could also be fancy and make it an abstract class but thats technically the same

spring reef
#

also imo it doesnt look clean in the code when using it

#

for this one: the usercharacter shouldnt load its own game data. load it while loading the user and pass it via setter

#

or in the constructor as well

#

your DTO shouldnt have any functionality but holding data

#

where is that

#

you move that method to where you get the usercharacter from

#

yeah same for that

#

looks like a DAO method

#

yeah you move that out too

#

thats why i would make a DAO which holds a cache and database/your interface to get the data from

#

and that dao would be your interface to get/load your data

#

and also saving it

#

and everything else happens behind that dao so you dont have to bother in every other class

#

no its just passed via DI

#

you create it at your entry point (if you have any, you should), instantiate it there and pass it around from there

#

if you dont have one

#

just make the dao itself static

#

not overcomplicating, but maintaining the structure of your project.

#

you seperate your code, keep it modular and abstract the data sources from each other

#
  • you encapsulate the data access logic, you dont have to bother how you get the data, but you do know you get it
#

it would also prevent the issues you have right now with your api class

#

which would need to stay static for the rest of your life and make its way into every other class

#

which isnt very optimal

#

with that current design you will soon or later be over-wiring your code and make it interdependent

#

yeah well, later you dont have this yet anymore

#

you will want to die while refactoring it

#

you do you, im just here to review

#

glad to help

#

now excuse me, i gotta destroy little kids in rocket league

night knoll
#

A class that allows to register multiple auth process with set priorities, meaning any other module will be able to add as many auth processes as it wants
https://github.com/KarmaDeb/LockLogin2/blob/master/LockLoginCommon/src/main/java/es/karmadev/locklogin/common/api/user/auth/CProcessFactory.java
https://github.com/KarmaDeb/LockLogin2/blob/master/LockLoginSpigot/src/main/java/es/karmadev/locklogin/spigot/event/JoinHandler.java#L430
https://github.com/KarmaDeb/LockLogin2/blob/master/LockLoginSpigot/src/main/java/es/karmadev/locklogin/spigot/process/SpigotAccountProcess.java

#

Any suggestion?
It currently it works

round fossil
#

does CProcessFactory require a concurrenthashmap

#

or can it work with a normal hashmap per say?

#

cuz if it needs a concurrent-supported map you wanna adhere to the liskov's substitution principle

#

local variables should be named lowerCamelCase also, some of your functions are too big

#

Optional<? extends UserAuthProcess> this makes almost no sense, since Optional is rarely just a producer

#

I like when you use this and super keyword if possible to distinct between other types of field as well as method invocations

night knoll
night knoll
round fossil
#

since it asserts the map does need the concurrency functionality

round fossil
#

Optional<UserAuthProcess>

night knoll
#

That's what I've mean with "bad habbit", I use them, everywhere

round fossil
#

What

#

ConcurrentHashMap is for multithreading

#

If u just wanna avoid CME when used on a single thread then you dont need CHM

opaque plover
#

honestly morice just kudos to you for making an enka api wrapper, i think i can find a use for it sometime when I'm bored with Genshin again lol

night knoll
spring reef
#

then we can continue

#

there is so much boilerplate and i dont understand shit whats going on in there

night knoll
night knoll
round fossil
#

bro u need to understand that not every variable is a constant

#

esp those atom refs

#

the start function is imo way too huge

#

testing all that logic is gonna be tough

jagged rock
#

Yeah this is doodoo

#

I got nothing to add

night knoll
#

Like a function to call the events and stuff?

round fossil
#

myea

#

well

spring reef
round fossil
#

^ pretty much that

night knoll
#

I'm scary that if I touch something it will simply stop working

spring reef
round fossil
#

refactoring does not change behavior tho @night knoll

#

for instance u may wanna extract smaller methods out from that bigger method

#

or change name of variables

night knoll
jagged rock
#

They are

#

These don't follow conventions

image.png
#

You can use a guard clause

image.png
#

And make a setStatus method

#

Instead of whatever this garbage is

image.png
night knoll
# 
private final AtomicLong timeLeft = new AtomicLong(1);

(for instance?)

jagged rock
#

this does the same as a regular get() with an added if check

image.png
night knoll
round fossil
night knoll
#

Hands to work then

night knoll
#

You were so right about splitting the start method.
All those

events.stream().filter((e) -> e.trigger() == null || e.trigger().equals(TaskEvent.RESUME)).forEachOrdered((event) -> {
                            Object runner = event.get();
                            if (runner instanceof Consumer) {
                                ((Consumer<Long>) runner).accept(ELAPSED.getAndAdd(interval));
                            }
                            if (runner instanceof Runnable) {
                                ((Runnable) runner).run();
                            }
                        });

are now a single method without even arguments 😳

#

But I think it stills kind of "big"?

        if (!status().equals(TaskStatus.STOPPED)) return;
        setStatus(TaskStatus.RUNNING, false);

        AtomicReference<ScheduledFuture<?>> future = new AtomicReference<>();
        future.set(EXECUTOR.scheduleAtFixedRate(() -> {
            if (status().equals(TaskStatus.STOPPED)) return;

            if (status().equals(TaskStatus.RUNNING)) {
                long timeLeft = this.timeLeft.get();

                currentContext = TaskEvent.TICK;
                executeEvents(true);

                if (timeLeft == 0) {
                    if (!isRepeating.get()) {
                        future.get().cancel(true);
                        setStatus(TaskStatus.STOPPED, false);
                        //taskStatus.set(TaskStatus.STOPPED);

                        currentContext = TaskEvent.END;
                        executeEvents(false);
                    } else {
                        this.timeLeft.set(this.limit - this.interval);

                        currentContext = TaskEvent.RESTART;
                        executeEvents(false);
                    }

                    elapsedTime.set(this.interval);
                }

                if (updateTL.get()) {
                    this.timeLeft.set(timeLeft - this.interval);
                }
                updateTL.set(true);

                return;
            }

            if (status().equals(TaskStatus.RESUMING)) {
                currentContext = TaskEvent.RESUME;

                executeEvents(true);
                setStatus(TaskStatus.RUNNING, true);
            }

            if (lastTaskStatus.get().equals(TaskStatus.RUNNING) && status().equals(TaskStatus.PAUSED)) {
                lastTaskStatus.set(TaskStatus.PAUSED);

                currentContext = TaskEvent.PAUSE;
                executeEvents(false);
            }
        }, 0, interval, workingUnit));

        currentContext = TaskEvent.START;
        executeEvents(false);
spring reef
sly jackal
#

but should it

stark scarab
opaque plover
#

and the only thing that makes me want to commit homicide against you alex is the fact that your Daddy_Stepsister class name is not following the conventions

night knoll
#

WTF? It's real lmao

#

Daddy_Stepsister

#

I love this name

#

I take it, idc it doesn't follow name conventions

opaque plover
#

and some other things are that you can use early returns instead of nesting here (for the outermost if clause)

image.png
night knoll
#

It's hillarous

opaque plover
#

though this is preferential ofc

opaque plover
night knoll
#

No way

#

fr?

opaque plover
#

yeah

stark scarab
#

the Stepsister class is prefixed with Daddy_ to avoid issues while shading and relocating because it contains a few other classes that might already exist (TickUtils, JoinListener and BooleanPersistentDataType)

image.png
opaque plover
#

also alex this is also very preferential but it makes me cut onions, why are you iterating with an actual iterator instead of using for-each or something

#

this makes it look like it's decompiled code xd

opaque plover
stark scarab
#

probably it's useless

opaque plover
#

fair enough

stark scarab
#

as you can see from the bottom-most commented out method, I once wanted to generify it. currently most of those methods are copy/paste

#

but that won't work because some methods use -1 as magic value, others have "0" as best value while others have 0 as worst value, and some use -2 and -1 as magic values

#

wanna see another nasty thing?

#

this is a field instead of an init block to avoid IntelliJ's auto-formatting to move this below the other fields

image.png
cobalt trellis
#

As a general rule if CAS or other atomic ops are not used, then atomics are not useful

#

Your setRepeating method shouldn't use CAS either as that is pretty much a direct assignment at the end of all things

#

Also idk if it is wise to use COWList for taskEvents. If you write frequently to it it's usage is to be avoided

night knoll
#

So I used atomic on everything

#

And my scheduler is (in fact) concurrent, as it can be modified (and accessed) from multiple places at the same time

#

Is that, or I don't actually understand what concurrency is

cobalt trellis
#

Atomics are the best way, but only because of CAS

#

If you cannot use CAS or other atomic operations it is just a fancy field marked with volatile

round fossil
#

You don’t need atomics most of the time if you just process data concurrently

#

Its only good when you update some variable based on its old value when multiple of these updates can happen concurrently

#

Because essentially the hardware is called to compare and swap values (CAS)

#

Also must be said that concurrency does not equal multithreading, they correlate but should be differentiated from one another

round fossil
#

Are we considering enterprise important?

stark scarab
round fossil
#

Na think its pretty good

#

Pretty readable when I tried to understand what was going on

stark scarab
#

what I don't like is that half of those methods are 90% the same

#

but it's only 90%, that's why I failed at refactoring them into one "generic" method

round fossil
#

Yeah well I wouldn’t mind too much there tbh

spring reef
#

your constructor is too big

#

each of those methods could be 3 individual methods

#

plus do you really need a linkedhashmap

#

also im not a fan of this final overuse and your variable names are too short

#

in fact you could also divide GroupManager into several context related classes as well. that would also lead you away from the "Manager" suffix which you really dont want to have

stark scarab
stark scarab
stark scarab
spring reef
spring reef
spring reef
# stark scarab example?

f.e.

old

    public double getFetchPricePerPlayer(final CommandSender commandSender) {
        try {
            if (yaml == null || !Daddy_Stepsister.allows(PremiumFeatures.FETCH_PRICE_PER_PLAYER))
                return getPercentagePrice(commandSender, main.getConfig().getString(Config.PRICE_FETCH));
            final Iterator<String> it = groups.keySet().iterator();
            Double bestValueFound = null;
            while (it.hasNext()) {
                final String group = it.next();
                if (!commandSender.hasPermission(Permissions.PREFIX_GROUP + group)) continue;
                final String pricePerPlayer = groups.get(group).priceFetch;
                if (pricePerPlayer.equals("-1")) {
                    continue;
                }
                bestValueFound = bestValueFound == null ? getPercentagePrice(commandSender, pricePerPlayer) : Math.min(getPercentagePrice(commandSender, pricePerPlayer), bestValueFound);
            }
            if (bestValueFound != null) {
                return bestValueFound;
            } else {
                return getPercentagePrice(commandSender, main.getConfig().getString(Config.PRICE_FETCH));
            }
        } catch (NumberFormatException exception) {
            return 0;
        }
    }

new

public double getFetchPricePerPlayer(final CommandSender commandSender) {
    if (yaml == null || !Daddy_Stepsister.allows(PremiumFeatures.FETCH_PRICE_PER_PLAYER)) {
        return getPercentagePrice(commandSender, main.getConfig().getString(Config.PRICE_FETCH));
    }
    
    double bestValueFound = findBestValueForGroups(commandSender);
    
    if (bestValueFound != 0) {
        return bestValueFound;
    } else {
        return getPercentagePrice(commandSender, main.getConfig().getString(Config.PRICE_FETCH));
    }
}

private double findBestValueForGroups(CommandSender commandSender) {
    Iterator<String> it = groups.keySet().iterator();
    double bestValueFound = 0;
    
    while (it.hasNext()) {
        String group = it.next();
        if (commandSender.hasPermission(Permissions.PREFIX_GROUP + group)) {
            String pricePerPlayer = groups.get(group).priceFetch;
            if (!pricePerPlayer.equals("-1")) {
                bestValueFound = calculateBestValue(commandSender, pricePerPlayer, bestValueFound);
            }
        }
    }
    
    return bestValueFound;
}

private double calculateBestValue(CommandSender commandSender, String pricePerPlayer, double currentBest) {
    double percentagePrice = getPercentagePrice(commandSender, pricePerPlayer);
    return (currentBest == 0) ? percentagePrice : Math.min(percentagePrice, currentBest);
}
novel meteor
jagged rock
#

Not a fan of the world setup in the game class

#

And I'd split some things

#

Scoreboard teams are also icky to be done there, maybe make some sort of builder or utility class

#

Static game instance can cause some issues

#

When I make my minigames I make sure to split event listeners and schedules on the main phase into separate methods

novel meteor
jagged rock
#

I didn't really comment on this but instead of tying these listeners to each phase, I have 2 approaches I take depending on context

image.png
#

One is a "Permanent Phase" class which is a phase that immediately ends when started, but doesn't dispose until the entire game disposes

#

Something like this

novel meteor
#

That's a great idea

jagged rock
#

But sometimes you might want to toggle and this isn't generally the best approach

#

So I have a system I call "Vanilla Mechanics" which is a sort of registry for toggleable mechanics

#

You need to be careful with cleanup

image.png
#

But it works something like this

novel meteor
#

Yeah, that would be useful as well
Man, you really need to write these down XD
I'd pay for this shit

jagged rock
#

That takes time and writing my secrets doesn't pay my bills

novel meteor
#

yeah, fair point

jagged rock
#

I want to make sure everything is properly done and polished before I write an entire minigame guide so every amateur can make their own copy of hypixel

night knoll
#

But that's not for this thread

viral mantle
#

this is my code for scoring

#

thebridge plugin

#

and I was wondering if there was a better way of doing this?

#

I wanted to get other people's opinion

novel meteor
#

use early return when possible

if (!(game.getPlayers().containsKey(player.getUniqueId()) && event.getCause() == PlayerTeleportEvent.TeleportCause.END_PORTAL)) 
return;```
#

or you get this

LYxwcI4.jpg
#

I think you would have to have the end enabled in the world for the teleport event to fire
but having the end enabled is gonna lower the performance
So doing this in the player move event might be better, but idk

viral mantle
#

alralr

viral mantle
#

so to see which is better

frosty grove
#

partially lazily coded

#

does some bytecode analysis

#

and transformation

tawny tartan
#

some code for my personal API

#

😌

woeful pasture
#

right now I'm just thinking how it may actually be better to use something like an ExecutorService or just Bukkits built in task system with a wrapper of sorts, though I don't know how your NovaThread works under the hood

jagged rock
#

Not a huge fan of the numerical switch

jagged rock
#

Interface full of null / empty methods, looks unfinished to me

jagged rock
#

I'd rename Pos to Position, loop through the entryset instead of looping through keys and calling get(key)

#

Guard clauses and all

frosty grove
#

its finished

#

its complete

frosty grove
#

protected/package-private ftw

alpine anvil
jagged rock
#

atomicinteger

#

just make a dedicated class for this

#

alhamdullillah

image.png
#

That's enough

#

Also your database handling is trash

alpine anvil
frosty grove
#

might be why it looked incomplete

frosty grove
#

or the Usage class

tawny tartan
tawny tartan
#

I could use ExecutorService but idk

woeful pasture
opaque plover
#

https://github.com/RoughlyUnderscore/rust-guess-the-number/blob/main/src/main.rs i just started with rust and decided to create an utmost generic number guesser. i have no understanding of ownership, lifetimes, macros, &str vs String, etc, i've watched the tutorial on the very basics of rust syntax and created this. any criticism welcome

GitHub
rust-guess-the-number/src/main.rs at main · RoughlyUnderscore/rust-...

Contribute to RoughlyUnderscore/rust-guess-the-number development by creating an account on GitHub.

round fossil
#

Doesn’t look too bad, but I suggest learning difference between &str vs &String (and just String I suppose)

#

Lifetimes aren’t hard basically the life of a variable, which is decided by its scope I don’t wanna compare it to Java cuz that might mislead you but yeah

opaque plover
opaque plover
round fossil
#

I think you wanna learn what macros, not necessarily being able to write ur own at this stage of the learning phase

round fossil
opaque plover
#

actually maybe you have a good tutorial? the one I'm watching rn is basically for people whose first language is rust, I was wondering if there was maybe a tutorial for rust for those who understand basic programming concepts. it's a bit annoying to listen to 7 minutes of somebody explaining control flow with if-else

#

I've heard the rust docs are a good tutorial by themselves but I want to stick to video if possible

round fossil
opaque plover
#

thanks, I'll have a look 🙂

opaque plover
lunar vale
novel meteor
jagged rock
#

Or better yet, don't hardcode messages

woeful pasture
#

language files go brrrr

lunar vale
#

i dont like spaces and i prefer double s

#

i just do alt 21

#

or alt 0167

#

i dont touch category so i didnt remove the spacds

cobalt trellis
#

Wut are these codes?

#

You mean \u00A7?

opaque plover
lunar vale
cobalt trellis
#

Yes I know but I do not use this accursed feature

analog gyro
alpine anvil
#

That isnt how it should be using futures

stark scarab
#

he's using futures everywhere just because players can have a configurable language

#

I already said to just keep a Map<UUID,Language> and Map<Language, Messages> and load the player into the map on join

#

and the whole usage of futures seems pretty odd too, it should rather be sth like this

return CompletableFuture.supplyAsync(() -> {
  ItemStack item = ...;
  String language = mySqlThingy.getLanguage(...);
  String message = myMessageThingy.getMessage("some-message", language);
  // apply to item
  return item;
});
#

instead of creating a future, then passing that to something else and completing it there

cobalt trellis
#

Wait .... Languages are set via SQL???? Humanity is doomed.

stark scarab
#

also the whole code is repeating itself over and over again

cobalt trellis
#

Well that is irrelevant in comparison to the SQL

stark scarab
cobalt trellis
#

The intValue call can be done at a later time

#

And are you sure that this hack even works?

stark scarab
#

it does work

#

but it's ugly

stark scarab
cobalt trellis
#

Especially when storing doubles (such as 0) or ints (anything < Integer.MIN_VALUE) or longs (anything < Integer.MAX_VALUE)

stark scarab
#

only place where i noticed that it's needed is when parsing ItemMeta, as the buildEnchantments method does an instanceof Integer check

#

and for that it works fine

# 
    private static Number narrowNumberType(Number number) {
        double asDouble = number.doubleValue();
        long asLong = number.longValue();

        double longAsDouble = asLong;

        if(asDouble == longAsDouble) {
            if(asLong > Integer.MAX_VALUE || asLong < Integer.MIN_VALUE) {
                return asLong;
            } else {
                return number.intValue();
            }
        }

        return asDouble;
    }

I now changed it to this but it's still shit

cobalt trellis
cobalt trellis
#

Oh and return asDouble should be return number

stark scarab
#

true

#

it's kinda weird that gson parses something like "1" into a double in the first place

night knoll
feral burrow
#

sorry I was trying to fix the author of my git commits being invalid-email

#

I figured there was a better way lol

#

saveDefaultConfig(); done

# 
public class ReadyListener extends ListenerAdapter {
    @Override
    public void onReady(ReadyEvent event) {
        System.out.println("Discord bot is ready!");
    }
}

like this?

#

as for combining the listeners, you say it can get messy when you have a bunch, wouldnt I have to separate them again?

#

ok that makes sense,
btw just so I understand this better, when I do an @EventHandler, is the server event manager finding the method that has the appropriate event in the paramaters? so the name of the method doesnt matter?

#

can I have the same event in two different classes?

#

ok

#

should I use reflection to register the events? you said my method is too much

#

thats because I might have another listener of the same name somewhere else

#

in the future

#

should I do ChatBridgeOnPlayerJoin.java ?

#

yeah

#

ok

#

thanks for the help

night knoll
#

Do you guys use this. Whenever possible

#

and super.

jagged rock
#

Why is all of this static?

night knoll
#

yes

#

which one

#

and why

night knoll
#

is this good ?

#

do u prefer hikari static to nonstatic ?

#

I would remove setPlugin in the main class @night knoll

#

why

#

And your friends

#

Don't have to be static

#

friends

night knoll
night knoll
#

ah

#

like what if I just setPlugin(null)

#

but i need get plugin

#

Yes

#

put

#

plugin = this

#

In on enable

#

aight 1m

#

aight

#

this good

#

but what u think for my main

#

i mean

#

for my static values

#

and methods

#

am i do static they ?

#

example hashmap

jagged rock
#

You don't need static pretty much anywhere

hard island
#

?mappings

rich fogBOT
night knoll
#

Guys what's the designer pattern called where:

You make a project out of interfaces, like everything, then impl them somewhere else

If it's encapsulating then idk

#

like bukkit api basically

#

also should I use that pattern or not

#

like rn I only use interfaces for objects that will clearly have different implementations

#

like a database, there might be a local, sql, etc

dusty axle
#

Inheritance? You’ve just described a basic object oriented programming concept lol

jagged rock
#

That's called multi platform code

#

You just make interfaces for everything

#

And then an abstract class with a couple abstract methods for each platform to impl

#

An abstract class sql databases which is then extended by an abstract class for remote sql databases

#

and you end up with 10k lines of code that do nothing

night knoll
#

But it supports multiple

#

wait tell me more about the abstract classes

jagged rock
#

It's a class that can't be instanced directly and must be extended

night knoll
#

why not just use the interfaces atp

#

not ik what they are

#

but why have an abstract class for a database when there's an interface

jagged rock
#

You make an interface and an abstract class that impls it and provides like 90% of the code you need

night knoll
#

Oh

#

so like

#

All the data things

#

Like the simple getter and setters

jagged rock
#

Are done by the abstract class

night knoll
#

then the impl do all the complicated

#

methods

jagged rock
#

And your impl only cares about loading the driver

night knoll
#

Do you think it's a good practice

jagged rock
#

It's what I've been doing

#

And yes it's proper use of inheritance

night knoll
alpine anvil
#

jda would use it for different types of things

#

eg channel types

lunar vale
#

databases

#

when they all use mongodb and have completely different methods

night knoll
#

if you have a database for like

#

punishments

#

just have the generic methods

#

getPunishmentLogs(User)
saveLog(Log)

dusty axle
#

mongodb

Oh no

lunar vale
#

what about it

dusty axle
#

Every time I have used it I have regretted it

jagged rock
#

I usually structure my databases like this

image.png
night knoll
#

How would you guys recommend I do this

#

I was thinking about a Factory

#

Also would be nice for recommendations for the rest of the project

#

I just started it

round fossil
#

Looks alright

night knoll
#

Do you think I should use a factory pattern

round fossil
#

You can have a static factory for the builder

#

But nah builder here is considerably a good choice

night knoll
#

yeah ik lol there's a lot of fields

#

ok then what I might do is include my Duration and NameableUserEntity class into the builder

round fossil
#

I would only use a factory to decouple the creation and instantiation of these objects if a lot of classes construct them and if you can abstract away the need to have that many arguments passed upon creation

round fossil
#

I suggest you to read up on effective java

#

It covers creation of objects and good practices

frosty grove
#

wrote this for an open source project im a part of

#

already sent an early version of this once

night knoll
#

I need constructive feedback on this local database

#

Everything works I tested it

round fossil
#

Depend on Plugin, not JavaPlugin
Avoid shortcut/acronym names like DB, only tome its allowed is if its http or something universally known, DB is arguably not.
Don’t use File, use Path.
Odd choice of return type in CF (hinting at Boolean)

#

@night knoll

#

Personally I would avoid coupling the async storage layer with the storage logic layer

#

(Look at humble objects principle)

night knoll
#

So the depend on Plugin, why would you recommend that

round fossil
#

Let me ask you this what’s the benefits of depending on JavaPlugin?

#

If you depend on Plugin its easier to mock the dependency, and you decouple yourself from an abstract class

night knoll
#

DB, I mean it's in the database package and it implements database

Idk how to create a file using path api lol

The return type boolean idk what else I would use

And idk what coupling async storage means

#

What does couple mean

round fossil
#

Parent child implementation inheritance is one of the most coupled prone concepts in all of object oriented programming

#

When instances depend on each other directly

night knoll
#

Oh

#

Oh I see

#

You want me to dependency inject more

round fossil
#

Well no

#

I just suggest you to depend on Plugin as a type and not JavaPlugin

#

Since its more beneficial for you

night knoll
#

I could just make it not async

#

And the caller does all the threading

#

that does sound better imo

round fossil
#

I’d avoid that

#

Having an async service layer is always nice for the api consumer

#

Not having to deal with thread safety, atomicity designs, mem fences or race conditions

#

Etc

night knoll
#

So you want me to make something like

round fossil
#

Hace you looked at luckperms?

night knoll
#

AsyncManagementDatabase extends ManagementDatabase

night knoll
round fossil
#

We say that storageimplementation.java in this case is humble with respect to storage.java

night knoll
#

Oh I see

round fossil
round fossil
spring reef
#

a state ¯_(ツ)_/¯

night knoll
#

Ok I'm back

night knoll
#

And yeah I see what you mean about the database

#

the async one like wraps the implementation

night knoll
#

Decouple yourself from an abstract class

stark scarab
#

is it fine to use an assert here? isWildcard() already checks whether wildcardPrefix is null, but without the assert, IJ complains that it might be null. I do NOT want to add any additional logic as that'd be weird imho

image.png
sly jackal
#

ignore ij

#

its dumb

#

if you really get triggered by the warning assert is fine

#

since it will never assert to false anyways

#

basically ij doesnt know that you catch this in isInWildcard

dusty axle
#

The implementation is isWildcard could be changed if it is overridden by a subclass, which is why IntelliJ doesn’t count it as guaranteed not null

night knoll
#

and they said to throw and exception

stark scarab
stark scarab
#

is this a proper way of writing tests or is it fucked up lol

image.png image.png
round fossil
#

I mean I’d say you’re doing it a bit wrong (but thats from how I learnt)

#

Basically your tests are testing things that your test shouldn’t test, it should be covered in other tests

#

Like usually, if you wanna test the method Group::containsUser you don’t test Group::addUser and Group::setPermission

#

Those methods should be well tested in other unit tests

#

Also

#

Avoid writing testBlahBlah()

#

Every1 knows u’re testing ur code

#

You should look up proper test naming conventions

stark scarab
#

i remember junit refusing to run methods that didnt have "test" in the same even when they were annotated

#

or maybe it was the class that had to have Test in the name?

round fossil
#

that’s old junit I believe

stark scarab
#

I'm using this from the gradle docs

image.png
round fossil
#

Yeah well u shouldn’t need to forcefully have the prefix test

#

One more thing

stark scarab
round fossil
#

It is pretty nice to static import all of Assertions.java

round fossil
#

Creating a group should be covered in other unit tests

stark scarab
#

and then pass that group object around? I don't really understand

round fossil
#

ofc this implies the tests for Group::containsUser may fail if the tests from Group::addUser failed

#

Well I mean

#

Its just that you don’t wanna be performing any larger tests

#

Well what I wanted to say is that your unit tests almost become module tests @stark scarab

#

It is better to have more tests to cover ur code than to cramp them

frosty grove
#

you can read the stack trace so you'll know what fails right

#

why do you need such small tests

#

id have a test per group of methods

round fossil
frosty grove
#

tf is the fragile test problem

round fossil
#

google it

#

its quite a common symtom you get if you aren't experienced with unit tests and tdd

frosty grove
#

and isnt it better to test a system by emulating real use cases instead of a fabricated environment

round fossil
#

i mean u have system tests for that

#

but they are different

frosty grove
round fossil
frosty grove
#

i know what you mean but at the same time, testing addUser and containsUser in one test doesn't sounds like a problem to me

#

same system

round fossil
#

anyway SYSTEM TESTS are for testing the entire system, INTEGRATION TESTS for testing interactions of modules

frosty grove
#

if one breaks, the other is unusable anyways

#

had to write a custom test system

round fossil
#

you don't get the point because you haven't ever exposed yourself to a proper test environment more than just writing some unit tests here and there, but basically you wanna decouple ur test structure from ur code architecture, else you will end up with tests that will bring other code symptoms to your actual code architecture like fragility and inflexibility

#

when your test structure is coupled to ur code architecture tightly enough we call it the fragile test problem

frosty grove
#

hm ok

round fossil
#

I mean it becomes noticeable in larger code bases mostly

#

coupling methods here and there may not affect u

frosty grove
#

you need some kind of setup for most tests anyways tho which then couples it

#

like how do you check containsUser if you dont want to call addUser

round fossil
#

its fine to use addUser

#

its just that in that method where you test containsUser, you don't wanna test addUser

frosty grove
#

ig

#

or you could make a testUserManagement

frosty grove
round fossil
#

idk

#

i havent ever seen some1 do it that way

#

apart from overloaded static functions

frosty grove
#

i had to write a custom test system because junit loads all test classes

round fossil
#

ah I see

frosty grove
#

which was a problem for bytecode transformation tests

lunar vale
spring reef
# lunar vale i wish i knew what unit testing and proper structure looked like 
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;

import org.junit.jupiter.api.Test;

class AreaCalculationSettingsTest {

    @Test
    void setSourceWhereMissing_ShouldGenerateNamesForUnnamedSources() {
        // Given
        AreaSource namedSource1 = new ProductWidthAreaSource("source1");
        AreaSource namedSource2 = new ProductWidthAreaSource("source2");
        AreaSource unnamedSource1 = new ProductWidthAreaSource(null);
        AreaSource unnamedSource2 = new ProductWidthAreaSource(null);
        AreaSource namedSource3 = new ProductWidthAreaSource("source5");

        AreaCalculationSettings areaCalculationSettings = new AreaCalculationSettings();

        areaCalculationSettings.addAreaSource(namedSource1);
        areaCalculationSettings.addAreaSource(namedSource2);
        areaCalculationSettings.addAreaSource(unnamedSource1);
        areaCalculationSettings.addAreaSource(unnamedSource2);
        areaCalculationSettings.addAreaSource(namedSource3);

        // Safety asserts to ensure that addAreaSource() doesn't do anything prohibited
        assertNotNull(namedSource1.getName());
        assertNotNull(namedSource2.getName());
        assertNull(unnamedSource1.getName());
        assertNull(unnamedSource2.getName());
        assertNotNull(namedSource3.getName());

        // When
        areaCalculationSettings.setSourceWhereMissing();

        // Then
        // Assert that all sources have names assigned
        assertNotNull(namedSource1.getName());
        assertNotNull(namedSource2.getName());
        assertNotNull(unnamedSource1.getName());
        assertNotNull(unnamedSource2.getName());
        assertNotNull(namedSource3.getName());
    }
}
#

or with a few more tests


import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

import org.junit.jupiter.api.Test;

import com.example.xyz.model.Shelf;

public class ShelfBuilderTest {

    @Test
    public void widthMustBeGreaterThanZero() {
        // Given, When, and Then
        assertThrows(IllegalArgumentException.class, () -> new ShelfBuilder().setShelfWidth(0));
    }

    @Test
    public void mountingHeightMustBeGreaterThanZero() {
        // Given, When, and Then
        assertThrows(IllegalArgumentException.class, () -> new ShelfBuilder().setMountingHeight(-1));
    }

    @Test
    public void mountingHeightMustBeSet() {
        // Given, When, and Then
        assertThrows(IllegalStateException.class, () -> new ShelfBuilder().build());
    }

    @Test
    public void shelfHasCorrectMountingHeight() {
        // Given
        ShelfBuilder shelfBuilder = new ShelfBuilder()
            .setMountingHeight(1700);

        // When
        Shelf shelf = shelfBuilder.build();

        // Then
        assertEquals(1700, shelf.getMountingHeight());
    }

    @Test
    public void shelfHasCorrectWidth() {
        // Given
        ShelfBuilder shelfBuilder = new ShelfBuilder()
            .setMountingHeight(1700)
            .setShelfWidth(50);

        // When
        Shelf shelf = shelfBuilder.build();

        // Then
        assertEquals(50, shelf.getWidth());
    }

    @Test
    public void shelfHasDefaultThicknessOfThirty() {
        // Given
        ShelfBuilder shelfBuilder = new ShelfBuilder()
            .setMountingHeight(1700);

        // When
        Shelf shelf = shelfBuilder.build();

        // Then
        assertEquals(30, shelf.getThickness());
    }

    @Test
    void shelfHasDepth() {
        // Given
        ShelfBuilder shelfBuilder = new ShelfBuilder()
            .setMountingHeight(1700)
            .setDepth(42);

        // When
        Shelf shelf = shelfBuilder.build();

        // Then
        assertEquals(42, shelf.getDepth());
    }
}
lunar vale
#

thakns

stark scarab
dusty axle
#

Agile™️

frosty grove
night knoll
#

It let's me organize messages like this:

plugins/MyPlugin/messages/SomeMessage.txt

#

And then I can do stuff like this

            ```Bukkit.broadcast(new PublicBanAnnouncement(target, issuer).getAsComponent());```
stark scarab
stark scarab
round fossil
#

ye

stark scarab
#

:3

opaque plover
novel meteor
#

Really tiny brigadier command "library" or whatever
Completely ignores bukkit permissions by default, too lazy to add support for it
Essentially a Spigadier, but for 1.20 and should work on higher versions so long mojang does not change anything.
https://github.com/steve6472/Brigit

alpine anvil
#

do you use netbeans

novel meteor
#

the what ?
No IntelliJ

alpine anvil
#

hmm

novel meteor
#

What made you think I used netbeans ?

eager wasp
#

The code formatting

alpine anvil
#

^^

novel meteor
#

I should have put that as a disclaimer lol
Not changing that in my personal projects.

night knoll
#

Do you guys think something like this would be a good format

#

try {
Do stuff with command
}catch(E e) {
Send msg
}catch(E2 e) {
Send msg
}

opaque plover
# night knoll try { Do stuff with command }catch(E e) { Send msg }catch(E2 e) { Send msg }

https://blog.jeff-media.com/dont-jump-off-bridges-or-why-you-shouldnt-use-exceptions-to-control-code-flow/

JEFF Media Developer Blog
mfnalex
Don't jump off bridges, or: Why you shouldn't use Exceptions to con...

Exceptions are a wonderful thing. They can let your code recover from unexpected situations in a nice and clean way, if you use them properly. That does not mean that you should always rely on them though. For example, imagine this: Most highways have guardrails in the middle to avoid cars going into oncoming traffic....

night knoll
alpine anvil
#

Why

rancid fern
#

Lazy probably

#

too many things to check

night knoll
#

It just looks so messy

rancid fern
night knoll
#

Sometimes it's not just checking the parameters

#

it's also doing a lot of weird stuff as the function lol

jagged rock
#

ehh

#

solution is cleaner

round fossil
night knoll
round fossil
#

i dont remember

#

but there is a name for that particular paradigm

night knoll
#

Yeah I was reading everywhere that it is an anti pattern

#

it's just that

#

idk

#

I really do feel like it would be really nice

round fossil
#

i mean

#

in most cases its gonna be an anti pattern

night knoll
#

like save

#

and if it doesn't work it blocks the code flow

#

should I just return a boolean or?

#

And I really don't know how I should break everything down lol

spring reef
#

you shouldnt do it

spring reef
spring reef
#

or for if everything explodes

spring reef
night knoll
spring reef
#

you can break the larger methods down into several smaller ones

#

would be called "extract method refactoring" then

night knoll
#

just that some methods will need a ton of paremeters

night knoll
# 
public record PlayerConnectionListener(ManagementDatabase managementDatabase) implements Listener {

    @EventHandler
    public void onPreLoginEvent(AsyncPlayerPreLoginEvent event) {
        try {
            PunishmentPortfolio portfolio = managementDatabase.getPunishmentPortfolioAsync(event.getUniqueId()).get();
            handlePlayerIfBanned(portfolio, event);
        } catch (Exception e) {
            e.printStackTrace();
            event.disallow(AsyncPlayerPreLoginEvent.Result.KICK_OTHER, new InternalErrorMessage().getAsComponent());
        }
    }

    private void handlePlayerIfBanned(PunishmentPortfolio portfolio, AsyncPlayerPreLoginEvent event) {
        Punishment possibleCurrentBan = portfolio.getCurrentBan();
        boolean isNotBanned = possibleCurrentBan == null;
        if(isNotBanned) return;
        kickPlayerDueToBan(possibleCurrentBan, event);
    }

    private void kickPlayerDueToBan(Punishment banPunishment, AsyncPlayerPreLoginEvent event) {
        event.disallow(AsyncPlayerPreLoginEvent.Result.KICK_BANNED,
                new BanScreenMessage(banPunishment).getAsComponent());
    }
    
}

Is this a good way to handle bans?

#

I just wondering if the catch Exception on the catch block is appropriate:

} catch (Exception e) {
        e.printStackTrace();
        event.disallow(AsyncPlayerPreLoginEvent.Result.KICK_OTHER, new InternalErrorMessage().getAsComponent());
}```
stark scarab
opaque plover
#

hypothetical question, not working on a project
is this way of working with operations a good practice? (chaining)

favicon.execute(OperationChain(Rotations.FLIP_HORIZONTAL).then(Rotations.FLIP_VERTICAL).do());
round fossil
#

Looks fine

#

I mean I like when I can write a pre cached operation and then just invoke it at will w/o having to build it again and again

sly jackal
#

too bad you cant use operations

#

like +

#

but this looks similar to what I would do

#

except teh do feels kinda pointless

#

I would probably just put it into the favicon class or whatever

#

like a way to flip the images

night knoll
#

Hey everyone! 👋 I'm fairly new to the Minecraft development scene and I'm eager to improve. I'd really appreciate any constructive criticism on my plugin, PixelPaste. Whether it's praise or pointers, your feedback will be invaluable for my development journey. The code is open-source, and you can check it out directly via the GitHub link on the Spigot plugin page.

https://github.com/jaceg18/PixelPaste

Thanks in advance for taking the time to help me out! 🙏

GitHub
GitHub - jaceg18/PixelPaste: A lightweight minecraft plugin that al...

A lightweight minecraft plugin that allows you to turn images into builds. - GitHub - jaceg18/PixelPaste: A lightweight minecraft plugin that allows you to turn images into builds.

woeful pasture
woeful pasture
#

the whole handler system seems redundant

sly jackal
night knoll
woeful pasture
#

yeah having one executor for multiple commands doesn't scale in an organized manner. By doing handlers the way you are doing them right now you're solving a problem you could just solve by using the API

night knoll
sly jackal
#

you would make a command executor for each /command you add I imagine

#

but you can always split it up into your subcommands too if you have a lot of them

woeful pasture
night knoll
woeful pasture
#

its just shorthand for both and doesn't do anything besides that

night knoll
#

Is there any obvious bottlenecks/flow issues in the code from what you guys have seen thus far?

#

From what I can tell while running it, it seems fine. But I honestly don't trust myself to use the API correctly.

sly jackal
#

tbh there are not a lot of places for bottlenecks in a relatively small plugin

#

if its working good in the server its nice

woeful pasture
#

any reason you are running this the next tick?


Bukkit.getScheduler().runTask(PixelPaste.getInstance(), () -> {
    for (int x = 0; x < width; x++) {
        for (int yz = 0; yz < height; yz++) {
            int[] coords = ("vert".equals(orientation)) ? new int[]{x, yz, 0} : new int[]{x, 0, yz};
            Location loc = initialLocation.clone().add(coords[0], coords[1], coords[2]);
            original.put(loc, world.getBlockAt(loc).getType());

            Material newType = (x == 0 || x == width - 1 || yz == 0 || yz == height - 1) ? Material.GLASS : Material.AIR;
            world.getBlockAt(loc).setType(newType);
        }
    }
    PendingConfirmation.storeOriginalBlocks(player.getUniqueId(), original);
});```
night knoll
#

Running it the next tick?

woeful pasture
#

runTask can pretty much just take the task another tick to run in many situations as of how the queue works

# 
Bukkit.getScheduler().runTask(PixelPaste.getInstance(), () -> {
     originalBlocks.forEach((loc, originalMaterial) -> loc.getBlock().setType(originalMaterial));
 });```
 same thing here
night knoll
#

I see, there is a reason for this though. It's too keep the blocks placing on the servers main thread.

#

If you go far back in the code, when the method that has the code above in it is called, its called from another thread.

#

I may be wrong on my assumption here, I am still unfamiliar with the API. So if im wrong please tell.

woeful pasture
#

ahhh also for setting type I'm not sure how much you do this in one tick, but you might want to look at this thread that goes over distrobuting tasks over multiple ticks. I think the way you are doing it is fine unless the pastes get big

SpigotMC - High Performance Minecraft
Resource - Guide on workload distribution or "How to handle heavy s...

This guide is meant for everyone with intermediate/advanced experience with Java and Spigot.

Ever wanted to fill in a huge chunk of blocks without the...

#

I could be wrong I just don't see anything about distrobuting the task over multiple ticks if needed

sly jackal
#

prolly devide it up into a block count or size

#

block count would work for any dimension

woeful pasture
sly jackal
#

yes

#

I agree

#

I mean its hard to determine where the limit is right

night knoll
# woeful pasture ahhh also for setting type I'm not sure how much you do this in one tick, but yo...

Thanks again, for the resource.

If you have the time, I would like to ask a question about a certain portion of my plugin.

I am currently using GCD Algorithm to find the Greatest Common Divisor among blocks to prevent skipping and divisible placing of the blocks. However, when there is NO GCD between 2 numbers, it'll do the whole thing at once, causing server lag. Is there a way to prevent this? I know I can just use 1 as the GCD but that makes it super slow when building.

Heres the relevant portion of the code

public static int gcd(int a, int b) {
        return (b == 0) ? a : gcd(b, a % b);
    }

I'm not going to include the whole class that uses this process, you could probably understand from context clues.

sly jackal
#

but I was saying if you get over some large amount of blocks to do, you would probably do the rest in a separate worker

night knoll
#

I don't think GCD is the best approach due to this, however I can't really think of another algorithm that can be directly implemented into my project without significant functionality changes.

sly jackal
#

idk about the maths but if you can detect that spike beforehand that is probably where you would devide and conquer

night knoll
#

Thats a good idea. Your basically saying split it up until multiple chunk task that have GCD's?

#

If and only if there is no GCD with the base size.

sly jackal
#

well like I said im not sure about the actual maths but that sounds like a good approach

#

if its fast to actually get the gcd but slow to act upon it in certain outcomes, then this would def be a good way

night knoll
#

Getting the GCD is instant. Slow part of it comes when I tried defaulting it to placing 1x1 block per tick because I couldn't find the valid GCD. Your idea of seperating the task in a way that allows each chunk task to have a GCD will work best.

woeful pasture
#

if your issue is placing blocks is lagging the server you should use the work distro thread I sent above as reference to create a system to control how many blocks are placed per tick

night knoll
#

Placing blocks doesn't lag the server. Placing them all at once does. There is a 'control system' already to an extent. Which is GCD to loop through to place GCDxGCD blocks per tick. But I could expand on it with that documentation to make a more full proof system.

jagged rock
#

Not a huge fan of how you manually hardcode each color and block type rather than using a config

night knoll
opaque plover
round fossil
#

i mean yeah

#

any creational design pattern

opaque plover
#

can you tldr the difference between a provider & a factory?

round fossil
#

ugh

#

idk much about provider

#

I think thats just some spin off of a factory

#

guice uses that for their dependency stuff

opaque plover
#

fair enough, I'll look it up when I'm home

sleek shard
#

this seems like a fun thread - might aswel join it

night knoll
#

Why wont my NPC start moving? . The path is possible to reach from the starting location

📎 message.txt
#

Its repeately printing out the same debugs

[23:31:04 INFO]: Target Location: Location{world=CraftWorld{name=world},x=130.0,y=-60.0,z=-60.0,pitch=0.0,yaw=0.0}
[23:31:04 INFO]: Distance to target: 2.024280192220081
[23:31:04 INFO]: Setting NPC target to: Location{world=CraftWorld{name=world},x=130.0,y=-60.0,z=-60.0,pitch=0.0,yaw=0.0}
[23:31:04 INFO]: NPC is NOT navigating.
#

It works fine with teleporting the NPC but not using the navigator

spring reef
#

i would expect a start method on the navigator

night knoll
#

Nah It was an issue with out I was updating the index. I fixed it

spring reef
#

as a code reviewer i still would expect a start method on the navigator

night knoll
#

There is not a start method on the navigator

cobalt trellis
#

I think that class will be a bit too much of a pain in the future for me, but whatever ™️ https://gist.github.com/Geolykt/374679b451171d155a23d68ab6147b8a.
It's a 62-bit unboxed integer concurrent hashset - which makes use of next to no synchronization/locking. Unlike fastutil-concurrent-wrapper it also supports iterators!

round fossil
#

did u write any unit tests to test its thread safety

cobalt trellis
#

Not at all - especially considering that unit testing concurrent stuff is difficult to say the least.
That being said I am currently writing something that would catch most obvious defects.

cobalt trellis
#

Okay concurrent insertion seems to work mostly or at the very least I don't see anything out of the ordinary. Removal is fully untested though but that is mostly caused by that fact that my usecase only involves adding elements to the set (which is why the set cannot really shrink in size - that is once the memory is allocated it lives for the lifetime of the set).

round fossil
#

Ah nice, well I just really (and I think many others do) appreciate when the concurrency stuff is tested

#

Good shit geol :)

cobalt trellis
#

After writing some unit tests I came to the conclusion that the set somehow doesn't like the value 0. So yeah the impl is a bit bork right now but I didn't expect it to function well as-is anyways

cobalt trellis
#

Hm, looks as if that is caused by me not using CTRL_BIT_WRITE. Whatever, I'll ponder about this another day as the solution doesn't seem as straightforward as just using it. I guess I could just get rid of CTRL_BIT_WRITE which would allow for 63 bit integers but increment and decrement every single value by one for writing and iteration respectively.

cobalt trellis
alpine anvil
#

this is gonna close soon

sly jackal
#

liar

opaque plover
#

shalt not be closed

sleek shard
#

neverrrr

night knoll
round fossil
#

you need to split the logic up in smaller functions

night knoll
#

In the whole startAuthProcess method?

round fossil
#

that one, and the other maratoh one for asyncprelogin

night knoll
#

Yes... I was thinking about it

round fossil
#

good thought

#

:)

night knoll
#

It's a method monster 😂

round fossil
#

also like

#

private static final String IPV4_REGEX =
"^(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\." +
"(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\." +
"(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\." +
"(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$";
private static final Pattern IPv4_PATTERN = Pattern.compile(IPV4_REGEX);

#

why separate

#

well there are a lot of questionable things

night knoll
#

I don't remember why I did like this, probably because on my mind looked better than having it directly on the method

round fossil
#

int random = new Random().nextInt(100);

#

this is quite expensive also

#

prob wna cache the random at least

night knoll
#

No, because it's a catpcha code

#

And I want some characters to be strickethrough, and others don't

round fossil
#

no

night knoll
#

Oh

#

You mean the random instance

round fossil
#

I mean cache the new Random() part

#

ues

night knoll
#

Yes, that's right

round fossil
#

this is why also Math cached it

#

bcuz its expeeensive :>

night knoll
#

So, Math.random is the same as new Random but returning from 0 to 1 instead?

round fossil
#

yes

#

or well nextDouble iirc

night knoll
#

I think I will generate a new SecureRandom instance each iteration instead 😈

round fossil
#

but if u want a faster one u can use SplittableRandom

night knoll
#

SplittableRandom never heard of it

round fossil
#

well now u have :)

#

fast but more predictable to some extent

night knoll
#

Well, I don't really care if it's predictable

#

I just want it to have some letters to be "harder" to read

round fossil
#

yeah then use splittable

spring reef
spring reef
#

Math just hold one shared instance to not create it over and over again

#

you really only want to use Random when you use a custom seed

#

otherwise i can only strongly advice ThreadLocalRandom

round fossil
#

SplittableRandom is 3 times as fast as Random altho as said more predictable (and for multithreading you just split it) and it has reproducability even if you do parallel programming while thread local random in principle just isolates a random instance per thread

spring reef
spring reef
#

otherwise also that advantage is negligible

round fossil
spring reef
#

but we should get away from that rocket engineer thinking and just stick to the best practices, which is indeed, to reduce the amount of Random objects created

spring reef