Optimization possibilities with stacking if-statements?

Question

private String formatPrice(int price) {
    String p = "";
    if (price > 10000000) {
        p = " (" + ((double) Math.round(price / 100000) / 10) + "m)";
    } else if (price > 100000) {
        p = " (" + (price / 1000) + "k)";
    } else if (price > 1000) {
        p = " (" + ((double) Math.round(price / 100) / 10) + "k)";
    } else if (price > 0) {
        p = " (" + price + "gp)";
    }
    return p;
}

Is it possible to simplify this piece of code without slowing down performance too much? It doesn't look like it's been done properly.

Have you identified performance to be a problem here? How many times does this method get called in your program? — Oliver Charlesworth, Jul 23 '14 at 21:38
Little quirks aside, the code doesn't strike me as unreasonable. What exactly is the motivation for rewriting it? — NPE, Jul 23 '14 at 21:39
Performance isn't an issue. It is essentially called within a game loop up to 15 times per frame. @NPE I've always had this hatred towards stacked if-else-statements. It doesn't look right and I'm never sure if it's proper. — John, Jul 23 '14 at 21:41
So you should be optimizing it for *legibility*, not for *performance*. Or, if it works (and has a unit test), then just leave it alone ;) — Oliver Charlesworth, Jul 23 '14 at 21:42
This question appears to be off-topic because it is about code review (should be on http://codereview.stackexchange.com instead). — Oliver Charlesworth, Jul 23 '14 at 21:49
I apologize, I wasn't aware that existed. Will keep in mind for the future, thank you. — John, Jul 23 '14 at 21:52
`Math.round(price/100)` probably doesn't do what you think. The argument is **integer division**, so it will divide `price` by 100 to get an _integer_ result, and will truncate toward 0. Then it converts the `int` to a `float` for use by `Math.round`, but its value is still an integer. So `Math.round` just converts from `float` back to `int` without changing the mathematical value. Then you convert that to a `double`. I suspect this is not what you want. — ajb, Jul 23 '14 at 21:58
Truncation is my intention, yes. Essentially what I'm trying to do (and it does successfully) is convert the integer 22000 into "22k", or 39999999 into "39.9m" I checked stackexchange some and found this to be a good way to shorten decimals. However, I like Elliott Frisch's use of String.format to do that below. — John, Jul 23 '14 at 22:01
If you're truncating then `Math.round` is unnecesary. `((double)(price/1000))/10` will get you a value that is a multiple of `0.1`. Even so, it won't be an _exact_ multiple, so there's still a chance you will get something ending in `".299999999"` instead of `".3"`. You should definitely use `String.format` which will avoid that problem. — ajb, Jul 23 '14 at 22:07

score 1 · Accepted Answer · answered Jul 23 '14 at 21:47

1

Is it possible to simplify this piece of code without slowing down performance too much?

If I understand your question, yes! You could make the method static. You could also shorten it up significantly with String.format()

private static String formatPrice(int price) {
  if (price < 0) {
    return "";
  }
  if (price > 1000 * 1000) {
    return String.format("(%.1fm)", ((double) price) / (1000 * 1000));
  } else if (price > 1000) {
    return String.format("(%dk)", price / 1000);
  }
  return String.format("(%dgp)", price);
}

answered Jul 23 '14 at 21:47

Elliott Frisch

198,278
20
158
249

1

Cool, but this is rapidly turning into code review (in which case, this question should arguably be voted as off-topic in favour of http://codereview.stackexchange.com). – Oliver Charlesworth Jul 23 '14 at 21:49
This is done exactly how I like to resolve deep if/else if stacks. Instead of holding a return value and returning it at the end, return inside each if block. If necessary, extract a method which does that. – Carl Manaster Jul 23 '14 at 21:51
This looks great, never knew String.format could be so useful. Thank you. – John Jul 23 '14 at 21:53
What is fm? Op in his question has only `m`? – Damian Leszczyński - Vash Jul 23 '14 at 21:54
@Vash-DamianLeszczyński It's a floating point format. One decimal digit, followed by a `m` (for million). – Elliott Frisch Jul 23 '14 at 21:55
I don't think static is going to do much. private methods can't be overridden, so there's no lookup required to call them -- they use `invokespecial` at the bytecode and are fully resolved at compile-time. The only thing you'll save is pushing `this` onto the stack before the call and popping it back off afterwards, and those are super duper quick. – yshavit Jul 23 '14 at 21:59
Yep, naturally. ;-)... – Damian Leszczyński - Vash Jul 23 '14 at 22:01
@yshavit Fair enough, but when I see a `private` function that doesn't access any instance fields or methods... – Elliott Frisch Jul 23 '14 at 22:02
This algorithm is much slower than the one in the question. Running 100,000 iterations over random data the answer algorithm is slower in all cases (as much as 5x). String.format is relatively slow and should rarely be used when performance is paramount. – Jeff Jul 23 '14 at 23:35

score 0 · Answer 2 · edited May 23 '17 at 12:21

0

It looks ok for me. I don't see any big optimizations that can be done here. It is also quite clean. However, you might like to see alternative implementations of the same algorithm.

edited May 23 '17 at 12:21

Community

1
1

answered Jul 23 '14 at 21:44

Kao

7,225
9
41
65

Optimization possibilities with stacking if-statements?

2 Answers2