[Gnucash-changes] r13248 - gnucash/trunk/src/calculation - Fix Bug 107876 - financial calculator would call exit(1) if calculation resulted in an interest rate of zero.

[Chris Shoemaker]

On Sun, Feb 12, 2006 at 07:12:46PM -0500, Neil Williams wrote:
> Author: codehelp
> Date: 2006-02-12 19:12:45 -0500 (Sun, 12 Feb 2006)
> New Revision: 13248
> Trac: http://svn.gnucash.org/trac/changeset/13248
> 
> Modified:
>    gnucash/trunk/src/calculation/fin.c
> Log:
> Fix Bug 107876 - financial calculator would call exit(1) if calculation resulted in an interest rate of zero.

Neil,
        The final result here is still not ideal.  I think the
code-churn here may be making it a bit difficult for you (and everyone
else) to see what's going on.  Here's what I'd recommend:

1) Go back to the original state from r8223.  

2) Make the smallest change sufficient to fix the
"exits-unnecessarily" bug, which is:
   a) replace "exit(1)" with return 0.0 and
   b) add "if (BB == 0.0) return 0.0;" to the payment calculation to
avoid div-by-zero.

3) Commit that change.  That's one bug fixed.  True, it doesn't fix
every bug, but one-thing-at-a-time.

4) If you want to fix more bugs, like the
"infinite-loop-for-impossible-calculation" bug, (which is just as
severe as exiting unnecessarily, but completely unrelated), then begin
by writing some test cases so that you know which calculations are
causing the problem.  (BTW, your code is catching one special case of
this bug, but it's not the right fix.)

5) Once you've got some test cases, (hopefully lots passing and a few
failing, or at least failing to complete), then you'll probably see
the right way to fix that bug as well.  That fix will probably involve
detecting the impossibility of the calculation based on the relative
values of the inputs, but not avoiding calculations that for
special-cases of input (like zero) for which the calculation is valid.

6) Re-run the test cases to verify that the fix works, and that you
haven't accidentally changed the calculation (like rounding) for
working cases.

Here are a couple general comments:

1) You shouldn't be using g_return{_val}_if_fail() unless the the
condition you want to assert is invariant for all correct usages.
I.e. You're asserting that if the condition fails it's a BUG and you
want glib to throw a CRITICAL error, which under some conditions will
close the program immediately.  (If you want to throw a critical error
based on what you want to say are invalid inputs, then you need to
describe the inputs that will be considered invalid at the point of
the public API, and in terms of the public API's inputs.)

2) If you determine that some calculations will be ill-conditioned for
zero values, then they are probably also ill-conditioned for near-zero
values.  In those cases, you should not use "if (d)", "if (d == 0)", nor
"if (d == 0.0)", but rather "if (fabs(d) < tol)".

3) Don't be caught in the psychological trap of always wanting to move
forward with something that "isn't-quite-right".  There's _no_ problem
with making a change, realizing it's wrong, and then backing up to
provide a completely different solution to the problem.  Many times
I've seen absolutely atrocious code result from this disease of "must
not revert, it _almost_ works, just one more tweak".  It just takes
experience (and people pointing it out to you) to recognize this
pitfall and avoid it.

Good luck, and thanks for working on this bug.

-chris