Error when you don't set the block data type

[TomLewis]

Ahoy! Took me a while to figure out why I was getting internal errors; When you use blocks broken, and you don't set a data parameter then youll get an internal error, It was either one of these, I cant remember, I think it was block 17. blocks broken: 56;5;0 blocks broken2: 17;700;0 If you leave the 0 of one of those you'll get an internal server error when you do an /ar check You should default to 0 if none is put in.

[Staartvin]

Hmm, strange; I don't seem to get that error. I used this config:

Test:
  prerequisites:
    world: world_nether
    money: 1000
    blocks broken: 56;100
  requirements:
    money: 10000
    blocks broken: 50
  results:
    rank change: TestGroup2
    command: broadcast &p has just completed path 'Default'!
    command2: give &p 1 100
    message: Congratulations, you completed the 'Default' path!
  options:
    display name: Test path

Worked fine for me. Maybe something else was incorrectly formatted?

[TomLewis]

Try 17;700 instead of 56;100, I cant remeber what one flagged it.

[Staartvin]

Nope, those both seem to work fine. Well, if you encounter it again, make sure to report it!

[TomLewis]

I just want to make sure you are testing with stats3 and not statz, as thats what im using, I have statz installed just to hook into stats3.

Sailor:
  prerequisites:
    in group: 'Sailor'
    time: 3d
  requirements:
    time: 3d
    blocks placed: 10000
    exp: 60
    blocks broken: 56;5;0
    blocks broken2: 17;700;0
    players killed: 20
  results:
    rank change: Sailor;Carpenter
    command: 'broadcast &p has just been promoted to Carpenter!; give &p 383:50 32; give &p 368 32; give &p 279 1; give &p 17 64; give &p 17:1 64; give &p 17:2 64; give &p 17:3 64; tell &p Gifted some goodies!'
    message: 'Congratulations, you are now a Carpenter. See www.piratemc.com/ranks'
  options:
    display name: Carpenter

This is what I have.

[Staartvin]

I testing it with Statz. Please be advised that Autorank 4 will drop support for Stats 3 (as it is very buggy).

[Staartvin]

You can convert Stats 3 data to Statz data if you'd like.

[TomLewis]

There was a few UI issues with Statz that never got fixed (Im PirateCraft on Spigot), and I wasnt sold on the database, you said you may chance it, and I cant be messing around with the insane amount of data I have, It took 2 DAYS to convert my Stats2 to Stats3, I have 3/4 years of data from 80,000 players, thats a lot of data, I only (This Wednesday) managed to get stats2 to convert to stats3, and even if I got all stats3 & autorank working by tonight, I still would loose 3 days of stats data from all the players, but I hit a brick wall when Autorank doesn't convert old data, its just ridiculous. Please don't drop Stats3 just yet, there is absolutely no reason to drop it, at all and it gives us server owners time to figure it all out, Statz isnt ready, it hasn't got a solid database schema, and its UI needs a lot of work (I posted in detail what it needs). Keep Stats3, Its not going to change any time soon, and then we can migrate to Statz when its more stable/mature and has a database schema this is 100% perfect. I DO NOT want to have to go through this utter mess of moving between database schemas again.

[Staartvin]

Hmm, I think I wrote a converter from Stats 3 to Statz. Is there any particular reason you do not want to use? You're free to reject of course. I agree somewhat that Statz database is not fully optimized yet, but Stats is not being updated any more. In my honest opinion, it seems buggy. I don't blame the author per se, it's also a mess because of the rewriting to Stats 3.

[Staartvin]

I'm curious, what would you like to see sorted before considering Statz a more mature plugin. I'm interested in making Statz as stable as possible of course.

[TomLewis]

I will write you up exactly what needs to be updated and changed to make Statz production ready, I need to focus on getting my actual server updated from Stats2 and an ancient version of autorank to Stats3 and the latest autorank first, I need to complete this step Im stuck, I cant even update my permissions system as im stuck on stats2. I need you to just let me use Stats3 for a bit, so I can get it converted from Stats2. I will write you a massive list, but my time is limited to very small pockets of time.

[ljacqu]

I'm curious, what would you like to see sorted before considering Statz a more mature plugin

From a technical point of view, it's apparent that the code base grew "organically" and it could use some restructuring.

[Staartvin]

@ljacqu While developing, I tried to keep it as organized as possible, but I'll see what I can do!

[TomLewis]

@ljacqu Maybe you could give some pointers to how it could be better organized? I will create a massive list of what needs to be improved/added into Statz on the Statz Issue system once I have fully upgraded to Stats3 and a newer version of Autorank, Im very far behind and struggling with issues of Autorank working with Stats3 properly/Converting my Autorank data, im hoping V4 will fix all these.

[ljacqu]

Here we go!

Project setup

• The bin/ folder and Eclipse-specific files (like .project) don't belong in the repository -> they could be added to .gitignore
• The dependencies in lib/ could be imported from Maven repositories:

      <!-- Essentials Repo -->
      <repository>
          <id>ess-repo</id>
          <url>http://repo.ess3.net/content/groups/essentials</url>
      </repository>
      <!-- Vault Repo -->
      <repository>
          <id>vault-repo</id>
          <url>http://nexus.hc.to/content/repositories/pub_releases</url>
      </repository>
      <dependency>
          <groupId>net.ess3</groupId>
          <artifactId>Essentials</artifactId>
          <version>2.13-SNAPSHOT</version>
      </dependency>
      <dependency>
          <groupId>net.milkbowl.vault</groupId>
          <artifactId>VaultAPI</artifactId>
          <version>1.6</version>
      </dependency>

Nothing wrong with keeping them as backup if the repository is down but typically you want to avoid using local folders whenever possible.

General findings

• Declare fields & variables with the interface type whenever possible, i.e. List<String> list = new ArrayList<>(); and Map<Integer, String> map = new HashMap<>(); — and not ArrayList<String> list = new ArrayList<>(); or HashMap<Integer, String> map = new HashMap<>();.
• Avoid large methods, this is by far the biggest technical problem in the codebase. For example CheckCommand#check, LeaderboardHandler#updateLeaderboard, PathBuilder#initialisePaths. Your package and class structure is really nice, however.
• Currently, you're forced to pass the Autorank class everywhere: the class is "too smart". Consider looking into dependency injection to get the specific classes you need. This allows for loose coupling and shows more clearly which classes a class depends on.
• I'm really missing unit tests in this project and I'd gladly contribute some. Specifically, some of the more complicated util methods like parsing time strings would really benefit from tests, as well as tests with various configuration files to make sure everything is being handled correctly.
• Avoid repeating code: the handling of a time string is duplicated in GlobalAddCommand, GlobalSetCommand, RemoveCommand, AddCommand and SetCommand (found by searching the codebase for contains("m") :wink:).
  • Also within MySQLManager there's a lot of duplicate code for dealing with those Futures. In Java 8 you could create some nifty methods with some lambdas, but even with Java 7 I would urge you to consider extracting the common handling into separate methods.
• Names of variables and methods could be improved to have more meaning (e.g. in AddCommand: int value, StringBuilder builder doesn't really tell me much about their purpose)
• The JavaDoc you write is useful and coherent. Kudos on that. Only improvement I see there is to add JavaDoc to more non-obvious methods.
  • However, there are a lot of "non-Javadoc" comment blocks on top of overridden methods—are those generated by default? I don't understand their point seeing as any reasonable IDE allows to see which method is being overridden or implemented.
  • Remove the standard "// TODO Auto-generated method stub" comments from Eclipse throughout the code.
• There's a lot of commented out code scattered all over the place. I'd suggest to delete all of those occurrences. You have a versioning system (i.e. Git) so no code will be lost anyway.
• Logging - generally good and you typically log exceptions when they occur. That's fantastic. Just avoid e.printStackTrace() statements and use a logger method instead.
• Consider introducing a constant TICKS_PER_SECOND and TICKS_PER_MINUTE. It's hard to understand code like 20 * 60 * PlaytimeManager.INTERVAL_MINUTES (taken from MySQLManager) off the bat.

Specific issues

• AddOnManager is unused and its purpose is unclear. I would suggest to remove it until you know what sort of add-ons you want to support and who you want to cater to.
• BackupDataManager's createNewFile() method name is misleading and vague.
• CommandsManager
  • You can improve the efficiency of looking up the command drastically by first checking the registeredCommands map if it contains the args[0]. This requires having a Map<String, AutorankCommand> instead of the Map<List<String>, AutorankCommand> but I think it would be a more natural structure and you would have O(1) lookup time. Since you want to keep the order of the commands you can keep a separate list with them. Those are only stored references so the additional list won't take much size.
• AutorankCommand and children
  • Most of the onTabComplete implementations are just a return null. Why not implement that on AutorankCommand directly? You can override it in the children whenever you really need to.
  • At a quick glance it appears that you always do return true to exit the onCommand method. It would be simpler to change its signature to void. Somewhere up in the call chain you need this return value, but you can have return true; there once.
  • The description, usage and permission fields are defined by their setters being called inside of the constructor. This is a little strange as you have public setters for these fields, which should be immutable by intention. Also, if I extend this command I really need to pay attention to realize that I need to define these fields. You have three possibilities to improve this (by order of preference):
    1. Add abstract String getDescription() etc. on AutorankCommand, which forces the implementors to define them. Remove the fields and the setters.
    2. Add the three fields as constructor parameters to AutorankCommand, which also forces the implementors to pass it to the super constructor.
    3. Minimally, the quickest way you can improve this a little is by changing the setters to have protected visibility.
  • I was going to suggest using List<String> args instead of String[] args in onCommand but I can't really find a place where it's a big inconvenience for you.
• Permissions
  • Consider introducing an enum for the permissions. This will yield clearer method signatures and will avoid any typos. It's also immediately clear in the code which permissions exist, and their usages can be traced better.
  • You have a trick to check if someone has a wildcard permission by doing something like player.hasPermission("autorank.askdjaslkdj"). I've seen it at least twice and given the initial surprise when seeing this, I would suggest extracting this into a util method somewhere.
• Configuration classes (me.armar.plugins.autorank.config package)
  • InternalPropertiesConfig#hasTransferredUUIDs(boolean) does not make use of the boolean parameter
  • SimpleYamlConfiguration#loadFile: You're catching exceptions just to rethrow them; this is redundant
  • I see a lot of identical methods among PathsConfig, PlayerDataConfig and SettingsConfig. An abstract parent might be helpful here to implement saveConfig(), reloadConfig() etc. only once.
  • Similar to the above point, in Autorank#onEnable you have to call an additional method right after instantiating the classes to load the files... Could this not be done in the constructor immediately? If you don't like that (there are valid reasons to want to avoid that), you could introduce a static method on those classes that will call the constructor and then the initializer method (createNewFile() resp. loadFile()). This gives you the most freedom while still making it simple to use the class for its regular purpose.
  • getConfig() in PathsConfig, SettingsConfig and PlayerDataConfig (and possibly elsewhere) has a redundant cast and literally behaves the same as if you just used the config field directly (i.e. return this.getConfig().getBoolean("show warnings", true); is 100% equivalent to return config.getBoolean("show warnings", true);). More importantly, getConfig() should be private since it belongs to your class and isn't something you want to expose.
  • PathsConfig#getRequirementValue can be simplified to calling getConfig().getString(pathName + ".prerequisites." + prereqName + ".value"). FileConfiguration#getString is null-safe and calls Object#toString for you.
  • Same applies to getResultOfRequirement() and getResultOfPath().
  • Minor: SettingsConfig#getMySQLCredentials and likely elsewhere: if you switch over an enum, it's good practice to throw an exception in the default: clause so something fails loudly if an enum entry isn't handled. This way you're informed immediately in case you add an a new enum entry and forget to update the code in the switch.
  • It's apparent from the List<String[]> return value of PathsConfig#getPrerequisiteOptions that there's a better way to handle it. I need to be more familiar with this plugin to make a suggestion, though.
• The data package
  • FlatFileManager#loadDataFiles should not be public. But actually the entire method should be removed because it initializes static fields which should be immutable! Consider using Guava's ImmutableMap.
  • Alternatively, you can add the file value directly on the TimeType enum. You can introduce a method like getFile() and you'll have nice, readable code. But TimeType has quite a general usage so I wouldn't necessarily recommend putting something this specific on the enum directly.
  • FlatFileManager.TimeType is used in so many places that it should be in its own class. It doesn't strictly belong to FlatFileManager.
  • In GrabAllTimesTask:
  • :warning: avoid using SELECT * if you're only interested in a few columns. Especially because you access what you want with rs.getString(1). This is code that will silently break someday.
  • there's a lot of cases in which the empty map times will be returned. This is a little cryptic to me. I see similar code where it's being called, MySQLManager#getAllPlayersFromDatabase. Might be that code was moved around and there are some unintended leftovers? If not, these sections would benefit of explanatory comments and maybe if you returned Collections.emptyMap() it's visible more clearly that an empty map is being returned.
  • GrabDatabaseTimeTask
  • I would suggest to rename GrabDatabaseTimeTask to GrabPlayerTimeTask. This has a better contrast with GrabAllTimesTask, which also gets times from the database.
  • :warning: Same as in GrabAllTimesTask, avoid SELECT * and then fetching some specific column. You can simply run SELECT time or whatever the column name is.
  • In the future if anyone has a huge database and complains that it's slow, you should look into creating indexes on that data table, especially on the uuid column.
  • Be sure to always close Statements and ResultSets in SQLDataStorage.
• Debugger: really cool idea! The DateFormat fields shouldn't be static as the class isn't thread safe.
• DependencyManager
  • The Map handlers is almost unnecessary. You go through a lot of hoops to always use getDependencyHandler(Dependency.ESSENTIALS) etc. instead of just maintaining four separate fields in the class.
  • DependencyHandler#get() is never used and can be removed from the interface. I don't think you want to allow other code to just get the plugin instance. That's what DependencyHandler and DependencyManager do—they wrap the specific plugins and offer a stable API for the other classes to use so they don't need to deal with third-party plugins.
• Language handling
  • A static field doing I/O on an enum (FileConfiguration LANG in Lang) isn't nice. It's working out OK for you right now but it will bite you in the ass whenever you'll want to add more specific behavior, like user specific languages. You already have a LanguageHandler which is looking quite good; you'd just have to transfer more responsibilities to that class. :)
  • I wonder how efficient Lang#getConfigValue is and whether there's a good improvement for it.
• LeaderboardHandler
  • boolean order in sortByComparator should be called ascending.
  • String layout, double validTime and int leaderboardLength should all be static final.
  • Avoid using the slower String#replaceAll if you're not using regular expressions (you're not); use String#replace instead.
• pathbuilder package & subpackages
  • Requirement
  • Specify in the Requirement javadoc that the implementing classes must have a no-args constructor (as required by the RequirementBuilder class).
  • getDependencyManager() and similar are really bad interface design. At the minimum they should be protected and the RequirementBuilder should be responsible for invoking Requirement#setAutorank. I expect a fully usable Requirement being returned from RequirementBuilder.
  • setOptions has no business throwing a NoClassDefFoundError.
  • RequirementsHolder
  • :warning: Method getDifferenceIndex
    • Avoid catching IndexOutOfBoundsException - prevent it from ever being thrown in the method
    • The return value is used in String#substring but it defaults to -1. string.substring(-1) throws an exception...
  • Method meetsRequirement
    • Use more appropriate variable name for uuid - it's the UUID of what?
    • Some variable scopes are too broad; boolean result is declared way higher than when it is used for the first time; idem for int reqId.
  • Requirement implementations
  • Many classes have package-private variables; they should be private.
  • Many classes have a check like if (achievementCount != -1) where it should maybe be if (count < 0)?
  • LocationRequirement: Calculations are done in meetsRequirement which are discarded if the worlds don't match. The world check could be done before the calculations.
  • An additional abstract class extending Requirement may make sense; a lot of classes retrieve some value and compare it to a predefined value. Apart from "getting some value" the rest of the logic appears to be the same among many classes.
  • Result could use some Javadoc. Can't really say much more about it right now.
  • PathManager
  • PathManager#matchPathbyDisplayName's isCaseSensitive parameter is always false; you can drop it and remove the case-sensitive implementation.
  • Similarly, PathManager#matchPathbyInternalName has isCaseSensitive = true only in one usage of four—is this intended?
  • setBuilder is public but only used within the constructor. Replace with this.builder = new PathBuilder(plugin)); and remove method.
• Permissions
  • 3 out of 4 implementations have to convert a collection to an array for String[] getGroups() instead of having the method return a Collection<String>. Change method signature to Collection<String> (or List<String> / Set<String> if you fancy that more).
  • GroupManagerHandler
  • Methods getGroup(), getPrefix(), getSuffix() are unused.
  • getGroups(): Iterate over Collection<Group> worldGroup directly instead of creating an intermediate List<Group> list. Also you put all of the group's names into List<String> groups to then convert it manually to a String[] array. Simplify to filling an array (or collection, see point above) immediately.
  • At a quick glance, the same applies to all the other methods returning String[].
  • PermissionsPluginManager: Rename methods findGroupManager etc. to something like isGroupManagerAvailable to give meaning to the boolean return type.
  • Additionally, since only used within PermissionsPluginManager, they should be private.
• PlayerChecker#getMetRequirementsHolders is spaghetti code; it's a mix of long conditional clauses and continue statements and needs restructuring.
• Handlers
  • Rename DummyHandler to NoOpHandler or EmptyHandler or something. Dummy makes it sound like a temporary or test implementation.
  • Rename the statTypes enum in StatsPlugin to StatType to conform to Java conventions.
  • StatsPlugin#getNormalStat should have a Map<String, Object> param—not HashMap<String, Object>.
  • StatsPluginManager's find* methods have the same issue as the methods in PermissionsPluginManager (see point there)
  • Once I understand the plugin a little better I may have a better suggestion for the HashMap<String, Object> arguments parameter in getNormalStat(). I see a lot of parameters.get("...").toString() in there, and stuff like (Integer) arguments.get("moveType") also suggests to me there's a better way of doing this. I can't make a suggestion yet, though.
• I skipped the updater and util.uuid packages
• AutorankTools
  • getCorrectReqName
  • The static List<String> reqTypes field should be a Set. This way in registerRequirement(String) you can simply call regTypes.add() without checking with contains() first.
  • :warning: The loop in getCorrectReqName() is nothing else than if (regTypes.contains(oldName)) return oldName... Please simplify.
  • AutorankTools should be a stateless utility class and should not be aware of any "application data." The getCorrectReqName method should be moved to some class that is more specific where it makes sense for it to know about the existing requirements.
  • getCorrectResName - all of the points for getCorrectReqName apply for this too.
  • largestK
  • Change signature from Integer[] to List<Integer>. You only call the method in one place—a place where you have to convert the List to an array to fulfill the method signature.
  • catch (ArrayIndexOutOfBoundsException e) is a bad idea. A simple check at the beginning could prevent this. An exception like this should never be caught.
  • sendColoredMessage(Player, String) is redundant as Player is a CommandSender, and a method sendColoredMessage(CommandSender, String) exists doing the same thing.
  • makeStatsInfo(Object...)
  • needs a more specific name
  • if the number of arguments is odd, you'll get an array out of bounds exception
  • I don't think it's intuitive that it will filter out negative numbers if they just so happen to be valid numbers. I think the entire method needs to be revised. After looking at the usages more carefully I can provide a suggestion.
  • makeProgressString, getStringFromSplitString are unused -> delete?
  • getFoodName: the simple cases can be concatenated and then item.name() can be returned.
  • getFoodItemFromName has comment "Cannot use switch, is only supported in Java 1.7". This is outdated. Use switch. ;)
  • findClosestSuggestion could benefit from returning an inner class consisting of a distance and suggestion field instead of data concatenated into one String.
  • createStringFromList: not necessary to create an array, just increment an i counter manually.
  • isExcluded(Player): I have no idea what this check does without checking the implementation. Rename to isExcludedFromRanking?
  • stringtoDouble, stringtoInt: to should be To; both methods don't throw a NumberFormatException. Remove from signature.
• ValidateHandler
  • I feel like List<String> permGroups should be called something else and String[] vaultGroups should be permGroups?
  • String[] vaultGroups is only used wayyy at the bottom of the method. Declare it there.
• WarningManager
  • The javadoc on the class says something about the messages being stored as "warning message>10" (i.e. a split with >). This no longer seems to be the case.
  • registerWarning(String message, int priority) is only ever called with priority = 10. This leaves you with two choices:
    1. Drop the priority parameter.
    2. Simplify to a Priority enum instead, having three values. You have to currently maintain code to ensure a priority between 1 and 10 but don't have a use for it.
• Autorank main class
  • initializeReqsAndRes doesn't belong in the main class. This needs some sort of RequirementFactory class or similar.
  • The main class should probably not be concerned about logging (looking at debugMessage(String)). The small method isn't a bother in the code there but it makes all classes depend on your main plugin file to debug messages, which tightly couples everything.
  • It's a really cool idea to offer public setters and getters for all of your main 'services' (looking at setLeaderboardManager, setPathsConfig etc. Those types should probably all be interfaces, though, since sometimes it's a little hairy to have to extend existing classes that are quite specific (I'd need to call the super(Autorank) constructor, and I have no way of avoiding code to be executed that is done within the constructor).
  • Discourage the use of Autorank#getAutorank; plugin developers should just do what that method does directly.

[Staartvin]

Wow. I was blown away by your post! Many thanks for all the suggestions, you are so right about most of it. As you've said a few times in your post, making suggestions is hard when you're not 'too familiar' with the code, but you did a really good job; I certainly have some work to do.

You also mentioned something about unit tests. I know what they are, but have never written any nor tested with it. Maybe you could provide a few (as well as the infrastructure needed for it to be used)?

I claim to be no expert programmer, but it's good to get some proper feedback about my codebase, it doesn't happen that often ;-) You mentioned some new concepts that I'm currently not familiar with (for example Dependency Injection). Let's hope I can grasp these concepts properly and am able to implement them. If you feel like providing suggestions or contribute to the codebase, you are more than welcome to!

Thanks a bunch and I'll get coding!

PS. I've saved this post for offline reading (if you don't mind), so I can come back to it.

[TomLewis]

@ljacqu Holy crap that post! the detail is amazing! Seriously thank you for writing that in detail, it means a lot to me to get a solid statistics recording plugin that is awesome, I rely on it! having a backbone breakdown like you have just posted is utterly amazing.

[Staartvin]

@FrozenBeard I'm still eagerly awaiting your suggestions for new (and probably) better features :)

[TomLewis]

@Staartvin They will come! I need to still get myself off stats2, to stats3 and the new Autorank (Gotta find the time to test with V4!) Then once this is stable and in production, I will test a convert to Statz and run it through its paces! I already have a rough list of ideas, but I want to give you a solid list!

[ljacqu]

Thanks for the positive feedback, @Staartvin @FrozenBeard. I didn't know what reaction this would get since it's a touchy situation to pop out of the blue with a wall of text. :smile:

making suggestions is hard when you're not 'too familiar' with the code

Yeah, I don't think it's OK to be like "this is not good" and not to offer suggestions for improvement. You can ask me for clarifications or suggestions for any of the points I brought up. Might just take a while for some of the bigger stuff for me to get familiar more :3

unit tests. [...] Maybe you could provide a few (as well as the infrastructure needed for it to be used)?

Absolutely! Testing is my thing so you'll have a PR coming your way. I help maintain AuthMe and the project has 832 unit tests right now... :P

Dependency Injection). [...] If you feel like providing suggestions or contribute to the codebase, you are more than welcome to!

Thanks! Dependency injection is another thing that was introduced into AuthMe and the solution for it eventually became a separate project that is now used by AuthMe, PerWorldInventory & voxelwind. Guice is another option. I can give a detailed comparison, and just for the record I'm not trying to force "mine" onto this project (no matter how this looks lol). (In short: Guice is a standard framework people will be familiar with and has more features; the linked injector is smaller in file size, defaults to wiring up singletons and you can @ me anytime for support and feature requests.)

I've saved this post for offline reading (if you don't mind)

I'm just really happy that my post is noticed—please, do whatever you want with it.