Rate my code.
- Thread starter Foodle
- Start date
- Status
PebbleHost
High performance, consistent uptime and fast support. Minecraft hosting that just works.
That is exactly what happens. For example, I worked with the EffectLib. When you spawn 1 particle you think "Oh, it's just 1 particle, it won't affect the server at all", but try spawning 10000 particles at once, every tick. Spoiler: it affects the server.
true, but I'd realize that before it would happen, wouldn't I?
and good point, LCastr0 , however if it's open source and MEANT to be used, I would probably and positively put a readme for people to understand what's going on, this is just to check code, and explain functionality, not conventions ~ a long term project probably will follow conventions, this was a quickaroo project.
Also, about the "they build up" ~ anyone with a solid knowledge of a programming would foresee that and not fall for it.
Banned forever. Reason: Ban Evading (NPoints)
^ that last part not exactly true at times you can think the implementation is a great idea! Then find out problems in about 5 minutes of testing.
Mistakes are the best way of learning, amirite?
. I will leave this thread open as I am open to suggestions, programming is a big world and I want to at least be able to make a career out of it.
Banned forever. Reason: Ban Evading (NPoints)
Thank you for your responses!
I think that it's because i use tabs instead of spaces, indeed a VERY bad habit, but I can't seem to get it out of my head.
Also, for the "standards" part - I am using the google style, google it(ha i did that on purpose)
As for the comments, that completely skipped over my head as I don't normally publish projects in github,
Thank you so much!
Now, to answer LCastr0, you are very right indeed. My spacing is weird, I think this is also part of github's fault, but that's just my guess.
Also, can you show me the "you leave too many blank lines" in code? I'd like to know where so I could fix it
Actually your not using the google style.
Google Style: https://google.github.io/styleguide/javaguide.html#s4.1-braces
Difference
Google: public void something() {
}
You: public void something(){
}
I only just changed style to google but I used to style my code like you do
No problem.
skimmed over other posts so not reposting what they said hopefully
in https://github.com/FoodleProgedy/St.../src/me/foodle/staffutility/StaffUtility.java
you shouldn't have enable() inside constructor.
constructors are for setting up the class not running other code.
instead constructor should just set up the clcass, then you run staffutility.enable();
btw you don't need comments really (in response to what someone said earlier). your code should be clear enough to not need comments.
you only need comments if it's actually confusing and needs commenting; your code shouldn't regularly require commenting.
that thing about it slowing it down a fraction of a second is stupid. the example with the particles is stupid too. obviously particles are going to cause lag, they're entire entities that are being created and that will take a toll on the server. if you run that method creating 1000 string objects as oppposed to particles nothing will happen because strings have nowhere near the strain particles have on a server.
running through a ifnull check is fine. instead of using your ItemChecks.ifNull use Objects' isNull. Your method is a rewritten version of it that isn't as good. Objects.isNull(Object obj) supports any object and will support checking if your itemstack is null fine.
i dont really know bukkit but those four execute() methods could probably be made nicer somehow.
also those methods in staffitemmanager eg interact/interactEntity should be private. unless bukkit's api doesn't support that.
that's just what i see through a quick read
keep it up
in https://github.com/FoodleProgedy/St.../src/me/foodle/staffutility/StaffUtility.java
you shouldn't have enable() inside constructor.
constructors are for setting up the class not running other code.
instead constructor should just set up the clcass, then you run staffutility.enable();
btw you don't need comments really (in response to what someone said earlier). your code should be clear enough to not need comments.
you only need comments if it's actually confusing and needs commenting; your code shouldn't regularly require commenting.
that thing about it slowing it down a fraction of a second is stupid. the example with the particles is stupid too. obviously particles are going to cause lag, they're entire entities that are being created and that will take a toll on the server. if you run that method creating 1000 string objects as oppposed to particles nothing will happen because strings have nowhere near the strain particles have on a server.
running through a ifnull check is fine. instead of using your ItemChecks.ifNull use Objects' isNull. Your method is a rewritten version of it that isn't as good. Objects.isNull(Object obj) supports any object and will support checking if your itemstack is null fine.
i dont really know bukkit but those four execute() methods could probably be made nicer somehow.
also those methods in staffitemmanager eg interact/interactEntity should be private. unless bukkit's api doesn't support that.
that's just what i see through a quick read
keep it up
Last edited:
for staffitem it's probably in your interest and good design for encapsulation to have your private fields immutable. with your getstaffitem you pass direct reference to your field and anyone can access and modify it. so you don't have control (meaning staffitem class only) over your private field. it is best to clone it, copy it, and return that instead so I can't come and write a plugin and mess every ItemStack up.
I used to do some Java programming (only for a course in school, not for personal projects), and I definitely don't know Bukkit, but I was looking through and noticed this:
StaffItemManager.java:62
Looks like you're checking for nullity, twice?
Could always incorporate the second check into the first check (isNull()), or you could just combine them into 1 statement:
If you want to simplify, you could always use ternary operator, and I believe Java has a shorthand for checking for null using the ternary operator:
a ?: b
is the same as
a != null ? a : b
And thus your code simplifies to
I think? Try it out and let me know.
StaffItemManager.java:62
Looks like you're checking for nullity, twice?
Could always incorporate the second check into the first check (isNull()), or you could just combine them into 1 statement:
Code:
if( !ItemChecks.isNull(player.getItemInHand()) && getStaffItemByItemStack(player.getItemInHand()) != null ) {
getStaffItemByItemStack(player.getItemInHand()).execute(e, player);
}If you want to simplify, you could always use ternary operator, and I believe Java has a shorthand for checking for null using the ternary operator:
a ?: b
is the same as
a != null ? a : b
And thus your code simplifies to
Code:
getStaffItemByItemStack(player?.getItemInHand())?.execute(e, player);I think? Try it out and let me know.
Ternary operator in Java is expression cannot replace if else function calling this instance.
Simply used to return a value.
Code:
String quick = isTrue() ? "yes" : "no";Still falls under my point. execute is function call. With ternary operator is only expression like variable. And must be assigned.And what is your response about this?
Last edited:
Yeah I removed it since I just realized he wasn't returning anything and can't use the null coalescing feature.
Jack ~ no, they're two completely different checks.
The "isNull" check is for the itemstack, and the != null is for checking if a itemstack that the player is holding is a staffitem or not. If it is not, it will return null, i am therefore stating it should not be null.
Also, badvibes ~ thank you so much. This gave me a clear idea of how much I have to learn.
But, for the "null" part, I have to disagree. Check java documentation, it just returns object == null, and I guess you shouldn't need an entire method for something that already exists, but it isn't any better than mine, it's practically the same, but I'll take your advice and use it instead of my own methods next time!.
As for the enable part, you should read the code in the enable method, it's done so that when users instantiate the StaffUtility class they don't have to also do staffUtility.enable() or something of the sort. An instantiation would be enough.
Also for the fields not being immutable, that's the point, a developer can tweak it to its liking.
As for the 4 execute methods I don't think there's any other way.
The "isNull" check is for the itemstack, and the != null is for checking if a itemstack that the player is holding is a staffitem or not. If it is not, it will return null, i am therefore stating it should not be null.
Also, badvibes ~ thank you so much. This gave me a clear idea of how much I have to learn.
But, for the "null" part, I have to disagree. Check java documentation, it just returns object == null, and I guess you shouldn't need an entire method for something that already exists, but it isn't any better than mine, it's practically the same, but I'll take your advice and use it instead of my own methods next time!
.As for the enable part, you should read the code in the enable method, it's done so that when users instantiate the StaffUtility class they don't have to also do staffUtility.enable() or something of the sort. An instantiation would be enough.
Also for the fields not being immutable, that's the point, a developer can tweak it to its liking.
As for the 4 execute methods I don't think there's any other way.
Last edited:
Banned forever. Reason: Ban Evading (NPoints)
In theory there'd be no point since constructor StaffItem takes an ItemStack. It can be reasonable to expect a developer would already construct his ItemStack and just pass along it. It's just for purpose of encapsulation one of the main principles and better design for the future (if you're thinking onto a career) to hide all private fields and not pass direct reference to them when needed (aforementioned anyone could modify its state and the class thus ha son control)
it's better just because it can take any object. using it will produce the same results, except now you can check if any object is null rather than just itemstack. additionally, you don't have to have that extra method in your plugin bc it's in the library.Jack ~ no, they're two completely different checks.
.
But, for the "null" part, I have to disagree. Check java documentation, it just returns object == null, and I guess you shouldn't need an entire method for something that already exists, but it isn't any better than mine, it's practically the same, but I'll take your advice and use it instead of my own methods next time!
As for the enable part, you should read the code in the enable method, it's done so that when users instantiate the StaffUtility class they don't have to also do staffUtility.enable() or something of the sort. An instantiation would be enough.
Also for the fields not being immutable, that's the point, a developer can tweak it to its liking.
As for the 4 execute methods I don't think there's any other way.
enable part is a good idea because otherwise creating the constructor performs other actions than intended by the user. for your plugin the developer probably would look through everything, but on larger scale projects i'd advise against doing it. just advice for future bigger projects.
there are other ways it's just been long since i've developed anything in java i can't think of anything
anyways
gl w your plugin and learning java
Sorry, but the fractions of seconds that it is slowing down might not be enough in this situation, but it is if you're doing something big.
Have you tried advent of code? Well, if you did, you will know in some cases the difference of 1 fraction of second between each iteration causes a big problem at the end.
The particle example was not ideal in this situation, but it was just an example.
If you want to keep having "just a fraction of a second" of performance issues, fine, go ahead, but I want to avoid that. Maybe if you someday work in a bank you'll know that seconds make a giant difference.
Last edited:
please stop. first, it's not "just a second". using that method will never add up to a second. don't talk to down to me because you have no idea what you're talking about. i've read heavily into java optimization & the jvm bc i used to write (de)obfsucators/optimizers. if you're going to continue down this path about how using an ifNull method is going to add up time (or an entire second as you think :rofl
please start linking articles & tests to back it up bc you're talking out of your ass.thanks
- Status
