3 possible cases for n: > 0, < 0 and == 0
Skip everything if == 0
reserve string memory for every valid Money values
Add sign first to avoid memory copy that inserting it at the end incur.
Remove a superfluous counter: nTrim
Eliminate 2 logical expression evaluations.
Signed-off-by: Olivier Langlois olivier@olivierlanglois.net
3 possible cases for n: > 0, < 0 and == 0
Skip everything if == 0
reserve string memory for every valid Money values
Add sign first to avoid memory copy that inserting it at the end incur.
Remove a superfluous counter: nTrim
Eliminate 2 logical expression evaluations.
Signed-off-by: Olivier Langlois <olivier@olivierlanglois.net>
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f38e39df7f009d9d229b14447a2644db65c3d183 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
388 | @@ -389,22 +389,36 @@ string FormatMoney(int64 n, bool fPlus) 389 | { 390 | // Note: not using straight sprintf here because we do NOT want 391 | // localized number formatting. 392 | - int64 n_abs = (n > 0 ? n : -n); 393 | + if (!n)
Is this special case needed? Or can it simply be handled by the if (n>=0) case?
not needed. It could be removed if desired and and this case be treated generically by the rest of the code. That being said, it must be discriminated anyway when handling the sign. Cannot prefix '+' or '-' to it. So why not leverage this to skip the whole processing.
Yes, the sign must be discriminated anyway, but for readability and flexibility it's best to have as little special cases as possible.
My main gripe here is that you're making the code longer instead of shorter. Have you benchmarked the improvement in any way?
That is really the sort of code I don't like to read, before AND after ^^.
laanwj,
what metric are you using to make the statement that the code is longer?
Line code count may be misleading. Out of the 26 new lines, 9 are empty lines or lines only having brackets. Also consider that previous statement:
int64 n_abs = (n > 0 ? n : -n);
is in fact a packed if else block.
fair enough. I'll produce a small benchmark program and report back the result here.
I find the old code easier to read, and ease of reading is more important than performance. Especially for a routine like this, which I strongly doubt is on any critical code paths.
Ok. it is not a good pull request. Hell is paved with good intentions.
I have benchmarked my modifs and the results are almost identical between old code and proposal.
Any memcpy avoided by new code is offseted possibly by the fact it has now 2 string object. local str and strprintf return value whereas the old code benefit from RVO (Return Value Optimization).
Thanks for your time guys. I'm learning and improving myself. I'll eventually come up with a good pull request!