Add basics for buy command #2795
Conversation
I think we could add optional |
I'll work on implementing this. |
|
@md678685 - Added the ability to read alternate config values specific to buying/selling. If you have any thoughts based on these changes, let me know. |
|
Currently looks good, though I don't have time to go over and do a full review of the PR right now. |
Ichbinjoe
left a comment
Ichbinjoe
left a comment
There was a problem hiding this comment.
This is a good start but I think there are some changes that can be made to make this into a great diff!
|
|
||
| // Check for matches with data value from stack | ||
| // Note that we always default to BigDecimal.ONE.negate(), equivalent to -1 | ||
| result = config.getBigDecimal("buy." + itemname + "." + itemStack.getDurability(), BigDecimal.ONE.negate()); |
There was a problem hiding this comment.
nit: Don't separate the declaration from the initial assignment - there isn't any reason to separate it.
There was a problem hiding this comment.
This was fixed in my refactor.
| if (result.signum() < 0) { | ||
| final ConfigurationSection itemNameMatch = config.getConfigurationSection("buy." + itemname); | ||
| if (itemNameMatch != null && itemNameMatch.getKeys(false).size() == 1) { | ||
| result = config.getBigDecimal("buy." + itemname + ".0", BigDecimal.ONE.negate()); |
There was a problem hiding this comment.
BigDecimal.ONE.negate() is used so much... maybe makes sense to extract it away as a constant?
There was a problem hiding this comment.
I refactored the function and accounted for this change.
| * @param itemStack The item stack to look up in the config. | ||
| * @return The price from the config. | ||
| */ | ||
| public BigDecimal getBuyPrice(IEssentials ess, ItemStack itemStack) { |
There was a problem hiding this comment.
I feel like this whole function (as well as getSellPrice) can be folded into getPrice() where getPrice() has an additional argument for which 'keyspace' it is looking up the price in i.e. the 'buy' keyspace vs the 'worth' keyspace.
There was a problem hiding this comment.
Totally agree. I just refactored this.
|
|
||
| @Override | ||
| public void run(final Server server, final User user, final String commandLabel, final String[] args) throws Exception { | ||
| if (args.length < 2 || !NumberUtil.isInt(args[1])) { |
There was a problem hiding this comment.
If NumberUtil.isInt fails here, it isn't really a case of not enough arguments - this will result in a confusing error message for the user.
There was a problem hiding this comment.
I have not written much Java and this is my first time contributing to this repo. What function should I use to perform this check? Essentially, I just want to make sure that args[1] can be parsed to an integer value and is not a string/float/etc.
There was a problem hiding this comment.
I took my best shot at this. Let me know if my latest code works.
| throw new NotEnoughArgumentsException(); | ||
| } | ||
|
|
||
| int amountToGive = NumberUtils.toInt(args[1]); |
There was a problem hiding this comment.
Can this be defaulted to a reasonable value (like 1) if not provided?
| leftoverValue = leftoverValue.add(worthSingleItem.multiply(BigDecimal.valueOf(item.getAmount()))); | ||
| } | ||
|
|
||
| user.sendMessage("Not enough inventory space. Refunding $<amount>."); |
There was a problem hiding this comment.
This needs a format string to it can be localized to different languages.
There was a problem hiding this comment.
Also did we really refund them if we never took it to begin with?
There was a problem hiding this comment.
Agreed. I couldn't think of a better way to phrase it. Maybe just a message of something like "You didn't have enough inventory space. You purchased X items instead of Y."
Any input here is appreciated.
| int amountToGive = NumberUtils.toInt(args[1]); | ||
|
|
||
| if (amountToGive <= 0) { | ||
| throw new Exception("You cannot buy 0 items."); |
There was a problem hiding this comment.
This needs a localization lookup.
| user.takeMoney(worth.subtract(leftoverValue)); | ||
| user.getBase().updateInventory(); | ||
| } else { | ||
| throw new Exception("You do not have enough money."); |
There was a problem hiding this comment.
Also needs a localization lookup
There was a problem hiding this comment.
First time contributing to this repo, so please forgive my ignorance here. I understand how i18n works from the other files. Am I required to provide a translation for each language as a part of this PR? Do you have a recommended translator or translation tool I should use to do the translations?
There was a problem hiding this comment.
Don't remember who told me this, but I add my messages to just messages.properties. You can ignore the other files.
| } | ||
| } else if(!leftovers.values().isEmpty()) { | ||
| for (ItemStack item : leftovers.values()) { | ||
| leftoverValue = leftoverValue.add(worthSingleItem.multiply(BigDecimal.valueOf(item.getAmount()))); |
There was a problem hiding this comment.
So this is more expensive (cpu time) / more verbose than counting how many total leftovers there are then doing a single BigDecimal multiply operation (no need for subsequent BigDecimal add operations)
There was a problem hiding this comment.
I'll fix this is a separate commit.
| } | ||
|
|
||
| BigDecimal worth = ess.getWorth().getPrice(ess, is); | ||
| BigDecimal worth = ess.getWorth().getSellPrice(ess, is); |
There was a problem hiding this comment.
Should /worth list both the buying and selling prices?
There was a problem hiding this comment.
There should probably be a way to configure whether or not it shows up, in the case that servers don't want to use both buy and sell at the same time, for example.
Ichbinjoe
added
the
status: waiting on author
|
Closing this PR since the author has been inactive for several months. Can re-open if there are signs of life, otherwise this is open to anyone else who may be interested. |
I am interested in looking into this, even though I have very little experience contributing to code. I have some coding experience though, but not much with Java. This looks like a nice challenge. Create a new PR or reopen this? |
6 participants
This PR is the start of the /buy command per this issue:
#2785
I am relatively new to Spigot and Java development, so please let me know if you see a better way to do this. I based most of this code off of Commandgive and Commandsell. The issue was pretty non-descript about exact functionality of the sell command, so I have built the command currently so that you can do
/sell <item> <amount>. I am creating a pull request early to get some feedback on my approach.Still to do:
There is a small bug where if the user's inventory is full and they try to buy something, the value withdrawn from their account is incorrect. I believe this is due to my not re-assigning in the foreach loop. (will fix soon)If you guys see anything that I could improve in my approach, please let me know. I am sure I have missed an edge-case somewhere.
One thing I did think about adding is some kind of "buy multiplier" in the config. Many servers want players to sell an item for $3 but buy the same item for $30. This multiplier could be configurable as a float value in the config. I'd appreciate any feedback or suggestions here. Maybe it is worth creating a new section of the config that allows server admins to set on an item-by-item basis as well?Implementing Buy and Sell overrides in worth.yml per thread below.
Closes #2785