Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have created a tool that lets me calculate the profitability of doing certain actions in an MMORPG game which I play, it's more edging towards a quick-and-dirty solution than towards a neat solution because it's simply a tool and does not need excessive robustness, however I do think there is room for improvements.

I'd like a review on all aspects, first I'll start of by explaining what it is really about as some domain knowledge is required.

Domain knowledge

This tool focuses on items of 29 Defensive Rating (DR) as input and it is all about upgrading the items, ultimately the goal is to produce a 32 DR item, which can be sold for a fair amount of money.

The item comes with an upgrade of level 0, and for these practices it needs to be upgraded to level 7, though it can be upgraded beyond in the game, here is a table with characteristics:

  • Upgrade from 0 to 1: Costs 141,000, 100% regular chance
  • Upgrade from 1 to 2: Costs 141,000, 100% regular chance
  • Upgrade from 2 to 3: Costs 141,000, 100% regular chance
  • Upgrade from 3 to 4: Costs 141,000, 100% regular chance
  • Upgrade from 4 to 5: Costs 141,000, 50% regular chance
  • Upgrade from 5 to 6: Costs 211,500, 25% regular chance
  • Upgrade from 6 to 7: Costs 282,000, 12.5% regular chance

When an item succeeds to 7, it can be converted to a "32DR crystal", which can be sold.

When an item fails, it will produce 1~15 Dazzling Ores and a Level 100 Chip. If the upgrade that failed was from 6 to 7, it will produce 2 Level 100 Chips. Upon failure the item is lost.

To increase upgrade success, you can add 500 Shiny Crystals from upgrade from 4 to 5 onwards, which will double your success chance.

My goal is to calculate the most profitable method (possibly negative profit, also called loss) given the price of the 29 DR item.

Implementation-wise, I have decided for now to let it run on the console for ease of use and with a few variable parameters hard-coded. they resemble the price/value of an item in the game and that, because it is dependent upon players, may vary from day to day. It's just for personal use, so I do not think it is bad to recompile it every day.

In the future I want to support multiple types of items and goals (upgrade levels) though, where the following details are variable:

  • The upgrade costs
  • The maximum amount of Shiny Crystals needed for upgrading
  • The minimum and maximum amount of Dazzling Ores the item produces upon failure.

This is my code:

public class GE32Tool {
    /* Variable data */
    private final static int DAZZLING_ORE_PRICE = 120_000;
    private final static int CRYSTAL_32DR_PRICE = 45_000_000;
    private final static int LVL_100_CHIP_PRICE = 190_000;
    private final static int SHINY_CRYSTAL_PRICE = 6_000;

    /* Constant data */
    private final static int UPGRADE_MAXIMUM = 7;
    private final static int SHINY_CRYSTAL_MAXIMUM = 500;

    private final static int[] BASE_UPGRADE_COSTS = {
        141_000,    //to 1
        141_000,    //to 2
        141_000,    //to 3
        141_000,    //to 4
        141_000,    //to 5
        211_500,    //to 6
        282_000,    //to 7
    };

    private final static float[] BASE_UPGRADE_SUCCESS = {
        1,      //to 1
        1,      //to 2
        1,      //to 3
        1,      //to 4
        0.5f,   //to 5
        0.25f,  //to 6
        0.125f, //to 7
    };

    private final static int[] UPGRADE_FAILURE_CHIPS = {
        1,  //to 1
        1,  //to 2
        1,  //to 3
        1,  //to 4
        1,  //to 5
        1,  //to 6
        2,  //to 7
    };

    private final static int UPGRADE_DAZZLING_ORE_MIN = 1;
    private final static int UPGRADE_DAZZLING_ORE_MAX = 15;
    private final static float UPGRADE_DAZZLING_ORE_AVG = (UPGRADE_DAZZLING_ORE_MIN + UPGRADE_DAZZLING_ORE_MAX) / 2f;

    private void calculate() {
        Scanner scanner = new Scanner(System.in);
        System.out.print("Le Noir price: ");
        int leNoirPrice = scanner.nextInt();
        System.out.println();

        int initialCost = leNoirPrice + Arrays.stream(BASE_UPGRADE_COSTS, 0, 4).sum();

        Map<String, Float> costs = new HashMap<>();
        addToMap(costs, false, false, false);
        addToMap(costs, false, false, true);
        addToMap(costs, false, true, false);
        addToMap(costs, false, true, true);
        addToMap(costs, true, false, false);
        addToMap(costs, true, false, true);
        addToMap(costs, true, true, false);
        addToMap(costs, true, true, true);
        Map.Entry<String, Float> bestResult = costs.entrySet().stream()
                .sorted(Comparator.comparingDouble(entry -> entry.getValue()))
                .findFirst()
                .get();

        float finalProfit = -(initialCost + bestResult.getValue());
        System.out.println(String.format("Expected profit: %.2f", finalProfit));
        System.out.println("Using " + bestResult.getKey());
    }

    private void addToMap(final Map<String, Float> costs, final boolean upgrade5ShinyCrystals, final boolean upgrade6ShinyCrystals, final boolean upgrade7ShinyCrystals) {
        costs.put("5=" + upgrade5ShinyCrystals + " / 6=" + upgrade6ShinyCrystals + " / 7=" + upgrade7ShinyCrystals, 
                calculateUpgradeCosts(
                        upgradeCostsFor(upgrade5ShinyCrystals, upgrade6ShinyCrystals, upgrade7ShinyCrystals), 
                        upgradeSuccessFor(upgrade5ShinyCrystals, upgrade6ShinyCrystals, upgrade7ShinyCrystals), 
                        5
                )
        );
    }

    private float[] upgradeSuccessFor(final boolean upgrade5ShinyCrystals, final boolean upgrade6ShinyCrystals, final boolean upgrade7ShinyCrystals) {
        return new float[]{
            BASE_UPGRADE_SUCCESS[0],
            BASE_UPGRADE_SUCCESS[1],
            BASE_UPGRADE_SUCCESS[2],
            BASE_UPGRADE_SUCCESS[3],
            BASE_UPGRADE_SUCCESS[4] + (upgrade5ShinyCrystals ? BASE_UPGRADE_SUCCESS[4] : 0),
            BASE_UPGRADE_SUCCESS[5] + (upgrade6ShinyCrystals ? BASE_UPGRADE_SUCCESS[5] : 0),
            BASE_UPGRADE_SUCCESS[6] + (upgrade7ShinyCrystals ? BASE_UPGRADE_SUCCESS[6] : 0),
        };
    }

    private int[] upgradeCostsFor(final boolean upgrade5ShinyCrystals, final boolean upgrade6ShinyCrystals, final boolean upgrade7ShinyCrystals) {
        return new int[]{
            BASE_UPGRADE_COSTS[0],
            BASE_UPGRADE_COSTS[1],
            BASE_UPGRADE_COSTS[2],
            BASE_UPGRADE_COSTS[3],
            BASE_UPGRADE_COSTS[4] + (upgrade5ShinyCrystals ? SHINY_CRYSTAL_MAXIMUM * SHINY_CRYSTAL_PRICE : 0),
            BASE_UPGRADE_COSTS[5] + (upgrade6ShinyCrystals ? SHINY_CRYSTAL_MAXIMUM * SHINY_CRYSTAL_PRICE : 0),
            BASE_UPGRADE_COSTS[6] + (upgrade7ShinyCrystals ? SHINY_CRYSTAL_MAXIMUM * SHINY_CRYSTAL_PRICE : 0),
        };
    }

    private float calculateUpgradeCosts(final int[] upgradeCosts, final float[] upgradeSuccess, final int upgradeLevel) {
        if (upgradeLevel == UPGRADE_MAXIMUM + 1) {
            return -CRYSTAL_32DR_PRICE;
        }
        float successChance = upgradeSuccess[upgradeLevel - 1];
        float successCost = successChance * 
                (upgradeCosts[upgradeLevel - 1] + calculateUpgradeCosts(upgradeCosts, upgradeSuccess, upgradeLevel + 1));
        float failureCost = (1f - successChance) * 
                (upgradeCosts[upgradeLevel - 1] - (UPGRADE_DAZZLING_ORE_AVG * DAZZLING_ORE_PRICE) - (UPGRADE_FAILURE_CHIPS[upgradeLevel - 1] * LVL_100_CHIP_PRICE));
        return successCost + failureCost;
    }

    public static void main(String[] args) {
        new GE32Tool().calculate();
    }
}

(For reference with respect to variable names: The 29 DR item is called a "Le Noir" Armor)

When testing this application, you can use prices in the range of 300,000 to 1,000,000, that is the regular market value in the game.

share|improve this question
4  
Thanks for including a "domain knowledge" section to your post. I wish everyone would do this. –  toto2 Jul 10 at 16:45
add comment

3 Answers 3

Structural

At the moment, a lot of the details you would in the future want to abstract away (for your idea of multiple item types and goals) are sprayed all over your code. For example, look at how many places you have variables with names like upgrade7ShinyCrystals. While this could be avoided with data structures like arrays, you'd fast get into a situation of further complicating already quite complicated code. This is likely to lead to brittle code, where if you find yourself asking a question like "What if I want to also include upgrading to a lower level then selling in the profitability calculations?" you'll have difficulty getting an answer.

For this reason, I'd suggest a more object-oriented approach. Start by thinking about how to structure your data and functionality, and then go from there.

I'd suggest that your basic class at this point is ItemAtLevel. I'm calling it that to separate it from the more general concept of just an Item, where you might think of the current level as just one particular field. Most of the important information about an item is specific per level, so it would have fields for:

  • Value
  • Whether it is the highest level available
  • Probability of success upgrading to next level
  • Whether shiny crystals can be used
  • The value of items produced when the upgrade fails

I'd also suggest having a method to get the item at the next level, which would have this serve as a singly-linked list.

Algorithmic

I don't know whether you will ever be using this with a high enough volume of items, or with items where the potential upgrade paths are long/complex enough, for performance to matter. But iterating over every possible combination of crystal usage is needlessly inefficient, and also imposes unnecessary constraints on how you structure your main loops which might make the program harder to alter later.

It's easy to calculate whether or not to use shiny crystals at an item level. The condition is:

failureValue*(1-P) + successValue*P < failureValue*(1-2P) + successValue*2P - crystalCost

Which rearranges to:

crystalCost < (successValue-failureValue)*P 

Here, success and failure value are the values of the products you get for each result, and P is the probability of failure.

Note that this is something that depends only on features of a single level. So you can calculate individually for each level whether or not to use the crystals.

Putting it together

So putting that together, ItemAtLevel can have a boolean shouldUseCrystals() method which uses the above formula to determine whether or not it should use Crystals, then an int averageUpgradeProfit(bool useCrystals) method, which would be a simple calculation of the expected profit including both success and failure possibilities, along the lines of the one you do in your code.

With that, all you need to do is construct the ItemAtLevel instances for each level of your item- or items of interest, and then the rest is extremely simple. If you wanted to know the total expected profit upgrading to your highest level, you could just use a loop (or stream function) to total up the average profit at each level. If you wanted to know the optimal level to stop upgrading, it wouldn't be much harder.

(As a side-note, one assumption I've made is that you can buy and sell items at any upgrade level, and that the level will be reflected in the price. I don't think much of what I've said would be very significantly different if that wasn't the case, but there would be some alterations)

share|improve this answer
    
It doesn't appear that you directly know the value of the item in the intermediate upgrade levels -- its value is based on the expectation of return on future upgrades. I think you can find it by going in reverse (analyzing the ultimate upgrade first), though. –  Ben Voigt Jul 10 at 20:59
    
@BenVoigt Yeah, this was what I meant with my final parenthetical. It does unfortunately make things a bit more complicated. –  Ben Aaronson Jul 11 at 9:44
add comment

You're looping through every possible permutation of three boolean values. This could be achieved by looping on booleans:

final boolean[] BOOLEANS = new boolean[] { false, true };
for (final boolean upgrade5ShinyCrystals : BOOLEANS)
    for (final boolean upgrade6ShinyCrystals : BOOLEANS)
        for (final boolean upgrade7ShinyCrystals : BOOLEANS)
            addToMap(costs, upgrade5ShinyCrystals, upgrade6ShinyCrystals, upgrade7ShinyCrystals);

You can also try looping through the values 0 to 7, inclusive, but this is messy; it works a lot better in C than Java, since you don't need explicit boolean coercion.

for (byte b = 0b000; b <= 0b111; b++)
    addToMap(costs, (b & 0b100) != 0, (b & 0b010) != 0, (b & 0b001) != 0);

Demo on ideone.

share|improve this answer
add comment

Good to see you are using Java 8.

  • About the four hard coded prices, ideally you would connect to the game server through some public api to get those values. Otherwise, it makes more sense to write them to a file. Also, it's not clear why "Le Noir" price is treated differently from the other four prices.

  • It would be cleaner to put the code that asks the user for the "Le Noir" price in a separate method. You can also change calculate() so that it takes that price as a parameter. (This advice does not hold if you put all prices in one file.)

  • Your code screams ENUM. There are many places where you define various quantities in arrays of size 7. You should instead have an enum with the seven transitions where each enum value contains all the values you define in scattered arrays. You can put many different members and methods in an enum. See the tutorial.

  • There is a "logic error" in your code: You should not test all 8 boolean triplet combinations. When an item fails to upgrade, you won't try later upgrades.

  • There is a "statistical error": you don't want to compare the max value to the initial cost, but the average. If the average outcome is greater than the initial cost, you try the upgrades, and you don't otherwise.

  • I think you were looking for DoubleStream.max() when computing bestResult. You should probably switch from float to double anyway: there is no advantage to using float here, and it is annoying since you have to add a trailing f to your numbers. Note that as I mentioned above, you should compute the average and not the max, but there is also a method DoubleStream.average().

  • I really don't like the addToMap method. Wrap everything related to that in an inner class. And you should be careful with naming: it is not obvious at all from the name that addToMap actually does the cost calculation.

  • Use Collections instead of raw arrays since it is cleaner (more OO). But if you followed my advice, you won't have many raw arrays left at this point anyway.

share|improve this answer
    
Unfortunately I cannot agree with all points. I do not think I should be using enums as they indicate prices and you want easy access to them on an indexed basis. Also, the "logic error" may not be an error, as far as I know I do not try to upgrade an item again that has failed; though because I have those two bits of logic separated, it may do some redundant calculations. It may also be that my question was a bit unclear. I do think the bestResult is correct, as I (as player) have the choice on which of the 8 options to take, in there it has nothing to do with probability. –  skiwi Jul 10 at 17:29
    
Overall your answer did show a bunch of good improvements though, ones which I cannot believe I have missed, but that is what you get for not programming in OOP style. –  skiwi Jul 10 at 17:30
    
You can still order the elements of an enum. For example, you can have a static list (or some kind of iterator) within the enum definition that contains the 7 transition enum values in order. –  toto2 Jul 10 at 17:40
    
The question would then be more along the lines of: What would I gain by rewriting it as an enum? I do not see much gain by doing that. –  skiwi Jul 10 at 18:01
    
Your code would be a lot cleaner. It looks like a C program at the moment rather than OO. Cleaner code is easier to read and maintain. For an example of maintainability: if they add an eight upgrade level in the game, you would have to change values everywhere in your code, but only in one place if you used enums. And generally cleaner code has fewer bugs: it's easy for a bug to sneak in complex code. –  toto2 Jul 10 at 18:20
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.