Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Some interface for Currency Conversion #3

Closed
Articdive opened this issue Sep 30, 2021 · 14 comments
Closed

Add Some interface for Currency Conversion #3

Articdive opened this issue Sep 30, 2021 · 14 comments
Assignees
Labels
priority: normal Normal priority status: confirmed Approved / validated status: needs testing Ready for testing; testing required to proceed status: waiting for poster Awaiting information or changes from the original poster type: improvement A feature is added or adjusted

Comments

@Articdive
Copy link

No description provided.

@lokka30 lokka30 self-assigned this Sep 30, 2021
@lokka30 lokka30 added the type: improvement A feature is added or adjusted label Sep 30, 2021
@lokka30
Copy link
Member

lokka30 commented Sep 30, 2021

I'm interested in your suggestion although I am unsure how to go about this. What are your thoughts on your suggestion? Do you have any ideas to accomplish this?

@Articdive
Copy link
Author

Articdive commented Sep 30, 2021

Since MyCurrency1 and MyCurrency2 both extend Currency. CurrencyConverter definitely needs 2 Currency Objects. (Do not use generics for this!). Something like abstract class CurrencyConverter(Currency a, Currency b). Maybe you can trick around to make it an Interface but I can imagine it having side effects. The API should probably allow for there to be more than 1 Converter between 2 Currencies

@lokka30
Copy link
Member

lokka30 commented Sep 30, 2021

Since MyCurrency1 and MyCurrency2 both extend Currency. CurrencyConverter definitely needs 2 Currency Objects. (Do not use generics for this!). Something like abstract class CurrencyConverter(Currency a, Currency b). Maybe you can trick around to make it an Interface but I can imagine it having side effects. The API should probably allow for there to be more than 1 Converter between 2 Currencies

What if currency conversion is facilitated by the economy provider? e.g. adding BigDecimal convert(Currency otherCurrency, BigDecimal amount) to the Currency interface, where the economy provider decides what logic should occur when converting to specific currencies?

@Articdive
Copy link
Author

Difficult since what if my 3rd party plugin is adding a converter, I'd rather not have to take care of him using Essentials or using a different EconomyProvider.

@lokka30 lokka30 added status: waiting for poster Awaiting information or changes from the original poster priority: high High priority labels Sep 30, 2021
@lokka30
Copy link
Member

lokka30 commented Sep 30, 2021

Difficult since what if my 3rd party plugin is adding a converter, I'd rather not have to take care of him using Essentials or using a different EconomyProvider.

I'm quite unsure how the currency conversion works then. Do currencies specify a 'global value' or something, which is essentially a multiplier based on how much the currency is valued compared to a static figure (1.0)?

e.g. 'tokens' is worth 1.5 since it is rare
e.g. 'dollars' is worth 0.9 since it is common

@Articdive
Copy link
Author

Articdive commented Sep 30, 2021

It would be best for Currency plugins themselves to handle conversion from/to their own currencies, without having to add in a sort of "global weight". Let's say my plugin adds currencies for every Towny town. then let my plugin take care of converting all of that, however give me an API.

@Articdive
Copy link
Author

Creating a converting system that won't break down eventually would be irrealistic because economies are dynamic. So save yourself the pain of "ensuring the converters work" and just allow for them to work and let others take care of that.

@lokka30
Copy link
Member

lokka30 commented Sep 30, 2021

It would be best for Currency plugins themselves to handle conversion from/to their own currencies, without having to add in a sort of "global weight". Let's say my plugin adds currencies for every Towny town. then let my plugin take care of converting all of that, however give me an API.

Okay - it's a little clearer now. Would the method I mentioned above still be suitable in your suggestion?

adding BigDecimal convert(Currency otherCurrency, BigDecimal amount) to the Currency interface

@Articdive
Copy link
Author

Articdive commented Sep 30, 2021

Not exactly since I can't change the code of Plugin A's Currency. It would be best if there was a CurrencyConverter object you can register, unregister , override for example.

@lokka30
Copy link
Member

lokka30 commented Sep 30, 2021

Not exactly since I can't change the code of Plugin A's Currency. It would be best if there was a CurrencyConverter object you can register, unregister , override for example.

I see what you're getting at now but for the past few minutes I'm still having trouble thinking about how this will be designed. I'm thinking a class containing a HashMap<Currency, HashMap<Currency, Priority>> where the first Currency is the currency being converted from, the second to, and Priority is an enum containing LOW, MEDIUM, HIGH. The override method will only be in effect if the current priority is lesser than or equal to the priority set in the register method unless overridden of course.

@Articdive
Copy link
Author

Articdive commented Sep 30, 2021

You could make the CurrencyConverter have a hashcode() of Currency A and Currency B (Objects.hash(a,b)) and then a HashMap and CurrencyConverter has a getPriority() method. When you want to convert from A to B then you would get the hashcode from Currency A and Currency B using the exact same method in CurrencyConverter.

HashMap<Converter, Priority> and you can get Converter's hashcode from Currency A and B.

Maybe better to use a HashSet or something like that and find the highest Priority.

@lokka30
Copy link
Member

lokka30 commented Sep 30, 2021

You could make the CurrencyConverter have a hashcode() of Currency A and Currency B (Objects.hash(a,b)) and then a HashMap and CurrencyConverter has a getPriority() method. When you want to convert from A to B then you would get the hashcode from Currency A and Currency B using the exact same method in CurrencyConverter.

HashMap<Converter, Priority> and you can get Converter's hashcode from Currency A and B.

Maybe better to use a HashSet or something like that and find the highest Priority.

I have never worked with hash codes before. Will be an interesting ride! 😃

@lokka30 lokka30 added help wanted Requires contributor help to solve. good first issue A great choice for new contributors to work on labels Sep 30, 2021
@lokka30 lokka30 added priority: normal Normal priority and removed help wanted Requires contributor help to solve. good first issue A great choice for new contributors to work on priority: high High priority status: waiting for poster Awaiting information or changes from the original poster labels Oct 8, 2021
@lokka30
Copy link
Member

lokka30 commented Oct 10, 2021

Hey @Articdive, I've completed the currency converter now. Here is the class if you are interested.

I went for a HashSet<CurrencyConversion> approach, not using any maps.

@lokka30 lokka30 added status: confirmed Approved / validated status: waiting for poster Awaiting information or changes from the original poster status: needs testing Ready for testing; testing required to proceed labels Oct 10, 2021
@lokka30 lokka30 closed this as completed Oct 10, 2021
@lokka30
Copy link
Member

lokka30 commented Dec 3, 2021

@Articdive, what do you think of the changes proposed in #49 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: normal Normal priority status: confirmed Approved / validated status: needs testing Ready for testing; testing required to proceed status: waiting for poster Awaiting information or changes from the original poster type: improvement A feature is added or adjusted
Projects
None yet
Development

No branches or pull requests

2 participants