[Source Code] Helpers (Not a staff plugin) (Requesting feedback on my code)

[Fudged]

Source: https://github.com/Yergle/helpers

I created the plugin in the past two days, and really do not feel like posting this everywhere and having to maintain it. As far as I'm aware, this is a working plugin. The only key issues (in my opinion) is the lack of configuration.

The number of helpers are based off of permissions. "helpers.<helperType>.<amount>
Helpers types: miners, farmers, butchers, lumberjacks, excavators

Features:
- Unlimited helpers
- Automatically give out rewards
- Configuration on time
- In-game GUI editor for rewards

Suggestions I'd make if anyone wishes to take over this project:
- Unlimited worker types
- Full configuration
- Save inventories (currently, when a player leaves and server reloads inventories are cleared)


I'd also appreciate some feedback on my coding style (if you actually know what you're doing please -.-), and ways I can improve to become a better developer

Edit: Please do destroy my code (like point out every small issue) because the best way to learn is through failure

 

[Ambrosia]

Some bad code methods, you abuse the statics and you name you main package "Main".

 

[Fudged]

Some bad code methods, you abuse the statics and you name you main package "Main".

Can you please point out some of these bad code methods? And also, what are some ways to get around the static abuse? Thank you for the quick response

 

[Caedus]

Only looked at the one class, but don't store Player objects (as you do in User). Not sure but I believe it can cause memory leaks. Either way, you should save the players UUID instead, that way you can still get a player object from them without storing it

 

[Fudged]

Only looked at the one class, but don't store Player objects (as you do in User). Not sure but I believe it can cause memory leaks. Either way, you should save the players UUID instead, that way you can still get a player object from them without storing it

Thank you for pointing that out and taking time to look at it

I always try to store UUIDs but I have such a bad habit of storing Players <.<

 

[Hiry]

Thank you for pointing that out and taking time to look at it

I always try to store UUIDs but I have such a bad habit of storing Players <.<

Its just lots less data to store, and what he said with the memory leaks.

Also to answer your bad "static habit", you make the variables public, get an instance of the class you want to access the variables in, and access it using the instance.

Spoiler: Example

Class 1:

public class Class1 {

    Class2 instance = new Class2();

    String var = instance.variable;

}

Class 2:

public class Class2 {

    String variable = "Hello!";

}

 

[Fudged]

Its just lots less data to store, and what he said with the memory leaks.

Also to answer your bad "static habit", you make the variables public, get an instance of the class you want to access the variables in, and access it using the instance.

Spoiler: Example

Class 1:

public class Class1 {

    Class2 instance = new Class2();

    String var = instance.variable;

}

Class 2:

public class Class2 {

    String variable = "Hello!";

}

Ah thank you so much for the help!

 

[Fudged]

Spoiler: sorry its bigxddd

thats what she said xDXDXDD

what the fuck is this doing here
just make the methods and fields private and access them through your main static method that gets the instance of the plugin
no need for everything to be static
==============================

==============================

instead of using the same method for similar objects over and over again, just make a custom item object, with all the fields you need, and in your method where you make your objects add them to a list and loop through them so instead "Main#getItemsEditor()#getButcherItems()#get(whatever item in the array)"
you can simplify it to something like:

for (CustomItem customItem : Main#getItemsEditor()#getCustomItems()) {
            if (u#getCustomItem(customItem) > 0) {
                if (Main#getCustomItemsManager()#getItemsFor(customItem) != null) {
                    int i = r#nextInt(Main#getCustomItemsManager()#getItemsFor(customItem)#size());
                    if (u#getInventory()#firstEmpty() != -1) {
                        ItemStack item = Main#getCustomItemsManager()#getItemsFor(customItem)#get(i)#clone();
                        item#setAmount(item#getAmount + u#getCustomItem(customItem)#amount());
                        u#getInventory()#setItem(u#getInventory()#firstEmpty(), item);
                    }
                }
            }
        }

simple example

for (Object object : Main#getObjects()) {
    if (u#getObject(object)#amount() > 0) {
        //do shit here
    }
}

==============================

==============================

not really a mistake but just preference,

for (ItemStack is : inv#getContents()) {
            if (is#getType() == Material#AIR) {
                is#setType(filler here);
            }
        }

==============================

instead of using all these if statements u can use one big switch statement to make it a lot cleaner

==============================

Wow thank you for this I'll definitely keep all these in my mind whenever I make plugins in the future

 

[SirLove]

Cool!

 

[Blowns]

Understand the concepts of OOP.

 

[dakota]

Code looks bad. I think you should redo or fix up your code and then ask for opinions gl tho

 

[Fudged]

Fallacy. Storing player objects do not cause memory leaks. Everything can cause a memory leak if you don't know how to manage it. I don't know who spread the rumor. The only reason you do NOT want to save a player object is in a case where you don't want needless amount of data(with the player object you also save the location, pitch, yaw, armor contents, item contents of the player, you get the idea)

Here are some things I'd fix.

- You spam static. Try dependency injection.
- You called your custom runnable class "runnable". This could interfere with bukkit's default runnable class later on as you use it(even though the new bukkitrunnable class is waaaaayyy more efficient).
- Empty constructor in runnable class. No reason to put it there.
- Odd variable naming conventions. You shouldn't name your variables "s" or "p", etc etc - the code WILL look like as if it were obfuscated and it will be problematic for developers who wish to add on to your project.
- You create get and set methods in your files class yet the fileconfiguration class already provides them. ???
- In your items class, the massive amount of if statements in the editorInv class would make another programmer cringe. Use a switch statement, saves time and effort.
- You call your helpers'plugin main class Main, yet your helpers' plugin command "Helpers" ~ ????
- The command name check isn't necessary if you're planning to do one-command-per-class.
- Again, switch statements in helpers class.

Also valaiyar - that static getPlugin method already exists in bukkit. Do this and you will see:

(MainClass).getPlugin((MainClass).class) ~ it works wonders, however for those who just like to do (MainClass).get(depends on what you put here, many people do getInstance but i personally do getPluginName).

All i can think of.

Very helpful. Thank you

Understand the concepts of OOP.

I feel like you're answer is going to be everything, but what concepts in particular should I go do more research on?

Code looks bad. I think you should redo or fix up your code and then ask for opinions gl tho

I figured people were gonna say stuff like the code looks bad so that's why I didn't release it, and only asked for feedback on the code itself.

I've made changes in Eclipse, however I want people to criticize my code, not the code being suggested because there's no point in critiquing the code of others (perhaps professionals)

 

[Turtle]

Source: https://github.com/Yergle/helpers

I created the plugin in the past two days, and really do not feel like posting this everywhere and having to maintain it. As far as I'm aware, this is a working plugin. The only key issues (in my opinion) is the lack of configuration.

The number of helpers are based off of permissions. "helpers.<helperType>.<amount>
Helpers types: miners, farmers, butchers, lumberjacks, excavators

Features:
- Unlimited helpers
- Automatically give out rewards
- Configuration on time
- In-game GUI editor for rewards

Suggestions I'd make if anyone wishes to take over this project:
- Unlimited worker types
- Full configuration
- Save inventories (currently, when a player leaves and server reloads inventories are cleared)


I'd also appreciate some feedback on my coding style (if you actually know what you're doing please -.-), and ways I can improve to become a better developer

Edit: Please do destroy my code (like point out every small issue) because the best way to learn is through failure

don't call your main class "Main". It's bad practice as bukkit already has a main class.