2
\$\begingroup\$

I'm working on a skill tree calculator for a game and I made a method that checks whether or not it can subtract a point from the skill and then proceeds to do so. Everything is working as it should, however it's very slow when this method is executed in rapid succession (i.e. when 5 points are subtract at the same time). Can anyone help me with this and more important explain why certain things are faster and what to avoid?

public void cmdSubtractPoint(int index, SkillTree tree) {

    boolean subtract = false;

    if (this.blueTree.get(index).getInvestedPoints() != 0) {
        //Only subtract a point if the skill isn't empty

        if (index == 0 || index == 1) {
            if ((((this.blueTree.get(0).getInvestedPoints() + this.blueTree.get(1).getInvestedPoints()) - 1) < 5)
                    && ((this.blueTree.get(2).getInvestedPoints() + this.blueTree.get(3).getInvestedPoints() +
                    this.blueTree.get(4).getInvestedPoints() + this.blueTree.get(5).getInvestedPoints() + this.blueTree.get(6).getInvestedPoints()) != 0)) {
                this.showToast();
            } else if ((((this.blueTree.get(0).getInvestedPoints() + this.blueTree.get(1).getInvestedPoints()) + this.blueTree.get(2).getInvestedPoints() + this.blueTree.get(3).getInvestedPoints() - 1) < 10)
                    && ((this.blueTree.get(4).getInvestedPoints() + this.blueTree.get(5).getInvestedPoints() + this.blueTree.get(6).getInvestedPoints()) != 0)) {
                this.showToast();
            } else if ((((this.blueTree.get(0).getInvestedPoints() + this.blueTree.get(1).getInvestedPoints()) + this.blueTree.get(2).getInvestedPoints() + this.blueTree.get(3).getInvestedPoints() + this.blueTree.get(4).getInvestedPoints() + this.blueTree.get(5).getInvestedPoints() - 1) < 15)
                    && ((this.blueTree.get(6).getInvestedPoints()) != 0)) {
                this.showToast();
            } else {
                subtract = true;
            }
        } else if (index == 2 || index == 3) {
            if ((((this.blueTree.get(0).getInvestedPoints() + this.blueTree.get(1).getInvestedPoints()) + this.blueTree.get(2).getInvestedPoints() + this.blueTree.get(3).getInvestedPoints() - 1) < 10)
                    && ((this.blueTree.get(4).getInvestedPoints() + this.blueTree.get(5).getInvestedPoints() + this.blueTree.get(6).getInvestedPoints()) != 0)) {
                this.showToast();
            } else if ((((this.blueTree.get(0).getInvestedPoints() + this.blueTree.get(1).getInvestedPoints()) + this.blueTree.get(2).getInvestedPoints() + this.blueTree.get(3).getInvestedPoints() + this.blueTree.get(4).getInvestedPoints() + this.blueTree.get(5).getInvestedPoints() - 1) < 15)
                    && ((this.blueTree.get(6).getInvestedPoints()) != 0)) {
                this.showToast();
            } else {
                subtract = true;
            }
        } else if (index == 4 || index == 5) {
            if ((((this.blueTree.get(0).getInvestedPoints() + this.blueTree.get(1).getInvestedPoints()) + this.blueTree.get(2).getInvestedPoints() + this.blueTree.get(3).getInvestedPoints() + this.blueTree.get(4).getInvestedPoints() + this.blueTree.get(5).getInvestedPoints() - 1) < 15)
                    && ((this.blueTree.get(6).getInvestedPoints()) != 0)) {
                this.showToast();
            } else {
                subtract = true;
            }
        } else if (index == 6) {
                subtract = true;
        }

        if (subtract) {
            this.pointsInBlueTree -= 1;
            this.blueTree.get(index).subtractPoint();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Can anyone help me with this and more important explain why certain things are faster and what to avoid. That comes dangerously close to code understanding instead of review. Have you benchmarked your code to see how slow 'slow' is? \$\endgroup\$ – Mast Aug 5 '15 at 14:06
  • \$\begingroup\$ I have not, but there is a noticeable delay, and I have already posted this to stackOverflow but they told me to post it here. \$\endgroup\$ – NoahKr Aug 5 '15 at 14:09
  • \$\begingroup\$ Is SkillTree a class you wrote? Maybe its get method speed can be increased. Can you post it too (if it's not too big) ? \$\endgroup\$ – Diego Martinoia Aug 5 '15 at 14:19
  • \$\begingroup\$ SkillTree is an enumeration of three entries, that shouldn't be the problem. Do you still want me to post it? \$\endgroup\$ – NoahKr Aug 5 '15 at 14:21
  • 1
    \$\begingroup\$ I see an argument called tree but I don't see it used in your function. Perhaps you could explain what your function is supposed to do with that argument? \$\endgroup\$ – JS1 Aug 5 '15 at 17:36
2
\$\begingroup\$

This is pretty incomplete, but hopefully it gets at the biggest issue.

Repetition

Instead of recalculating things and calling getInvestedPoints() repeatedly (slow), you can store the values in an array.

public void cmdSubtractPoint(int index, SkillTree tree) {

    boolean subtract = false;
    int investedPoints = this.blueTree.get(index).getInvestedPoints();

    int[] numInvested = new int[7];
    for (int i = 0; i < numInvested.Length; i++ ) {
        numInvested[i] = this.blueTree.get(i).getInvestedPoints();
    }

    if (index >= 0 && index < 2) {
        if (numInvested[0] + numInvested[1]...)
    } else if (index < 4) {
        ...
    } else if (index < 6) {
        ...
    } else if (index == 6) {
        ...
    }

    if (subtract) {
        ...
    }
}

Also, rather than adding things every single time (e.g. get(0).getInvestedPoints + get(1).getInvestedPoints()...), you can have a single variable serve as the value and alter that based upon if the first test fails.

int bottomBounded = numInvested[0] + numInvested[1];
int upperBounded = numInvested[2] + numInvested[3] + 
                   numInvested[4] + numInvested[5] + numInvested[6];
if (bottomBounded - 1 < 5 && upperBounded != 0) {
    this.showToast();
} else {
    // Alter bottomBounded and upperBounded
    if (...)
}

Take this with a grain of salt, because it could lead to some nasty nested if-statements. If you know of a better way to rewrite this that doesn't involve nesting statements, go ahead and do it.

These are just a couple of spots that could be worked on. There are more, but given the structure of your code trying to do a lot more might make it difficult to read.

Smaller Things

  • In the code I posted above, you'll notice I replaced things with a single less-than condition rather than two checks for equality. It's a small thing and not too big a deal, but in the future you can take advantage of the fact that else if implies mutual exclusivity between its conditions; that is, only one can be true.
  • Don't repeat yourself--when index == 0 || index == 1, there are 2 possible outcomes: this.showToast() and subtract = true. See if you can rewrite your code so that you don't repeat this.showToast().
  • Try to keep your lines < 80 characters long; it helps with readability so people don't have to scroll to see the rest of a line of code.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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