Please review my code and help me improve it!

Discussion in 'Plugin Development' started by Tster, Jan 6, 2014.

Thread Status:
Not open for further replies.
  1. Offline

    Tster

    It's all in one class, has comments, error catching and I'm pretty happy with it.

    Main class: https://github.com/Tster/OreGen2/blob/master/OreGen2/src/io/github/tster/oregen2/OreGen2.java
    Config.yml: https://github.com/Tster/OreGen2/blob/master/OreGen2/config.yml

    I think I have the mind for programming, but not the experience - I need to work on doing things more efficiently. I don't know what good practices there are etc.

    If you could review my code and help me improve as a coder (please explain amendments so I understand), I'd really appreciate it.

    Thanks,
    Tster
     
  2. Offline

    RawCode

    Dont parse config every time on every block change.
    Do it single time and store results, on server reload update cache.
     
  3. Offline

    Tster

    Am I not doing that? At the start I use config.getValues() to store the config in a hashmap called configMap.
     
  4. Offline

    Syd

    1.
    Instead of saving the whole config in your own map, use the Configuration interface provided by Bukkit.
    It's much easier to work with methods like getDouble(path, default) and is does cache the values as well. (So you wont have file access every method call)

    2.
    Indentation/code style is important. It makes your code way better readable.
    Most IDEs should have a implemented auto formatter. In Eclipse it's ctrl+shift+f.

    3.
    If you reset a global variable everytime you call a method and you use this variable only within this method, you should declare it local in the method.
    So you won't have to reset it everytime and you'll save memory, as these variables are only present, when they're called.

    4.
    Try to learn, understand and use object orientation.
    For your basic plugin it's totally fine to use just one class, but with bigger tasks you will need more features of java.

    Access modifiers are a part of that. In your current code it would mean that, additional to 3., you should declare every global variable as private as well as the chooseBlock method.
    Nothing not within your class will ever need this stuff, so you shouldn't show it to them.
     
  5. Offline

    Tster

    Syd

    Wow
    Thanks for that, I'll go through each of these I try to use your tips.

    In response to your first point, the goal of my config is to allow users to select ANY block. How can I get a value if I don't know the key?
    Should I read each line of the config and make an array with all the keys, and then use a 'for' statement for each key in the array, which then gets the value?
     
  6. Offline

    Syd

    Code:
    for(String key : getConfig().getKeys(false)) {
        double d = getConfig().getDouble(key);
        //do whatever you want with d
    }
    getKeys(false) gives of a Set of all top level keys (STONE, COAL_ORE and so on), so you can get their double value with getDouble(key) easily without knowing the exact keys while coding the plugin.
     
  7. Offline

    RawCode

    maxDecimalPlaces = value.toString().length() - value.toString().indexOf('.') - 1;
    doubleFromString = Double.parseDouble(value.toString());
    totalDraws = totalDraws + doubleFromString * Math.pow(10,maxDecimalPlaces);
    doubleFromString = Double.parseDouble(entry.getValue().toString());
    currentDraw = (int) (currentDraw + (Double.parseDouble(entry.getValue().toString())

    You parse stored data EVERY call without storing results.
     
  8. Offline

    Bart

    Other than some of the things people have mentioned here, it really isn't that bad. It could just do with a quick formatting (ctrl+shift+f on eclipse+windows) just to keep some kind of standard going within your codebase.
     
  9. Offline

    Tster


    Thanks

    Alright, I'm going to get going on some of these suggestions and upload a new version soon.

    Cheers, Tster

    New code at same link:
    Main class:https://github.com/Tster/OreGen2/blob/master/OreGen2/src/io/github/tster/oregen2/OreGen2.java

    Syd, Is this good? - at least I think I did what you suggested.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 6, 2016
  10. Offline

    Syd

    You don't use the configMap variable anymore, so you could remove it.
    And there is no need for try catch when calling getDouble, as it can't throw an Exception. (If something is wrong it will return 0)

    The only thing I don't understand is the stuff with maxDecimalPlaces.
    I guess you need whole numbers - so why don't you just allow only integers in your config?^^

    However, if the plugin does what you want, it's good.

    PS: You should tell your IDE to use less spaces. oO
     
  11. Offline

    Tster


    Ahaha! I was error testing on an older version using the config:
    thisinotamaterialname: cheesecakes

    or something similar, and I didn't change the config when testing my new code. I was getting an error from my random number generator trying to generate between 0.0 and 0 xD

    The maxDecimalPlaces is for exactly what you said. I wanted to allow users to input in any way. By limiting them to integers I was making it harder to do it as they pleased (in my opinion)
    For instance, if they wanted each chance to be a percentage - for their ease of understanding - they would limit themselves to a minimum 1% chance for diamonds.

    Basically I do a lot of work just for the sake of the user editing chances in any way they please.

    My IDE is eclipse btw.
     
  12. Offline

    bigteddy98

    This will display double messages:
    Code:java
    1. getLogger().info("OreGen2 by Tster has been enabled!");

    Because you send a message, but Bukkit itself also does. Not a big problem, but... Just saying ;).
     
  13. Offline

    Tster

    IIRC, to be precise, it says it is 'being enabled' and not necessarily has been enabled.
     
  14. Offline

    bigteddy98

    True that, my fault. Nice code anyway :)
     
  15. Offline

    Tster

    I have no idea whether to leave it with both or not, maybe it'll help in troubleshooting an error. Anyway, I see where you're going.
    Thanks :)
     
  16. Offline

    Syd

    Tster
    Okay, if you want to allow your user to use doubles it seems to be a way.
    Then, however, it would be more efficient to pre-calculate these values, as they'll never change while the plugin is running. (Thats what RawCode meant)
     
    Tster likes this.
Thread Status:
Not open for further replies.

Share This Page