Read comment

[gravypod]

Removed thread pool, added thread safe objects, moved backup into its own thread, and got a cookie from ElgarL!

[Exolius]

Read my above line comments.

Also I would like to know what IDE your making these changes with and if you test the plugin before submitting any changes with a pull request to the main repo?

[gravypod]

Its stated in the header.... the way you are supposed to do it...

[Exolius]

You didn't answer a single one of my questions, like if you test the plugin before you make a commit, or why your line endings make git think a while file has been changed.

[gravypod]

Yes, I tested this.

[mantun]

gravypod, since the line endings are changed, it is hard to see what are the real changes in the code. Can you make a summary of what are the exact issues this commit is solving? Or what are the features this commit is adding?

In the commit message you assert 'fixed your code', but I don't exactly see what is fixed. Reformatting and refactoring are not 'fixes'. Pretty code is of course nice to have, but I get the feeling that all code that does not conform to your strict coding standards is considered bad. Keep in mind the project is less than 500 lines of code. For such a small program, I don't think the current state of the code is unacceptable. As I said earlier, the main problems with such reformatting and changing end-of-line characters is that they make it hard to see what are the real changes in the code.

Now my review of the real changes:

1. mcstats is added. This is a way to track plugin usage. I don't know what we can do with that information, other than feel good (or bad) about the number of servers that are using it. Personally, I hate that and I think if we add it, it should be off by default. But that is just my opinion, so I won't make any suggestions here, the project owner, can decide by himself.
2. A flag isBackingUp is added. However, it is never set to true, so the check for isBackingUp() is pointless, it is always false. So while you assert that you tested this, it was not fully tested, because it is still possible to run a manual backup, while the scheduled backup is running.
3. Locking by world is done with 'synchronized (world)'. Fortunately, this will prevent backup corruption occuring because of isBackingUp not working. That said, I am not sure we want to lock the world object - we don't own that object and there may be code in the server that is also synchronized by world. If that is so, the backup may interfere with the normal server operation. What is worse, the proposed code still has this issue https://github.com/Exolius/SimpleBackup/issues/3, and if we add the fix for it, the synchronization can potentially cause a deadlock.
4. The 'h', 'd', 'w', 'm', and 'y' modifiers are refactored as enum. They are listed like that on a single place in the code, we'd need enum if they were used in multiple times. The worse part is that if the user makes a mistake, like typing '1n', the code will throw an IllegalArgumentException and the plugin will fail to load. This also makes the modifiers case-sensitive, and I am not sure we want them like that.

In conclusion:

• 2 and 3 above still need work. In my experience, when writing multi-threaded code it is very hard to get it right. That is why it is best to do the code in the simplest way possible. Currently, the plugin runs the backup in a separate thread and doesn't need anything more in that regard. That is why I don't think we need changes to the threading at all.
• 4 brings up a good point - currently, if the user makes an error in the configuration, we need to notify him, instead of silently ignoring the error. In my opinion, IllegalArgumentException and the plugin not loading is not a good way to notify them. We need a better message. @Exolius, if you think we should add error messages, please, post an issue.

As always, thanks for the time you put into this.

[gravypod]

Fixing my line endings