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

1 messages · Page 1 of 1 (latest)

round fossil
#

The purpose of this (mega) thread is to let you, developers, get your code reviewed by others as well as reviewing other developers code.

Just a few things to think about whilst being here:

  • Be nice and respectful, remember that you are reviewing someone else’s work.
  • Be constructive, give feedback that is actually helpful, this is easiest done by providing explanations when you point something out.
  • Stay on-topic, for general Spigot and BungeeCord API questions, use #help-development. For other off-topic discussions use #general. For plugin assistance, use #help-server etc.
  • Do not forget to give them feedback about the things they have done correctly, that is just as important.

ℹ️ I will make sure this thread is never archived. If you see someone who is asking about getting their code reviewed in other channels, prompt them with the command ?codereview

round fossil
night knoll
#

epic

jagged rock
#

Sure, let's give it a try

#

Other than using souts (I'm a lazy bastard), what could I work on hmm

alpine anvil
#

jitpack.yml

jagged rock
#

I'll remove it, ik

#

it never worked

night knoll
alpine anvil
#

left in a todo, its literally so bad

image.png
jagged rock
#

It's what I do when it's 3am and I have stuff to do when I get up

jagged rock
#

Like make a class to just compress files with a specific given method, make a map

#

effectively changing that switch to a registry of compressors

round fossil
# jagged rock https://github.com/IllusionTheDev/Cosmos/tree/master/Cosmos-Plugin/src/main/java...

I’d use this keyword whenever possible to clarify what the caller is.

Use Path api over legacy File api.

Avoid making your lowest level classes depend on highest level ones, for instance FileDataContainer is loosely coupled to your entry point class. (You usually solve it being having a middle level class)

Some of your functions are somewhat too colossal such that it becomes hard to exit early when reading them if we just want to understand how some parts of it work.

#

Jitpack 🥲

jagged rock
#

as previously mentioned, it never worked

#

and I can't bother

round fossil
#

[MEGA THREAD] Get Your Code Reviewed or Review Others

jagged rock
#

Avoid making your lowest level classes depend on highest level ones, for instance FileDataContainer is loosely coupled to your entry point class. (You usually solve it being having a middle level class)

Not sure how I'd make FileDataContainer not rely on an instance of my main class

#

As it does some stuff under the hood

image.png
night knoll
jagged rock
#

Though yeah I could change some stuff around now that I think about it

night knoll
#

I would say the same as Conclure said, I would rather use Path instead of File

jagged rock
#

Ehh I've never really benefitted from the advantages of the Path API

#

and I'm quite old-fashioned :p

#

Just old habits

night knoll
round fossil
# jagged rock As it does some stuff under the hood

Yeah, well, your entry point class is technically becoming a data object/context object also since u use it to get other relatively high level classes, I’d probably try to give that duty to another class.

jagged rock
#

I don't think the Path API existed back in java 6

round fossil
#

Yea it didnt

night knoll
round fossil
#

But its problematic since is inconsistent across different platforms and can lead to DoS

jagged rock
#

Hmm changing FileDataContainer would be quite weird actually

#

I could make a middle class to manage this kind of stuff but it's up to the developer that's using the API, to actually chose what container things would be held in

#

You can perfectly load from mongo and save to sql as far as I'm concerned

round fossil
#

Yeah, well im speaking from an enterprise pov, obviously decoupling things too much compromises usability

jagged rock
#

Yeah this is not meant to be enterprise code

#

just a modular project

round fossil
#

Yeah all good, its nice with the interfaces tho

#

That makes me relieved

jagged rock
#

Well my idea was honestly just making sure that regardless of the use-case scenario you could always expand

#

So if a plugin 5 years from now wants to add amazon s3 or something

#

They could just implement an interface and register it

round fossil
#

Yeah robust design, this type of experience you only get after writing thousands of lines of code 🥲

jagged rock
#

I've suffered for long enough

midnight spade
#

Oooo

#

Great now I'm part of this thread:)

night knoll
#

this is gonna be fun

surreal peak
#

👀

night knoll
round fossil
#

nice but

#

Collections.newSetFromMap(new ConcurrentHashMap<>());

#

this is bad

jagged rock
#

Sets.newConcurrentHashSet

night knoll
#

My idea is to not depend on third-party dependencies

#

I mean, it's in spigot, but Sets#newConcurrentHashSet its not spigot 1.8 (for example)

I want to provide backward compatibility

midnight spade
#

why is that bad specifically?

#

and why is Sets.newCHS better?

night knoll
#

Which is slower and heavier than actually having a hashset

midnight spade
#

well

#

so does hashset

#

@night knoll

jagged rock
#

true but not concurrenthashset

#

google just uses this

image.png
#

So you'll be fine in this case

night knoll
#

So, instead of using Sets#newConcurrentHashSet I could directly do Set<X> set = ConcurrentHashMap.newKeySet() (speaking about backward compatiblity, as legacy spigot does not have Sets#newConcurrentHashSet)

jagged rock
#

Yeah pretty much

round fossil
#

newSetFromMap(new ConcurrentHashMap<>());

#

Like its way more bug prone than just using ConcurrentHashMap::newKeySet

proper relic
round fossil
#

Is this migration logic of sql to no sql?

surreal peak
#

id love to see flag encoding here

image.png
proper relic
proper relic
#

so you never have to type sql

surreal peak
#

FLAG1 | FLAG2 you know

proper relic
#

i dont know 💀

surreal peak
#

Less booleans everywhere

proper relic
#

doc link?

surreal peak
#

Using the bits of a int

#

For flags

#

Bit set basically

proper relic
#

im still so confused lol

round fossil
#

like what is the pros of turning the syntax semantic keywords into classes?

proper relic
#

easier dynamic use

round fossil
#

whats dynamic use?

proper relic
#

like when you don't know the data or tables/columns/rows at hand

round fossil
#

but isnt the syntax also just reusable at hand?

#

sql itself?

surreal peak
#

instead of those booleans saying nothing

proper relic
#

those flags say a whole lot 🤣

#

arguably worse bc i have like a type annotation

#

so it shows what the value is before the boolean

round fossil
#

I dont wanna come off as rude or annoying, but this abstraction over sql seems rather useless, like usually you wanna create an abstraction in terms of object relational mapping but that’s it

proper relic
round fossil
#

Yeah thats fair

opaque plover
#

i'll just join the thread so that i don't forget it exists

rancid fern
#

Same

gilded gate
rancid fern
# gilded gate https://github.com/Psikuvit/Pharaoh-Factions

Pharaoh_Factions does not follow conventions
Use DI instead of a static plugin instance
Move PlayerMenuUtillity somewhere else instead of keeping it in Pharaoh_Factions
FactionsMethods should be renamed to something else
Some of the classes in util package aren't util classes

gilded gate
rancid fern
woeful pasture
gilded gate
#

ohhh

rose lark
#

Yeah, related to Olivo's point - dont store the plugin instance in an interface

image.png
rancid fern
#

Added a few more notes to my message above

gilded gate
gilded gate
#

do I use DI even in the FactionMethods class?

rancid fern
rancid fern
gilded gate
#

its not

woeful pasture
rancid fern
woeful pasture
#

cuz its fan art

rancid fern
#

The artist still has the rights to that image

#

Do they allow you to use it

rose lark
#

thats why i drew my pfp in paint

woeful pasture
gilded gate
rancid fern
surreal peak
#
  • Not really sure about that Pharaoh_Factions::getMenuUtility, could just use menus::computeIfAbsent instead of those three map lookups, also consider using an UUID instead of Player objects
  • In your Faction class, where are your private final -_- (in a lot of places actually) and don't expose those collections, instead you can create methods that f.e. add a member, remove etc.
  • In your FactionsDataFiles::loadFaction I'd return the actual faction rather than doing multiple things in one place (constructing it and adding it somewhere), you still have to use it where you call that method so why not directly return it? Also that class is kinda messy
  • I'd implement some kind of caching too so you don't have to keep loading or saving files when it's not needed
  • Menu.FILLER_GLASS might actually be static, this might help you with your guis: https://www.spigotmc.org/threads/a-modern-approach-to-inventory-guis.594005/
  • Looking into the future, your Messages class might hold some kind of caches instead of having constants (believe me, i have around 300 lang keys in my plugin and having 300 fields goes brr)
  • For the remaining: could recommend to use an itembuilder, some of your classes might be records and the rest has already been said by other people
#

dang markdown actually works for once

woeful pasture
gilded gate
surreal peak
#

ya, not theres no reason to have a player as key in a map tho

gilded gate
#

alr thx

#

I made some changes will you review it again after I commit?

surreal peak
#

sure

gilded gate
# surreal peak sure

I'd implement some kind of caching too so you don't have to keep loading or saving files when it's not needed like save stuff in a list then save it or load it when server starts/stops?

surreal peak
#

ye, i saw you were saving files in your setFaction method somewhere

alpine anvil
surreal peak
#

maybe think about some player wrapper that holds the faction adns tuff ig

gilded gate
surreal peak
alpine anvil
#

ouchers

surreal peak
#

lmao

#

Claim might be a record if your java version supports it

cobalt trellis
surreal peak
#

no resourcebundles 👀

cobalt trellis
#

pft. I can do everything with json

surreal peak
cobalt trellis
surreal peak
#

dang the formatting

cobalt trellis
#

Yeah for whatever reason I decided to use tabs there

surreal peak
#

wait thats supposed to be like that 😳

#

good enough

cobalt trellis
gilded gate
surreal peak
#

probably want UUID->Faction and i recommend you to write things like Map<X, Y> m = new HashMap<>() so dont put the actual type as type in the declaration but use an interface or smth, will save you from later headaches when you decide to change the type

#

and for me: every parameter that i see with final in front of it, i will bonk you

round fossil
#

You also have liskov’s substitution principle

lunar nacelle
#

any improve for this ?

#

or is there anyway for me to moduelize each part?

surreal peak
#

probably dont expose collections, and if you really need to (like for those rewards), instead of returning null, return Collections.emptyList()

lunar nacelle
#

wdym?

jagged rock
#

Also follow proper order conventions, where your constructor is the first thing after variables etc

#

And I'd probably make a sort of level enum and use a map instead of a bunch of longs

#

Other than that, repeat yourself a bit less

surreal peak
#

and use && and ||, people dont seem to know those exist

jagged rock
#

traditional for loops already account for nullability and empty collections so you can bypass all this

image.png
lunar nacelle
#

but if is it null

jagged rock
#

fully name your variables instead of labelling them as t

lunar nacelle
#

check if it is empty will return in an exception

surreal peak
#

a traditional for loop doesnt work on null

jagged rock
#

NPE or just no iterations?

lunar nacelle
#

I was using for instead of foreach previously

surreal peak
#

npe because for loops translate to Iterable#iterator

jagged rock
#

huh sure

#

sanitize inputs then

#

I'd also uhh

surreal peak
#

and for each loops use that too, only not for arrays, where its an indexed loop internally

lunar nacelle
jagged rock
#

Yeah with that level enum you end up nuking half of this

image.png
lunar nacelle
#

wdym by level enum?

jagged rock
#

I can't figure out a good name for this but just an enum with PRESTIGE, REBIRTH etc

lunar nacelle
#

:l?

jagged rock
#

at work we use an economy registry system to standardize all our currency types

lunar nacelle
#

hmmm

jagged rock
#

here you could do something like that, too

#

but honestly this does seem like quite a bit of clutter for what I'd consider a data class

lunar nacelle
#

hmm :l the data do it to acess in in PrisonPlayer class

#

like prestige need to access player rank

jagged rock
#

something I'd consider would probably be making an ExecutableCommand class and shifting all the console command logic to that

lunar nacelle
#

as well as rebirth and etc

jagged rock
#

for your rewards

#

and just overall splitting all this logic

#

idk this seems like a violation of the Single Responsibility Principle

lunar nacelle
#

:l?

#

why

#

player datas are storaged in prisoner object

jagged rock
#

true

#

but you're doing more than that

lunar nacelle
lunar nacelle
jagged rock
#

Split it up

lunar nacelle
#

yeh but how :l

jagged rock
#

At work we do have a user class but it's literally just a map wrapper

surreal peak
#

ew

jagged rock
#

well

#

more than that

#

it has transient data and data that goes to a database

surreal peak
#

when you realize javascript objects are just a Map<String, Object> 😂

jagged rock
#

then you realize objects are just a Map<String, Object>

surreal peak
jagged rock
surreal peak
#

although its all memory

lunar nacelle
#

or somehow I could split rebirth and prestine and phoenix to other class ?

#

or modulize it?

#

I'm not that comfortable to this to be 700 lines :l

surreal peak
#

only 700

gilded gate
woeful pasture
#

first thing, is naming conventions

jagged rock
#

Immediately not a fan

woeful pasture
jagged rock
#

There's also liskov substitution

gilded gate
#

IKKKK I will change it laterrr

jagged rock
#

and the misuse of Objects.requireNonNull

woeful pasture
#

don't wrap everything in Objects.requireNotNull

jagged rock
#

You also don't fully name your variables, treat your main like a data class

woeful pasture
jagged rock
#

well

gilded gate
#

can you tell me what is wrong to fix it you kinda confused me

jagged rock
#

basically the idea that you should like

#

use the lowest interface possible while retaining functionality

#

fancy term for List<Whatever> whatever = new ArrayList<>(); instead of ArrayList<Whatever> whatever = ...

#

Basically replacing the type in the left should still make sense

gilded gate
#

I only use List<>

jagged rock
gilded gate
#

do I use Map?

jagged rock
#

also your command system is a bit messy

#

and you hard-reference chunks which will cause nasty memory leaks

image.png
#

which all goes on a static hashmap because you never bothered to make a manager / registry class type deal

image.png
gilded gate
gilded gate
jagged rock
#

You also hard-reference to players which is another nasty leak

image.png
gilded gate
jagged rock
#

exposing collections directly, which is a violation of encapsulation

image.png
gilded gate
jagged rock
#

and you're also trying to load players on startup which will cause issues

image.png
#

Instead of hard-references you use things like the player's UUID

#

Or the chunk's location or something

#

For liskov substitution, aim to always use the lowest possible interface for you stuff (on the left)

#

so
Map<Key, Value> map = new WhateverMap<>();

#

instead of WhateverMap<Key, Value> map = ...

#

Just helps with the process of changing to something else

#

Same with lists and sets

#

Your command stuff is messy and copy-pasty

#

And you don't seem to have an item utility class which means you're manually creating itemstacks which is just plain dumb

gilded gate
#

@jagged rock sorry I cant understand the solution

#

can you give me an example of what to do to use ClaimUtils without static's

jagged rock
#

make an instance

#

nuke all static

#

done

gilded gate
woeful pasture
#

make a static

#

instance like so

# 
public class ClaimManager { 
  private static ClaimManager instance;

  private final Map<UUID, Claim> claims;
  private ClaimManager(){
    this.claims = new HashMap<>();
  }

  public static ClaimManager getInstance(){
    if(instance == null){
      instace = new ClaimManager();
    }
    return instance;
  }
}```
This code ensures only one instance ever exists of ClaimManager and ensures you control everything on map of what you expose
jagged rock
#

ew singleton

gilded gate
#

ohhh so it doesn't allow more instances so the hashmap stays the same

#

ok I get it now

woeful pasture
#

Singleton is beautiful when used correctly

#

ore you could probably just do what ImIllusion wants and pass around a single instance with DI, though that doesn't make much sense with a project unless you plan to expand this APIs use to more plugins

gilded gate
#

I will expand it in the future

#

so do I just use DI?

woeful pasture
# gilded gate I will expand it in the future

expansion only matters with singleton if you need to reuse the manager class, which in most cases you don't need to so if you think you'll ever need more than one instance of the ClaimManager class use DI

gilded gate
#

that's one thing what do I have to change too

#

@jagged rock went kinda too fast and I wasn't able to process all of that

woeful pasture
#

who ghost pinged me :(

jagged rock
#

the lack of futures

woeful pasture
#

hmmm I was debating whether it was in scope to actually add that stuff

#

I was kinda wondering that the whole time I was making it

gilded gate
#

btw do I put this inside the method or out?

image.png
woeful pasture
#

Generally I think its convention to put this stuff in constructors, but that is personal preference

gilded gate
#

alr thanks

#

will it be even better if I put it in the interface class?

#

for easy access

#

@woeful pasture

woeful pasture
#

keep that stuff out of interfaces

gilded gate
#

alr thx

alpine anvil
jagged rock
#

I'd add a Plugin suffix to your main class name

#

use a proper command engine

#

if(whatever) return; ->

if(whatever) {
  return;
}
#

Overall stick to proper defined conventions instead of doing your own, 1-liner ifs like that are not well seen among the top

rancid fern
#

SneakyThrows 💀

jagged rock
#

You can @Getter the whole class instead of each field

#

Separate your library loading and discord bot logic

#

add spacing and all

alpine anvil
jagged rock
#

Pre-cache your files with separate class rather than reading config sections all the time

#

also the fuck is this

image.png
alpine anvil
#

my way to not use the libraries feature and have direct downloads

jagged rock
#

lot of repetition

alpine anvil
#

dont question why i dont want to shade my lib

jagged rock
#

getMessageConfig like 25 times

alpine anvil
#

yeah i need to fix my dry stuff

jagged rock
#

might as well make a proper MessagesFile class which handles all the coloring and formatting and that

#

Which is what I use

#

messages.sendMessage(sender, "no-permission");

#

accepts either stringlist or string, colors

#

I can add placeholders if I want

#

or have a function lambda

alpine anvil
#

yeah, i have a MessageConfig class in my lib that handles messages.yml, then another class for formatting

alpine anvil
#

i need to fix my cmd stuff

jagged rock
alpine anvil
#

my storage handler stuff is my main issue rn

#

bc i did have the cache in the sub class but i realised there isnt a need to load all the playerdata from sql stuff on startup

#

probably will start with that

surreal peak
#

also dont expose the votes in your DiscordUserData, same for PlayerData, separate the listener part from the StorageHandler, that has nothing to do with storage, I dont think SuggestionHandler should have access to disabling the plugin, the exception should be caught in onEnable and the plugin should be shut down there

#

and a static getInstance()

candid osprey
jagged rock
#

a few things

#

1 - don't repeat getFastBoard like 25 times
2 - single letter variable naming? You can do better than this
3 - hardcoded messages, ew

surreal peak
#

wait until you look at WarpController

#

oeh not closing readers

jagged rock
#

did you know

#

you can make empty lines

#

for readabilty

woeful pasture
#

Am I missing something where is the fastboard cache

#

I can't find it but fastboards should be cached for updates unless you plan to never update them ever

candid osprey
candid osprey
woeful pasture
candid osprey
#

Like a hash map

#

of uuid and fastboard

woeful pasture
#

yes

candid osprey
#

I'll do that, thank you

#

damn feedback here was very fast

woeful pasture
#

I've got a couple other things, but I'll take time to type it out

#

a huge thing with OOP is single responsability, you really don't want classes having multiple purposes

https://github.com/Bonnie20402/SimpleWarpPlugin/blob/master/src/main/java/bonnie20402/simplewarpplugin/controllers/scoreboard/BaseScoreboardController.java
In its current form this class doesn't really make sense, I'm seeing a Listener that should be registered within a Object that should be specific to a player. As I stated above UUID -> FastBoard map should be managing this, or if you want to wrap your custom object UUID -> BaseScoreboardController.

    @EventHandler
    private void onPlayerQuit(PlayerQuitEvent e) {
        if(this.getFastBoard() == null) return;
        if(this.getFastBoard().getPlayer() == e.getPlayer()) {
            this.getFastBoard().delete();
        }
    }``` This code shouldn't  be anywhere near a object that is driven by a player
It'd be best if you just removed this Listener from BaseScoreboardController and made a designated class for the Listener.
#

https://github.com/Bonnie20402/SimpleWarpPlugin/blob/master/src/main/java/bonnie20402/simplewarpplugin/controllers/spawn/SpawnController.java
this class isn't really readable at all, you should probably use ide auto formating or just space the variables out a bit
this.spawn=spawn doesn't follow java's conventions it should be this.spawn = spawn only two spaces difference, but it adds up to make something less readable.

You can see all of the java language conventions here https://www.oracle.com/java/technologies/javase/codeconventions-namingconventions.html

#

^ This also applies with WarpController class

#

With your commands its very minor, but for User Experience sake you should probably consolidate most of this into one command with subcommands.
My personl favorite way to do t his is with a Map<String, CommandExecutor>. You can implement this yourself, or you could also use a well established Command Framework like ACF which allows easy creation of Subcommands etc. If you want to see an example of the method I suggested check out the github page here,
https://github.com/Y2K-Media-Creations/ErisCL

#

hopefully this helps

candid osprey
# woeful pasture Next I think the way you have MainScoreboard is nice and all, but you really sho...

thank you for your time!
I'll reformat the code following orcale java conventions that you linked.
About the commands, I don't think I need to use a framework (yet).
But man you are like the god of spigot or something, thank you for the libary.
I myself found the bukkit config system a little but weird, thus my usage of gson. The bad thing with gson is if I want to save any spigot object I have to write my own adapter for it.

About the listener:
I saw that the boards should be deleted each time the player quits (to prevent memory leaks maybe? idk) and I figured it should be the same for ALL of the boards, thus the baseScoreboardController implementing Listener. I'll separate it into another class as you told me to.

I (ik im a noob) actually never touched HashMaps and HashSets in my life. At school I learnt C, and I'm learning still java. (got a spring boot appointment to do lmao). but tbh school doesn't teach shit.
According to a quick reserach it seems that hashsets prevent duplicates, which is going to be good for my case, but they're slower than hash maps. Source: https://www.geeksforgeeks.org/difference-between-hashmap-and-hashset/. What would you use?

GeeksforGeeks
Difference between HashMap and HashSet - GeeksforGeeks

A Computer Science portal for geeks. It contains well written, well thought and well explained computer science and programming articles, quizzes and practice/competitive programming/company interview Questions.

woeful pasture
woeful pasture
candid osprey
#

Yes but a fastboard needs a player when created

woeful pasture
#

make a Manager class for your scoreboards

#

I'll lay it out more clearl with the Scoreboards you should have atleast 2 or 3 classes minimum
ScoreboardManager -> contains Map<UUID, FastBoard> and all relevant methods for accessing / controlling them
PlayerJoinListener -> Adds to scoreboard manager on player join
PlayerLeaveListener -> Removes from ScoreboardManager on leave to prevent memory leak

you could merge the listeners and do
PlayerJoinLeaveListener

candid osprey
#

Alright

candid osprey
#

I really enjoyed doing it this way

woeful pasture
#

almost

#

with Manager Pattern you never expose the collection directly

round fossil
#

Meh sometimes its fine

woeful pasture
#

instead of having getBoards you'll want

public FastBoard getBoard(final UUID uuid){
  return this.boards.get(uuid);
}
round fossil
#

But usually you’d return a defensive copy then

candid osprey
woeful pasture
#

yes, same with all operations your doing on the map

#

make a method for it instead of exposing the collection

candid osprey
woeful pasture
candid osprey
#

My goal here is to
-> make abstract base class that contains all of the boilerplate stuff

-> extend that same class with like a scoreboard for the world X, and another for the minigame Y, etc
is this a good aproach?
(you get what i mean?)

surreal peak
woeful pasture
candid osprey
#

ok

round fossil
#

Boilerplate refers to code that’s just there but doesn’t really add up to anything, for instance some of the syntax when writing a getter in Java can be considered boilerplate as its just unnecessary code one has to write every time a getter gets added

candid osprey
#

my english isn't the best

#

qwp

#

quite off topic, but anyone knows of some CLI based minecraft client?

#

Kind of just, uh, to spawn a player

rancid fern
#

Use Mineflayer

candid osprey
#

that povides basic chat input/output

#

I'll check it out

opaque plover
#

I just started getting into Python and I decided to make a quick parser. It's probably useless but it feels like a good start. At the bottom of the code is an example of a parsable configuration.
https://pastebin.com/z6qDq6Yi
I would like this to be reviewed 🙂

rancid fern
#

Python 💀

opaque plover
#

Yep. I am experienced with Java and Kotlin, why not learn something else too?

jagged rock
#

MinecraftConsoleClient

candid osprey
#

I never touched python in my life.
I heard the syntax is simple

jagged rock
#

it's made in C# and I have patched it before

candid osprey
#

I forgot I have to make my server offline because i have no other mc accounts lmao

#

🏴‍☠️

rancid fern
#

You can learn Dart uwu

#

Great language

opaque plover
woeful pasture
#

when working with python you should try to adhere to more functional concepts

round fossil
#

Yeah or just procedural in general

opaque plover
#

It's still possible to let it be a separate module without an OOP approach, right? My current usage of the parser is as follows:

import config_parser # me
import asyncio
import telegram
import logging


logging.basicConfig(level=logging.DEBUG, format="%(asctime)s - %(name)s - %(levelname)s - %(message)s")


async def main():
    parser = config_parser.RConfigParser(".rconfig")
    if not parser.has("token"):
        parser.set("token", input("Enter token: "))

    token = parser.get("token")
surreal peak
#

learn odin

#

its data oriented

#

why is the main method async 🤔

opaque plover
opaque plover
surreal peak
#

still dont get it

opaque plover
#

It should probably run async

surreal peak
#

i never understood what the point was of running smth async but then awaiting it

#

seems like the same as running sync to me, although it isnt

opaque plover
#

Good point actually

surreal peak
#

like in js, its all singlethreaded so async tasks run in another part of the v8 runtime

#

those tasks can run in parallel, but i still dont get it smh

round fossil
#

Its async because in py

#

You can only await async functions if the function itself is async

opaque plover
#

I don't have nearly enough experience with async stuff in Python or at all, so I'll have to pick that up. I still have material on basic Python to learn, I just wanted to put up something quickly for the sake of experience 🙂

round fossil
#

So it makes sense

#

@surreal peak

opaque plover
#

Thus this code from the docs, I just copied it haha

surreal peak
#

why not run the function youre awaiting on in sync then

#

in java it all makes sense

round fossil
#

Idk maybe the module he is using it has a bunch of async functions

surreal peak
#

my discord client is awaiting on every async method too 🤔

opaque plover
#

That I don't know. If the docs advise me to do so and I have no idea how to do it otherwise, I'll just do it how they advise

round fossil
#

Mye

candid osprey
#

I feel like I'm unessecary absteacting stuff away, but at the same time I feel it's more organized.

round fossil
#

it looks alright

#

Im not a fan of coupling logic in the constructor

#

(I'd use factory, or other creational patterns to deal with it fluently)

#

Also when you have a model view controller object you usually have an "underlying" unsafe class that allows to fully, and unsafely operate on an instance, then you have one view class, and one controller class both delegating to this instance, however operations through these are much safer to use (usually by you restricting input parameters by exceptions etc)

#

(@candid osprey :] )

candid osprey
#

So, this is must likely going to be full of bad practises, but im trying to re-use code.
(please have some patience as I sometimes may fail to explain myself propely)
It's my first time using the <> thingy (a class that expects a parameter?)
I'm trying to "override?" a super class variable, on my BasePlayerGuiController but it's not working. The plugin compiles, but Cannot invoke "bonnie20402.simplewarpplugin.guiviews.base.BaseGuiView.getGui()" because "this.guiView" is null I guess it's trying to get the guiView from the superclass and not from the child.
https://github.com/Bonnie20402/SimpleWarpPlugin/commit/393ab2220d2d08c5acfa45b1d0769dcb96ec3642

GitHub
Trying to abstract logic away. · Bonnie20402/SimpleWarpPlugin@393ab22
#

Thank you for your precious time ^_^

#

I'm guessing I'm doing something bad because the IDE itself warns me this

image.png
surreal peak
#

its generic

gilded gate
#

I have this code to modify the result of a itemstck in the smithing table

#

everything works fine but when I try to collect to the result it just get placed back

#

like I can pick it up but it get instanly placed back

woeful pasture
#

This thread is for code review not really code help I'd suggest you place this question in #help-development

candid osprey
woeful pasture
candid osprey
#

Yes

woeful pasture
candid osprey
#

Also pawn

woeful pasture
candid osprey
woeful pasture
#

in HomeModel you probably don't want #setOwner you honestly it should be stressed that the owner shouldn't change

candid osprey
#

true good observation

#

Lol I clicked the icon

woeful pasture
#

this is in HomeController.java

candid osprey
#

So maps are faster than doing a for each time

#

(I guess I’m too used to C)

woeful pasture
#

well I'd assume Maps are still faster in C, but don't you have to make your own map implementation, though I'm sure there is a libraries at this point lol

candid osprey
#

Everything is fase ( and dangerous ) in C

#

Fast*

woeful pasture
#

aha I know that much

surreal peak
#

big O notation is a lie anyways

surreal peak
#

gotta make one yourself if you need one, and remember instance methods dont exist

woeful pasture
#

you should use what is proper for the situation]

round fossil
#

Or I mean it is but yesnt basically 😅

surreal peak
#

yesnt thats the word

cobalt trellis
#

It isn't a lie really but generally there isn't a singular big O

surreal peak
#

foolish people just think that it means something while in reality it means something different

#

damn thats a nice sentence

cobalt trellis
#

And then you have to keep in mind that some ops are inheritly more expensive

surreal peak
#

ye

cobalt trellis
surreal peak
#

idk they interpret it the wrong way

#

i was like going to say smth smart but its too early for that

cobalt trellis
#

I don't see how you can get it wrong

surreal peak
#

well focussing on the wrong optimisations

round fossil
#

Yeah, I mean its theoretically useful for scalability mostly

candid osprey
surreal peak
#

dont directly expose that map

candid osprey
#

alright

#

but is making a map just for messages efficient?

surreal peak
#

ye

candid osprey
#

it also "saves" memory by not making me create a new hashmap everytime I want to send a message (is this right?)

surreal peak
#

and in your homecontroller, please only create that gson object ONCE, if you look at its constructoryoull get it

surreal peak
candid osprey
candid osprey
rancid fern
candid osprey
rancid fern
#

Liskov’s substitution principle uwu

proper relic
#

@surreal peak what ```java
public void renameColumn(Column column, String newName) throws SQLException {
getDatabase().getStmt().execute("ALTER TABLE " + getTableName() + " RENAME COLUMN " + column.getColumnName() + " TO " + newName + ";");

        if (primaryKey.getKey().equals(column.getColumnName())) {
            primaryKey = Map.entry(newName, primaryKey.getValue());
        } else if (keys.containsKey(column.getColumnName())) {
            keys.put(newName, keys.get(column.getColumnName()));
            keys.remove(column.getColumnName());
        }
    }``` I dont even loop through it once
alpine anvil
#

you frickin what

proper relic
#

fghj

#

fourteenbush says i looped through a hashmap twice

#

but like no i didnt

surreal peak
#
  • keys.containsKey(column.getColumnName())
  • keys.put()
  • keys.get(column.getColumnName())
  • keys.remove(column.getColumnName())
proper relic
#

yeah>

#

What's wrong with that

surreal peak
#

could start with keys.replace already

#

and dont do containskey, to then immediately do a get, do a get and check the result

proper relic
#

I see

surreal peak
#

i probably missed between containsKey and containsValue too at some point

proper relic
#

so I would get, if null, put, else replace.

surreal peak
#

ah wait im messing up between key and value for that one

#

but ye could probably use replace

#

and use keys.put(newname, keys.remove(columnname))

proper relic
#

wtf

#

does keys.remove return the value

surreal peak
#

ye

proper relic
#

like pop

surreal peak
proper relic
#

sweet

#

like this @surreal peak ? ```java
public void renameColumn(Column column, String newName) throws SQLException {
getDatabase().getStmt().execute("ALTER TABLE " + getTableName() + " RENAME COLUMN " + column.getColumnName() + " TO " + newName + ";");

        if (primaryKey.getKey().equals(column.getColumnName())) {
            primaryKey = Map.entry(newName, primaryKey.getValue());
        } else {
            DataTypes dataTypes = keys.get(column.getColumnName());

            if (dataTypes == null) {
                return;
            }
            keys.put(newName, keys.remove(column.getColumnName()));
        }
    }```
surreal peak
#

sec

proper relic
#

kk

rancid fern
surreal peak
#

actually do keys.remove on the dataTypes

proper relic
#

not end users

rancid fern
#

Put a warning somewhere

surreal peak
#

does a where work on alter?

proper relic
#

alright

rancid fern
#

If that's a library you should make it safe just in case

proper relic
#

is there an easy method to sanitize

#

yeah you're right

rancid fern
#

PreparedStatements

proper relic
#

Prepared statements?

#

why would that make a diff

surreal peak
#

otherwise call your table user TO user2; DROP schema public;-- :)

#

or smth, i quit doing sql injection lol it was too boring

rancid fern
#

You use them without string concatenation

surreal peak
#

lets all hope he checks his column name :)

proper relic
#

whateathgae

rancid fern
surreal peak
#

but ye anyways

public void renameColumn(Column column, String newName) throws SQLException {
            getDatabase().getStmt().execute("ALTER TABLE " + getTableName() + " RENAME COLUMN " + column.getColumnName() + " TO " + newName + ";");

            if (primaryKey.getKey().equals(column.getColumnName())) {
                primaryKey = Map.entry(newName, primaryKey.getValue());
            } else {
                DataTypes dataTypes = keys.remove(column.getColumnName());

                if (dataTypes == null) return;
                keys.put(newName, dateTypes);
            }
        }``` and make it DataTypes, not types
proper relic
#

ooh hot docs

#

where do you see types?

#

ohh

surreal peak
#

DataTypes

proper relic
#

but what about removing the old column

surreal peak
#

-> DataType, that object only is one type

proper relic
#

oh variable nmaing okay

surreal peak
#

well keys.remove(column.getColumnName()); removes old column

proper relic
#

OHH

surreal peak
#

if it was there, [newname, type] gets inserted

proper relic
#

shit man

#

thats genius

#

ok time to look at prepared statements

surreal peak
#

never knew those actually worked for column names

#

and ive never actually paid attention to what happens when you try to do sql injection

proper relic
#

yeah lmao

#

This is the proper way to use prepared statemnts? java PreparedStatement preparedStatement = table.getDatabase().getC().prepareStatement("SELECT " + column.getKey() + " FROM " + table.getTableName()); ResultSet resultSet = preparedStatement.executeQuery();

surreal peak
#

i believe olivo wants to say SELECT ? FROM ? and then setting them

proper relic
#

How does that work?

#

do I just put ?

#

then specify the index

#

and the elemnt

#

with like .setblah

surreal peak
#

believe so

proper relic
#

kk

# 
PreparedStatement preparedStatement = table.getDatabase().getC().prepareStatement("SELECT ? FROM ?");
        preparedStatement.setString(1, column.getKey());
        preparedStatement.setString(2, table.getTableName());
        
        ResultSet resultSet = preparedStatement.executeQuery();```
#

its weird how it starts from 1

#

but i guess it works

rancid fern
#

There we go

#

no more sql injecting

proper relic
#

lessgo

#

fuck injectors

#

all my sql uses more

alpine anvil
#

what are those getters

#

getStmt, getC

proper relic
#

lombok

#

when i name connection C and statement stmt

#

lol

#

@rancid fern if I have a value that may be an int, or a string, and I have a way to check if it needs '' around it or not, can I just use setString even if its gonna be an int?

rancid fern
#

You could probably use set string

proper relic
#

alright bet

rancid fern
#

It will probably be slightly slower performance wise when it's an int

#

but that's negliable

proper relic
#

damn alright

#

ill benchmark when im done

rancid fern
#

eh no need that's just micro optimization

proper relic
#

okay fine

surreal peak
#

theres a setObject too

proper relic
#

Ill use

#

wait

surreal peak
#

or just check the type of your param with a switch

proper relic
#

thats gonna be annnoyinggggg

#

I use object

#

ez

sly jackal
#

it might be quite a lot but if you are willing to flip through the classes I will be thankful 😅

surreal peak
#

{ on new line

#

BLEH

sly jackal
#

please look past that

woeful pasture
#

"please follow java conventions please" laugh

sly jackal
#

I am a c++ dev for a living so yeah

surreal peak
#

dont do that in c++

sly jackal
#

well too bad

#

if thats the only bad thing ill take it

surreal peak
#

you are the c# guy from now on

alpine anvil
#

average net beans user

#

why dont you handle tab complete in the same class as commands

woeful pasture
woeful pasture
surreal peak
#

probably wants to print a warning, but shouldnt be present in production code

image.png
woeful pasture
#

wait is there even a reason command would ever be null unless you didn't include it in your plugin.yml

surreal peak
#

no

woeful pasture
#

then that's kinda a useless check and if something happens you should probably let it error out

sly jackal
#

okay good to know

#

I thought a separate class for tab completion was the only option haha

surreal peak
#

seriously

image.png
woeful pasture
sly jackal
#

you mean in one file?

proper relic
#

There’s a class for both interfaces in one to make it even easier!

woeful pasture
surreal peak
#

uh getPlugin() everywhere in main file

sly jackal
#

or like as a child class

woeful pasture
#

you can have an infnite amount of interfaces

sly jackal
#

what should I use instead

woeful pasture
#

make a private static instance or just make a getInstance() if you need it that much

sly jackal
#

well yes I know, I guess i just didnt think about it when making the command, noted

woeful pasture
#

usually I'd opt for Dependency Injection though

sly jackal
#

yes

#

I agree

surreal peak
#

could probably do Math.max(0, delay) here, runTask already calls runTaskLater(.., 0)

image.png
alpine anvil
surreal peak
#

computeifabsent

#

and avoid those unneeded lookups

civic shell
surreal peak
#

uhh creating a new object to compare with?

image.png
surreal peak
#

and remove those public fields, this aint c# properties lol

surreal peak
#

also use early returns and clean up some of those command classes, looks like a big spaghetti monster now

#

havent looked at half of the classes

#

about whose code are we even talking

#

ah

#

im not

alpine anvil
surreal peak
#

private static SimpleSuggestionsPlugin instance;

#

expect a visit from me this evening

alpine anvil
#

i will send my dog on you

surreal peak
#

good to know

sly jackal
#

was this also meant for toast?

surreal peak
#

i bet youve heard about the term interface

image.png
alpine anvil
#

yeah

#

i forgot to make that an interface again

surreal peak
#

also this is useless

image.png
alpine anvil
#

7smile will scream at me

surreal peak
#

either make it extends runnable but handler::saveData is already one

sly jackal
surreal peak
#

Optional ew

image.png
sly jackal
#

ok

surreal peak
#

so many optionals created for no reason

alpine anvil
#

dont question it

#

i made chatgpt write it

#

bc gson confuse dme

surreal peak
#

HA CAUGHT YOU

sly jackal
surreal peak
alpine anvil
surreal peak
#

whas the "" doing here

image.png
sly jackal
#

i dont get it

#

what do I save conpared to what it is right now

alpine anvil
surreal peak
alpine anvil
#
  1. make my storage handler interface
  2. finish my sqlite
  3. rewrite json storage handler and remove optionals
surreal peak
#

nextId for what

image.png
sly jackal
#

okay maybe i get it if I rewrite it using computeifabsent

alpine anvil
surreal peak
#

doesnt sqlite have sequences or autoincrement?

alpine anvil
#

i have to know the next id

surreal peak
#

ah

#

also getItemMeta() creates a new one every time (maybe pdc) too and pdc::getOrDefault

image.png
#

oh and OptionalInt class exists

alpine anvil
#

do i have a double space between static and the O

surreal peak
#

ye 💀

#

wait what * items per page / items per page and that on ints

image.png
#

that supposed to do anything?

alpine anvil
#

for invs

#

where the 1st id would be

#

i dont remember why that isnt just a constant

surreal peak
#

chatgpt probably

alpine anvil
#

the inv stuff i rewrote off coll

sly jackal
#

lmao

novel meteor
#

Just found out about this thread.
Do I just drop a repo here and wait for angry ppl to tag me ? :D

surreal peak
#

basically

novel meteor
#

Hmmmmm MeguThonk
Seems fun :)
I think I'll do this once I get home

lunar vale
novel meteor
round fossil
#

best name I've ever seen in a while

novel meteor
#

tyty
idk why but those classes sometimes don't get included in the jar
I just gave up on solving that and did this terribly thing :D

round fossil
#

yeah, just why do u use json simple?

novel meteor
#

It was the first JSON library I found, I am used to using it.

round fossil
#

ah okay

#

because it sucks

#

so dont use it anymore

#

also interface IJSON

novel meteor
#

hahaha
maybe if you give me a good enough reason

round fossil
#

jackson, gson and moshi are better

#

like they are just better

#

and iirc json simple just extends Map, List and pass Object type to the type constructors or some weird shit

#

its just so awkward to work with

novel meteor
#

I find it quite ok but shrug

round fossil
#

for interfaces

#

don't use the prefix I

novel meteor
#

wans't that a java convention ?

round fossil
#

it was used to tell the difference between interfaces and classes when extending them

#

but in java u have extends and implements keyword

#

also this static init() stuff

#

u can sometimes use a static initializer instead

# 
static {

}
#

also why do u use this weird convention

novel meteor
#

hmm I wonder why I have not used it here acutally...

round fossil
# 
void function( 
{
  if (x)
  {
  }
}
#

its called allman style

#

and its used in C#

#

but rarely in Java, I suggest not using it to be consistent with the rest of the java community

novel meteor
#

you mean the brackets ?
That goes way back with me
I watched some java tutorial and a person was using that style, so I do now as well

round fossil
#

lets just say I would never sit in the same car as that person

#

private static Plugin PLUGIN;

#

ur plugin is not a constant

#

even if u were to have that variable be final

#

its not a constant, so don't name it like one

novel meteor
#

ok ye that one makes sense :D

round fossil
#

yea, well thats just a minor issue

#

I think the general issue here is that u use way too much static

#

dont worry

#

its not forge

#

we dont have to use static for everything

novel meteor
#

lol, funny thing is I used to do stuff with forge :D
I guess I could reduce the number of static vars in some places

round fossil
#

lmao

#

yeah forge doesnt have a lot of good practices

#

used to*

novel meteor
round fossil
#

what is this coroutine stuff

novel meteor
#

stuff I forgor to remove after trying to do stuff

round fossil
novel meteor
#

and found out it was a mistake

round fossil
#

lmao fake coroutines

#

I just read the implementation

#

u'd be baiting my ass off with this

#

but dw, java has some stuff in java 22 or sth for it

#

(virtual threads and structured task scopes)

#

yeah steve

#

static here is the main concern

#

why?

#

because it makes the code tightly coupled, thus hard to work around

#

that is to extend, maintain and build upon

#

it is also hard to test code that is tightly coupled because u cant isolate units (in ie unit tests)

novel meteor
round fossil
#

yeah dw, I think content wise, the library has good potential

#

if u just work a bit on the design :)

novel meteor
#

tyty

alpine anvil
novel meteor
round fossil
#

that post there is probably good enough, altho it misses some small stuff

#

public static final Map<String, CustomBlock> BLOCKS = new HashMap<>();

#

this map does not need to be static

#

public static Chunk currentLoadingChunk = null;

#

this variable also

novel meteor
novel meteor
round fossil
#

(which I think would lead to the removal of the static methods also)

novel meteor
#

I have the custom items, some of them (say the noteblock tuner) works with custom blocks, for example:
The setBlockState is static, what would I then have to do if I were to make the whole Blocks class non static ?
Would it be ok to access the Blocks object statically ?

round fossil
#

well

#

tbh

#

I would look into the singleton pattern steve

novel meteor
#

ye I had the same thought just as I asked

#

ty for confirming then :D

round fossil
#

:]

jagged rock
#

Single letter variables

#

and you repeat yourself quite a bit with e.getBlock()

#

There's also way too much going on in that class

novel meteor
#

I got too tired of writing "event" every time
Ye I see that now
Hmmm, I'll try to do something about that when I change the class

sly jackal
#

this has gotta be my favorite function name ever java public void iHatePistons2ElectricBoogaloo(BlockPistonRetractEvent e) {

novel meteor
#

tyty XD

night knoll
# 
public class TridentSpearModule extends JavaPlugin implements Listener {

    @Override
    public void onEnable() {
        // Register events and recipe
        getServer().getPluginManager().registerEvents(this, this);
        registerCustomRecipe();
    }

    @EventHandler
    public void onProjectileLaunch(ProjectileLaunchEvent event) {
        Projectile projectile = event.getEntity();
        if (!(projectile.getShooter() instanceof Player)) {
            return;
        }

        Player player = (Player) projectile.getShooter();
        ItemStack itemInHand = player.getInventory().getItemInMainHand();

        // Holding spear item?
        if (isSpear(itemInHand)) {
            // Cancel trident throw
            event.setCancelled(true);

            double range = 3.0;
            LivingEntity target = getTargetLivingEntity(player, range); // Return type
            if (target != null) {
                target.damage(5); // balance this later
            }
        }
    }

    // Function to check spear data
    private boolean isSpear(ItemStack item) {
        ItemMeta itemMeta = item.getItemMeta();
        if (item.getType() == Material.TRIDENT && itemMeta != null) {
            Integer customModelData = itemMeta.getPersistentDataContainer().get(new NamespacedKey(this, "CustomModelData"), PersistentDataType.INTEGER);
            // modify this when I know the numbers
            return customModelData != null && 10 > customModelData && customModelData > 1;

        }
        return false;
    }

    // Function to target an entity
    private LivingEntity getTargetLivingEntity(Player player, double range) {
        Entity targetEntity = player.getTargetEntity((int) range);
        if (targetEntity instanceof LivingEntity) {
            return (LivingEntity) targetEntity;
        }
        return null;
    }
#

ai wrote 80% of this

novel meteor
#

where is registerCustomRecipe() lol

night knoll
#

It's under this chunk of code

#

didn't think it was useful prolly gonna delete it

violet pier
#

Shiit yall still reviewing code? 💀

image.png
#

jkjk

violet pier
night knoll
#

let me google what a namespaces key is and fix that

novel meteor
#

the isSpear is dumb
"CustomModelData" integer ?!
I would just put a boolean instead (or a byte if not 1.20)

stark scarab
stark scarab
night knoll
#

Does isSpear slow it down, I might just keep it for organization

frosty grove
#

maybe pin this thread

stark scarab
novel meteor
frosty grove
#

just use nms smh

frosty grove
night knoll
novel meteor
stark scarab
violet pier
#

is it just boiler plate or smthn?

stark scarab
#

no, but it has like a million features lol

violet pier
#

with that much code im sure it does xD

novel meteor
#

It is "100% customizable" after all

violet pier
#

that must be hard to manage ide imagine or nah?

stark scarab
#

all the NMS stuff is in jefflib (the screenshot with 20k lines) so updating it is usually just updating jefflib

inland inlet
woeful pasture
#

Citric

inland inlet
#

Wats cirtic?

woeful pasture
#

Citric acid

inland inlet
#

You want to melt my image?

woeful pasture
#

yes

inland inlet
#

or do you want to melt my code?

woeful pasture
#

My main critisim would be, isn't the whole point of a library to abstract things? Why are you returning raw GSON objects

#

that should be abstracted into another interface

inland inlet
#

Idk if it would be called a lib, i just want to be able to make a class inherit it and be handled with ease

woeful pasture
#

don't use simple json either

#

use GSON

#

its built into spigot and more powerful

inland inlet
#

I use Gson

#

I use Simple Json for creaing th objects

woeful pasture
#

org.json.simple.JSONArray

#

one second

#

Gson is better in every way than Json Simple. You should remove it as a dependency. GSON provides better alternatives to everything Json Simple provides for example.

Json Simple would provide JSONArray
GSON provides JsonArray

Use the one Gson provides

Especially when GSON is native to spigot. shading Json Simple is a waste of space. If you are new to gson check out this.
https://www.tutorialspoint.com/gson/gson_quick_guide.htm

Gson - Quick Guide

Gson Quick Guide - Google Gson is a simple Java-based library to serialize Java objects to JSON and vice versa. It is an open-source library developed by Google.

inland inlet
#

What is the difference between them, is it going to be hassle migrating?

woeful pasture
#

it shouldn't be to bad. Gson is just more powerful Json Simple in every way

inland inlet
woeful pasture
#

well ofc there will be errors, but porting is worth it

inland inlet
#

Ok, thanks for the critic and help 😄

woeful pasture
#

also make sure you abstract JsonArray and JsonObject

#

don't return them raw it feels like it defeats the purpose of having anything there in the first place

inland inlet
#

The fucntion getArray?

woeful pasture
#

well do what bukkit does

#

instead of JsonObject return a ConfigurationSection or something similar

inland inlet
#

Well, this kind of made an issue for me

#

Does the Gson JsonObject

#

not allow true/false?

#

Do i need to make a new JsonElement Everytime

woeful pasture
#

I can't see screenshots use paste

inland inlet
#

?paste

rich fogBOT
woeful pasture
inland inlet
woeful pasture
#

why are you casting to JsonObject?

#

show me the getJson() method

inland inlet
#

Oooh

#

i didnt know about that

#

ty again 😄

jagged rock
jagged rock
stark scarab
# jagged rock

that's still less 😛 combined total is 33776 (and that's only angelchest core + jefflib, still missing stepsister, magicmaterial, and other libs)

night knoll
#

Second plugin I've made, does anyone have any critique or standards or things to keep in mind

inland inlet
#

Move your events to there own class

stark scarab
#

why are you using the initialize() method instead of either

  1. initializing it directly, or
  2. using the static { } init block?
#

why not just like this

public class MyClass {
  private static final int[][] CLASS_ARMOR_VALUES_RANGE = new int[][] {
            {1, 12},  // CLASS A
            {13, 24}, // CLASS B
            {25, 36}  // CLASS C
        };
night knoll
night knoll
inland inlet
#

It will clean up your class, and make it a bit more readable

woeful pasture
inland inlet
#

If you want to go one step further, id say you can also make a Utils class

#

that contains your utilities

#

like classifyEquipmentClass()

night knoll
#

So I have a seperate folder for events and I import that into the code that has all the management functionality

inland inlet
#

let me show you a strucutre

#

Here is my structuring

night knoll
inland inlet
#

Yes

night knoll
#

I am a bit confused on how an event triggers something in another class rather than triggering in its own class

inland inlet
#

?

#

How you can have an event in a seperate class?

night knoll
inland inlet
#

Ok, so you can either have them register them selfes

#

or you can register them manually

#

?paste

rich fogBOT
inland inlet
#

look in the main class under

registerEvents()
#

look in the main class under

handleEvents()
night knoll
#

So the event class is still where the main logic happens it just uses all the functions from the utility class

inland inlet
#

Yes

night knoll
#

And more functions are better than less it looks like

inland inlet
#

it allows for more complex projects that have 5k+ lines of code

inland inlet
night knoll
# inland inlet ?

If I have a loop that's 5 lines of code or so it'd probably be good to have it in a function right

round fossil
#

And that’s extremely powerful as the project scales

inland inlet
#

If you are only going to be using it in the same place

#

id leave as is

round fossil
#

For any large function, in order for you to understand what that large function does you need to read through the entire function @night knoll

#

By have more smaller functions with good names exiting early and still understanding what the function does becomes significantly easier

night knoll
inland inlet
#

For functions like the following ones, it is good to create a function

 BedwarsShopItem(Material material, int amount, String name, List<String> lore, ChatColor materialColor, String materialName, Material materialCost, int cost) {

        this.materialCost = materialCost;
        this.cost = cost;
        this.materialName = materialName;
        this.materialColor = materialColor;

        availableItemStack = new ItemStack(material, amount);
        notEnoughItemStack = availableItemStack.clone();

        List<String> availableLore = new ArrayList<>();
        availableLore.add(GRAY + "Cost: " + materialColor + cost + " " + materialName);
        availableLore.add("");
        if(lore.size() > 0) {
            availableLore.addAll(lore);
            availableLore.add("");
        }
        availableLore.add(AQUA + "Sneak Click to add to Quick Buy");
        availableLore.add(YELLOW + "Click to purchase!");

        List<String> notEnoughLore = new ArrayList<>();
        notEnoughLore.add(GRAY + "Cost: " + materialColor + cost + " " + materialName);
        notEnoughLore.add("");
        if(lore.size() > 0) {
            notEnoughLore.addAll(lore);
            notEnoughLore.add("");
        }
        notEnoughLore.add(AQUA + "Sneak Click to add to Quick Buy");
        notEnoughLore.add(ChatColor.RED + "You don't have enough " + materialName + "!");

        ItemMeta im = availableItemStack.getItemMeta();
        im.setDisplayName(ChatColor.GREEN + name);
        im.setLore(availableLore);
        availableItemStack.setItemMeta(im);

        ItemMeta im1 = notEnoughItemStack.getItemMeta();
        im1.setDisplayName(ChatColor.RED + name);
        im1.setLore(notEnoughLore);
        notEnoughItemStack.setItemMeta(im1);
    }
night knoll
round fossil
night knoll
#

Better than bad

inland inlet
#

xD

round fossil
inland inlet
#

Anyways, good luck on your further coding endeavors, and if you need any help, feel free to ask in #help-development , or you can dm me 😄

round fossil
#

Watch this 40:13 to 1:05:36

#

20 min

#

And it will make you a better programmer instantly

night knoll
night knoll
stark scarab
round fossil
#

Its 20 min about naming and size conventions for functions

inland inlet
#

@stark scarab
Line 268 - 276, you could use a switch if im not mistaken

round fossil
stark scarab
stark scarab
#

never heard of that

round fossil
#

Yeah annot library

#

Enterprise annot lib

stark scarab
#

thx I'll check it out

round fossil
#

Yeah, idr if spigot uses it, but paper does, or well adventure etc

#

Oh yeah

cobalt trellis
#

Adventure uses jetbrains

round fossil
#

I think the convention to prevent instantiation of utility classes is to throw AssertionError

#

not necessary but yea

cobalt trellis
#

From my experience cherqual is that library that is packaged everywhere but is never used

#

/s/cherqual/checkerqual

alpine anvil
#

no starter slash iirc

round fossil
#

Idr

#

But I use checkerqual always

#

So many handy annots like polynull, defaultqualifier etc

cobalt trellis
round fossil
#

Natively supported by iirc both eclipse and intellih

cobalt trellis
#

Well Eclipse only supports it via APs or a plugin if anything

round fossil
#

Yea

cobalt trellis
#

But as-is eclipse does not support it

round fossil
#

You know that better than me so yeah

cobalt trellis
#

It's a bit annoying that eclipse by default only supports it's own annotations but whatever

round fossil
#

🥲

night knoll
#

It's a good idea to use a BitSet to store a flag enum for an object?
Let me explain
I have this abstract class

public abstract class BossBarProvider<Player> {}

Which as the name suggests, is a "bossbar provider" for minecraft with backward compatibility and a que of bossbars (multiple bossbar support).
As many of you might know, BossBars have flags, and instead of using an array or any class that uses it (ArrayList) or anithing that extends to a collection, I'm using a BitSet and using the enumeration ordinal method to check whetever if the flag is "enabled" or not

public abstract class BossBarProvider<Player> {

    //Protected so it can be accessed by implementations
    protected final BitSet flagSet = new BitSet(BarFlag.values().length /*The bitset size is based on the flag enum size*/);

    /**
     * Set the boss bar flags
     *
     * @param flags the flags to add
     * @return the modified boss bar
     */
    public final BossBarProvider<Player> setFlags(final BarFlag... flags) {
        for (BarFlag flag : flags) {
            flagSet.set(flag.ordinal(), true);
        }

        return this;
    }

    /**
     * Remove the boss bar flags
     *
     * @param flags the flags to remove
     * @return the modified boss bar
     */
    public final BossBarProvider<Player> removeFlags(final BarFlag... flags) {
        for (BarFlag flag : flags) {
            flagSet.set(flag.ordinal(), false);
        }

        return this;
    }

    /**
     * Check if the boss bar has the specified
     * flag
     *
     * @param flag the flag
     * @return if the boss bar has the flag
     */
    public boolean hasFlag(final BarFlag flag) {
        return flagSet.get(flag.ordinal());
    }

    ...(abstract methods)
}

https://github.com/KarmaDeb/KarmaAPI2/blob/master/KarmaAPI-Minecraft/src/main/java/es/karmadev/api/minecraft/bossbar/BossBarProvider.java
Any opinion?

GitHub
KarmaAPI2/KarmaAPI-Minecraft/src/main/java/es/karmadev/api/minecraf...

KarmaAPI2 is the new version of the KarmaAPI. Contribute to KarmaDeb/KarmaAPI2 development by creating an account on GitHub.

jagged rock
stark scarab
#

yeah you're just reinventing the enumset

night knoll
#

Was there an object desidered for storing enumerations?

#

How would I create an empty EnumSet?

#

Or I don't understand how EnumSet works

cobalt trellis
night knoll
#

Wow 😳

cobalt trellis
#

Yeah, fancy

stark scarab
night knoll
#

As BossBar is also availabe for BungeeCord

#

The class is not designed for only Bukkit, but also BungeeCord and maybe Velocity

#

I want to make it so you have like "BossBarCreator#createBossBar" or something like that, so it won't matter the platform, there will be a constant BossBar API

#

Plus I want to make it like it ques the BossBars, so you could send multiple bossbars, and the API will manage the rest. (Scheduling the bossbar and sending the next bossbar when the current one ends displaying)

round fossil
#

Just go with a full on adapter and bridge pattern layer

#

these type of type parameters are usually those you wanna avoid if possible (transparent bivariant designs are very hard to maintain)

night knoll
round fossil
#

Yes

#

GlobalPlayer facades different implementations

#

And when you deal with creations of GlobalPlayer (since GlobalPlayer is not a concrete class) you may probably want to go with a factory pattern

night knoll
#

Do you mean a builder?
GlobalPlayerBuilder#named#withUID#atPosition#build: GlobalPlayer

round fossil
#

Sure, I was just thinking you about the creation in general as it is gonna be different implementations of GlobalPlayer depending on platform

#

But a factory
interface GlobalPlayerFactory {
GlobalPlayer create(PlayerContext ctx);
}

#

or sth

#

And then PlayerContext might have a builder perhaps

sly jackal
#

or the factory just has a few "constructors" that return the global player from a input player object

#

unless thats what you mean

round fossil
#

Sure, the point of a factory is to decouple instantiation from creation

#

By doing that you can effectively hide the compile time invocation of new, thus allowing for polymorphic behavior, which can be used effectively when dealing with multiple platforms

alpine anvil
#

gotta keep this alive

rancid fern
mystic solstice
#

This really needs to be a channel

#

This is my first plugin. I haven't used Java for a long time and I never was that good at it to begin with, so I'm curious as to what you guys think