[Done] My API Feedback

[lokka30]

I have been writing a test economy provider plugin so I have been able to see what it's like to use Treasury. So far, these are my thoughts on what might need changing.

Check the branch for all of the pending commits

To-do list

• Currency#isDefault should probably be named Currency#isPrimary, to suit the other methods in the EconomyProvider interface which are named 'primary'.
• EconomyFailureReason seems to miss out on a few things. I'll brainstorm these.
• Currency#getDisplayName should probably be named Currency#getDisplayNameSingular, to pertain to Currency#getDisplayNamePlural.
• I'm unsure about Account Permissions being TriStates (true/false/unspecified), I think a simple dual state (true/false) might be better. How is a plugin meant to react when the player isn't known to have or not have a permission? I think it should just default to false in this case.
• I think transactions should have a method that states if they are a 'common' transaction or not. E.g. a common transaction could be the JobsReborn plugin rewarding a user for a task. Then an economy provider plugin can choose not to store these common transactions in a database to save storage space.

Scrapped ideas

• Scrapped: I think Currency#to should be a default method, using MrNemo's formula from a month or two ago. This will also require adding a Currency#getRelativeValue method, e.g. dollars are worth 1.0 and tokens are worth 100.0, so tokens are 100x more valuable than dollars. needs to work with decimal amounts too like 0.5, that's where MrNemo's formula comes in.
• Scrapped: Ivan, didn't you create a currency parsing method? Because Currency#parse is not a default method. It's no biggie if we decide to remove it but it's unfortunate that all that time spent creating it, and it's no longer here?

[lokka30]

Pushed some new commits. 😃

e05a9e7

• switches back to TriState for permissions.
• removes (what I think are) some counter-intuitive defaults from PlayerAccount.java I reverted this change in the 3rd commit, somehow I didn't realise that these played an important role. I apologise for that!

4808176

• renames Currency#getDisplayName to Currency#getDisplayNameSingular, to pertain to Currency#getDisplayNamePlural.
• suppresses single deprecation warning in the migrator for using the 'get current API version' method.

68378f9

• reverts PlayerAccount default removals from the first commit
• minor corrections with grammar and unused imports.

[yannicklamprecht]

I'm on @MrNemo64's side @Nullable is more user-friendly. I would personally just overload the method with one argument less.
See the external reasons. The SonarQube rule lists all disadvantages you will have when using optional as a parameter.
https://stackoverflow.com/a/31923105/6686790 | https://stackoverflow.com/a/39005452/6686790
https://rules.sonarsource.com/java/RSPEC-3553

[lokka30]

Thanks for the feedback. @MrIvanPlays does this change your opinion at all?

[MrIvanPlays]

i saw your code and reviewed. your usage of them is wrong so you have to revert them. we can skip discussing whether to use optional or not for now.

[MrNemo64]

I'd like to link this stack overflow post about optionals as parameters
Also you are basically achieving nothing, you still need to check if the given optional is, itself, null and all the other parameters
Optionals exist to force the developer to handle a null value, not to indicate that something can be null

[lokka30]

Thanks guys. I've addressed this in the latest array of commits 👌🏻