1.16.x How can I optimize this?

Discussion in 'Spigot Plugin Development' started by kixmc, Oct 18, 2020 at 6:21 AM.

  1. Hello, I'm writing a plugin that needs to generate a random simple math equation. I suck when it comes to these kinda things, but here's what I have so far. It works perfectly, but I'd just like to optimize it so it's written as well as possible. Also, the solutions must always always be whole numbers - but the generator doesn't always come up with problems that result in whole numbers, and has to run through again until it makes one that works.

    I feel like I'm overcomplicating things but in all fairness I'm very sleep deprived and can't think that well, so I could just be crazy

    Code (Text):
        final static String[] operators = { "+", "-", "/", "*" };
        static Random r = new Random();

        public static String generateRandomEquation() {

            int num1 = r.nextInt(64);
            int num2 = r.nextInt(64);

            String spacedOp = " " + operators[r.nextInt(operators.length)] + " ";

            String equation = num1 + spacedOp + num2;
            double solution = eval(num1 + spacedOp + num2);

            if (numberIsWhole(solution) && solution >= 0) {
                return equation;
            }
            return generateRandomEquation();
        }

        public static boolean numberIsWhole(double input) {
            if ((input == Math.floor(input)) && !Double.isInfinite(input)) {
                return true;
            }
            return false;

        }

       public static double eval(String str) {
         // bunch of eval code here, not my focus now for optimization though
        }
     
     
    #1 kixmc, Oct 18, 2020 at 6:21 AM
    Last edited: Oct 18, 2020 at 8:36 PM
  2. i wonder why youre taking the approach that you are now and avoiding some of your interpretive uses, especially with the operators. however, you can certainly avoid the recursion and ensure proper generation on the first go by avoiding some possibilities. numberIsWhole will only happen with division with a remainder. you can use the modulus operator to figure out if the two numbers are evenly divisible and keep rerolling the numbers specifically until they are (tip, num1 will need to be the greater number for even division). the only time the solution can be negative is during subtraction, when num2 is greater than num1. simply put, switch the order if thats going to be the case.

    /e considering the communicative property, you could simply make the first number the larger number by default depending on what exactly youre doing with this. for multiplying and addition, youll get the same thing regardless, and for division and subtraction for your use case, the first one will be the larger anyhow.
     
    #2 Warren1001, Oct 18, 2020 at 6:42 AM
    Last edited: Oct 18, 2020 at 7:35 AM
    • Like Like x 1
  3. Make this an array of String

    Use ThreadLocalRandom

    Only relevant for division, the rest cannot be decimal results. Use modulo rather than brute forcing it. This will also get rid of the need to evaluate.

    edit: dammit Warren...
     
    • Like Like x 2
  4. I would do a couple of things differently. First, I'd use certain data structures for the operator itself (as an enum) and the equation (as a small data-class):
    Code (Java):

    enum Operator implements IntBinaryOperator {
        PLUS("+") {
            @Override
            public int applyAsInt(int left, int right) {
                return left + right;
            }
        },
        MINUS("-") {
            @Override
            public int applyAsInt(int left, int right) {
                return left - right;
            }
        },
        MULTIPLY("*") {
            @Override
            public int applyAsInt(int left, int right) {
                return left * right;
            }
        },
        DIVIDE("/") {
            @Override
            public int applyAsInt(int left, int right) {
                return left / right;
            }
        };

        private final String string;

        Operator(String string) {
            this.string = string;
        }

        @Override
        public String toString() {
            return string;
        }

       
        static Operator random() {
            final Operator[] values = values();
            return values[ThreadLocalRandom.current().nextInt(values.length)];
        }

    }
    This might seem a little overkill, but the advantage will become apparent in a bit.
    The second class is a simple "Pair" that holds the string-representation of the equation as well as the equation-result itself. This is merely to remove the overhead of evaluating the equation and then parsing it again since you already have the result as an int:
    Code (Java):
    static class Equation {

        public final String representation;
        public final int result;

        public Equation(String representation, int result) {
            this.representation = representation;
            this.result = result;
        }
    }
    and then finally the implementation is as simple as this:
    Code (Java):
    static Equation generateRandomEquation() {
        final ThreadLocalRandom random = ThreadLocalRandom.current();

        final Operator operator = Operator.random();
        final int first, second, result;

        switch (operator) {
            case PLUS:
            case MINUS:
            case MULTIPLY:
                first = random.nextInt(64);
                second = random.nextInt(64);
                result = operator.applyAsInt(first, second);
                break;
            case DIVIDE:
                result = random.nextInt(64);
                second = random.nextInt(result);
                first = result * second;
                break;
            default:
                throw new IllegalStateException("Unexpected value: " + operator);
        }
        return new Equation(first + " " + operator + " " + second, result);
    }
    This automatically removes the ability of the operation result not to be an int and does not need recursion or anything. It also calculates the result without having to parse the equation and allows for potential fine-tuning (you can simply remove a switch-branch and use lower numbers for multiplication, for example).
    Also adding another operator is really simple. You only have to add it to the enum and add another branch in the switch-statement.

    Another approach could be to move the whole logic directly to the enum. The enum could have an abstract method generateRandomEquation() which returns a single equation with the operator provided:
    Code (Java):
    enum Operator{
        PLUS {
            @Override
            public Equation generateRandomEquation() {
                final int first = random.nextInt(64);
                final int second = random.nextInt(64);
                return new Equation(first + " + " + second, first + second);
            }
        },
        MINUS {
            @Override
            public Equation generateRandomEquation() {
                final int first = random.nextInt(64);
                final int second = random.nextInt(64);
                return new Equation(first + " - " + second, first - second);
            }
        },
        MULTIPLY {
            @Override
            public Equation generateRandomEquation() {
                final int first = random.nextInt(16);
                final int second = random.nextInt(16);
                return new Equation(first + " * " + second, first * second);
            }
        },
        DIVIDE {
            @Override
            public Equation generateRandomEquation() {
                final int result = random.nextInt(64);
                final int second = random.nextInt(result);
                final int first = result * second;
                return new Equation(first + " / " + second, result);
            }
        };

        private static final Random random = new Random();

        static Operator random() {
            final Operator[] values = values();
            return values[random.nextInt(values.length)];
        }

        public abstract Equation generateRandomEquation();
    }
    with the usage
    Code (Java):
    Operator.random().generateRandomEquation();
    Now I don't really like this approach since there's a lot of duplicate code. Every branch has a very similar implementation, but I don't see a way to abstract that away...
    Anyhow, hope this helps!
     
    #4 Schottky, Oct 18, 2020 at 10:34 AM
    Last edited: Oct 18, 2020 at 4:20 PM
    • Winner Winner x 1
    • Creative Creative x 1
  5. You are really dedicated to making every possible variable final lol

    Don’t store a ThreadLocalRandom in a global variable

    I mean if the purpose was to include all the logic in the enum, there really is no reason you can’t put generateRandomEquation() in the enum, unstatic it and switch on the this instance
     
  6. Hell yea. If it's not final it might start a revolution or something...
    Agreed (thou it won't matter since Spigot is mostly singe-threaded. On the other hand, the chat is handled Async which might be a use-case for this functionality)
    Well here's the issue. generateRandomEquation() doesn't really belong to the class Operator. An operator (actually I should have been more specific and maybe call that enum BinaryOperator) should only be able to provide its symbol and evaluate an expression. The first approach tries to do that. generateRandomEquation() should actually be a method of the class Equation now that I think about it.
    The latter approach was an attempt to fuse the two methods and do exactly what you said; unstatic generateRandomEquation() and make it an instance-method of the enum. But then it felt wrong to switch on the enum itself when you can just make it an abstract method.
     
    • Like Like x 1
  7. Super informative, I'll definitely take all this into consideration. Thanks for taking the time to write all that up!!