Add basics for buy command by mcglonelevi · Pull Request #2795 · EssentialsX/Essentials

mcglonelevi · 2019-09-26T02:53:38Z

This PR is the start of the /buy command per this issue:

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:

Add the translation files.
I should probably add some logging similar to the Sell command.
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)
Most of the logic is in one massive function. I'd like to break it out into its pieces as methods to make it more readable.

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

mdcfe · 2019-09-30T10:25:49Z

One thing I did think about adding is some kind of "buy multiplier" in the config. [...] 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?

I think we could add optional buy and sell sections to worth.yml, such that buy and sell section values take priority over worth section values.

mcglonelevi · 2019-09-30T23:17:07Z

One thing I did think about adding is some kind of "buy multiplier" in the config. [...] 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?

I think we could add optional buy and sell sections to worth.yml, such that buy and sell section values take priority over worth section values.

I'll work on implementing this.

mcglonelevi · 2019-10-01T00:04:11Z

@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.

mdcfe · 2019-10-01T16:29:30Z

Currently looks good, though I don't have time to go over and do a full review of the PR right now.

Ichbinjoe

This is a good start but I think there are some changes that can be made to make this into a great diff!

Ichbinjoe · 2019-10-03T07:09:47Z

+
+        // 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());


nit: Don't separate the declaration from the initial assignment - there isn't any reason to separate it.

This was fixed in my refactor.

Ichbinjoe · 2019-10-03T07:14:14Z

+        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());


BigDecimal.ONE.negate() is used so much... maybe makes sense to extract it away as a constant?

I refactored the function and accounted for this change.

Ichbinjoe · 2019-10-03T07:15:16Z

+     * @param itemStack The item stack to look up in the config.
+     * @return The price from the config.
+     */
+    public BigDecimal getBuyPrice(IEssentials ess, ItemStack itemStack) {


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.

Totally agree. I just refactored this.

Ichbinjoe · 2019-10-03T07:17:48Z

+
+    @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])) {


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.

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.

I took my best shot at this. Let me know if my latest code works.

Ichbinjoe · 2019-10-03T07:18:54Z

+            throw new NotEnoughArgumentsException();
+        }
+
+        int amountToGive = NumberUtils.toInt(args[1]);


Can this be defaulted to a reasonable value (like 1) if not provided?

Ichbinjoe · 2019-10-03T07:23:50Z

+            		leftoverValue = leftoverValue.add(worthSingleItem.multiply(BigDecimal.valueOf(item.getAmount())));
+                }
+
+            	user.sendMessage("Not enough inventory space. Refunding $<amount>.");


This needs a format string to it can be localized to different languages.

Also did we really refund them if we never took it to begin with?

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.

Ichbinjoe · 2019-10-03T07:24:11Z

+        int amountToGive = NumberUtils.toInt(args[1]);
+
+        if (amountToGive <= 0) {
+        	throw new Exception("You cannot buy 0 items.");


This needs a localization lookup.

Ichbinjoe · 2019-10-03T07:24:23Z

+        	user.takeMoney(worth.subtract(leftoverValue));
+        	user.getBase().updateInventory();
+        } else {
+        	throw new Exception("You do not have enough money.");


Also needs a localization lookup

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?

Don't remember who told me this, but I add my messages to just messages.properties. You can ignore the other files.

Ichbinjoe · 2019-10-03T07:26:58Z

+                }
+            } else if(!leftovers.values().isEmpty()) {
+            	for (ItemStack item : leftovers.values()) {
+            		leftoverValue = leftoverValue.add(worthSingleItem.multiply(BigDecimal.valueOf(item.getAmount())));


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)

I'll fix this is a separate commit.

mdcfe · 2019-10-13T06:56:22Z

        }

-        BigDecimal worth = ess.getWorth().getPrice(ess, is);
+        BigDecimal worth = ess.getWorth().getSellPrice(ess, is);


Should /worth list both the buying and selling prices?

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.

pop4959 · 2020-05-27T05:23:04Z

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.

superspeed500 · 2025-12-21T15:47:08Z

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?

add basics for buy command

4b51f61

mcglonelevi mentioned this pull request Sep 26, 2019

/sell exists but /buy does not? #2785

Open

mcglonelevi added 2 commits September 30, 2019 12:29

fix bug with full inventory and check null worth

7494900

support reading alternate buy and sell config values

3d15488

Ichbinjoe suggested changes Oct 3, 2019

View reviewed changes

mdcfe added the type: enhancement Features and feature requests. label Oct 3, 2019

mdcfe added this to the 2.18.0 milestone Oct 3, 2019

mcglonelevi added 2 commits October 3, 2019 11:16

refactor Worth.java changes

bd8a6f5

handle fallback for invalid integers

0409afe

mdcfe reviewed Oct 13, 2019

View reviewed changes

Ichbinjoe added the status: waiting on author Pull requests that require changes from the author in order to merge. label Dec 29, 2019

mdcfe removed this from the 2.18.0 milestone Apr 30, 2020

mdcfe mentioned this pull request Apr 30, 2020

buy / sell signs with prices from worth.yml (not player-editable prices) #2638

Closed

pop4959 closed this May 27, 2020

moxventura mentioned this pull request Jun 11, 2020

Allow signs to update price from worth #3367

Closed

Uh oh!

Conversation

mcglonelevi commented Sep 26, 2019 • edited by pop4959 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdcfe commented Sep 30, 2019

Uh oh!

mcglonelevi commented Sep 30, 2019

Uh oh!

mcglonelevi commented Oct 1, 2019

Uh oh!

mdcfe commented Oct 1, 2019

Uh oh!

Ichbinjoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pop4959 commented May 27, 2020

Uh oh!

superspeed500 commented Dec 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mcglonelevi commented Sep 26, 2019 •

edited by pop4959

Loading