Re: [lmi] Currency class

[Greg Chicares]

On 2020-09-19 21:57, Vadim Zeitlin wrote:
> On Wed, 16 Sep 2020 21:00:42 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
[...]
> GC> I've pushed a new valyuta/002 9433c8e2 to origin, and
> GC> would appreciate your comments.
> 
>  I can't comment about the performance issues yet, but here are some
> comment/questions from reading the code.

Thanks.

>  Let me start with the currency class itself.
[...]
> - Hack with the unused bool argument to ctor taking data_type could be
>   made more readable by using "struct from_data_type_tag {};" and then
>   passing "from_data_type_tag{}" instead of "true". Ideal would be to get
>   rid of this ctor completely, or at least not make it public -- I don't
>   see why do we need it, except in the unary operator-() implementation?

Done: commit d9d16ef3ca06. As that commit shows, it's used in four
other files as well, most importantly in 'round_to.hpp', so that
a value like 3.14159 can be rounded to 3.14 and returned either as
3.13999999999999770823 dollars, or as exactly 314 cents.

> - One of the changes I'm very much looking forward to in C++20 is the
>   new operator<=>() which could be used to replace the 6 relational
>   operators we currently have. I don't know if we're ready to switch to
>   C++20 (and hence g++ 10) yet, but this is definitely much nicer than
>   writing out all the permutations.

Agreed. Commit message for 4b313b0e:
  "Maybe C++20's operator<=> will replace this anyway, someday."
For the moment, we're still building for release with the latest
official MinGW-w64 gcc, which is only version eight.

> - I thought currency(0) was fine if a bit long, but currency{} is IMO much
>   less readable. I think it would be worth defining "_c" literal suffix, as
>   mentioned in the README and using "0_c" instead.

See
  commit d7fcd483b Write zero cents more tersely
The readability of "currency{}" seems to be a matter of opinion.
Having made changes like this:
-    return round_minutiae().c(z * std::max(currency(), TotalAccountValue()));
+    return round_minutiae().c(z * std::max({}, TotalAccountValue()));
I now find even "std::max({}, some_variable)" perfectly clear; and it
has the great advantage that I can change the type of 'some_variable'
without having to change "{}". In that way, it's like 'auto'; adherents
of the "auto everywhere" school of thought logically must like this too.

I did balk at string literals, though, as explained in that
soon-to-be-pushed commit:

+/// Zero cents--akin to a user-defined literal.
+///
+/// UDLs seem less convenient because the obvious "0_c" is likely to
+/// collide with some other UDL, and "currency::0_c" is too verbose.
+/// "0_cents" may avoid both those problems, but "C0" is terser.
+/// "C0" is chosen instead of "c0" only for the pixilated reason that
+/// the capital letter looks kind of like a "0".
+
+inline constexpr currency C0 = {};

> - I'd definitely get rid of the "m" accessor.

It's a tradeoff. I wanted operator==() and operator<() to be
non-member non-friends, and a public 'm() const' seemed an
acceptable price to pay.

It's generally unreasonable to ask an object to show the raw
value of a private member. But it's not unreasonable to ask
a currency object how many cents it represents. Would your
objection be well answered by a s/\<m(/cents/g renaming?

>  The rest of comments in no particular order (i.e. in order of "git diff"):
> 
> - In many places we have variable initializations of the form
> 
>     currency foo = currency(expr);
> 
>   which could be simplified to
> 
>     currency foo{expr};

Done: commit 809d422ca. I left this old way of writing currency(0):
  currency a2 = currency() - a1;
in the unit test, followed by an alternative I prefer. Otherwise,
  grep 'currency.*=.*currency' *.?pp
finds only definitions of operators.

>   Going in the other direction, some people would recommend writing this as
> 
>     auto foo = currency(expr);
> 
>   which could work too, if we/you want to favour this "auto-almost-always"
>   style

Changing the type of so many variables from 'double' to 'currency'
made me think of that again. The benefit is that in
  :%s/double/currency/gc
the '/c' part (ensuring each change is correct) is a lot of work
much of which would be unnecessary with 'auto' nearly everywhere.
The disadvantage is that it's harder to discern a variable's type.
As a corollary, if we change
- double foo() {...}
+ currency foo() {...}
then an expression like
  auto bar = foo() * 13.0 + quux();
might break in some way that's hard to see.

But if we break things only in a branch, then reimplement cleanly
in 'master', we can cleave the 'master' commit into
 - phase one:
     s/double/auto/  where we can
 - phase two:
     s/double/currency/  only where we need to
Phase one is pretty easy to do reliably, and to test. Separating
it makes the more difficult part smaller.

> - In AccountValue::TxDoMlyDed() we have the expression
> 
>     currency(doubleize(MlyDed) * MonthsToNextModalPmtDate())
> 
>   which seems to be needlessly complicated, as we define the overloaded
>   operator*(currency, int) anyhow, so it looks like just
> 
>     MlyDed * MonthsToNextModalPmtDate()
> 
>   should be enough.

commit c95db3dd0d8630

> - Almost all uses of doubleize() are in material_difference() argument
>   list. My first idea was to provide materially_equal() overload taking
>   currency directly. My second, and I think, better, idea was to remove
>   calls to it entirely as 2 currency objects can't be materially equal if
>   they're different, i.e. it looks like we could just use the normal
>   difference for currencies instead. But there is a somewhat cryptic
>   "um...yes, it is" comment in ihs_acctval.cpp implying that we can't do
>   this, so I must be missing something here -- but what?

IIRC, I replaced that occurrence with an "==" comparison, and then
found that I had broken something. But maybe that was back when
I was experimenting with
  using currency = double;
which of course can't work now with all the d() calls.

BTW, those d() calls are like painting vermin with a fluorescent dye.
The goal isn't just to make them visible, but to eradicate them.
I would hope that the eventual 'master' will have a lot fewer d()'s.

Anyway, some floating-point differences are immaterial:
  assert(1.0 == 3.0 * 1.0/3.0);
but no integral currency difference is material. However, I'm
also thinking about defining 'currency' as a floating-valued
class that's scaled by 100, so that it can hold e.g. $1.23
as the exact integral value 123¢, but allowing it to assume
non-integral values; in that case, material equality still
matters.

> - Having to specify "<double>" explicitly after std::{min,max} looks a
>   bit ugly, but I'm not sure what to do about it. Adding some methods like
>   at_most() or at_least() returning double to currency class would obviate
>   the need to do it, but would this really make the code more readable?

commit 9ea1df7de6c and commit a32e29bb3cb

> - There is a "should there be currency::operator*=(int)?" comment in
>   ihs_avstrtgy.cpp which seems to be outdated because there is such an
>   operator in currency class.

commit 3d1ff11acb2b5

>  BTW, I know that you're not very enthusiastic, putting it mildly, about
> web-based code review systems
Too much like social media. Email suffices. With email,
I own a local copy of the content, "own" and "local"
being the key words. Email is to a web-based code-review
system as git is to svn, except that svn doesn't track
you and serves no ads.

If you want to experiment with another idea, consider pushing
throwaway "comment" branches to savannah origin. E.g.:

--8<----8<----8<----8<----8<----8<----8<----8<----8<--
diff --git a/ihs_avstrtgy.cpp b/ihs_avstrtgy.cpp
index fb3197fa..41f81d8e 100644
--- a/ihs_avstrtgy.cpp
+++ b/ihs_avstrtgy.cpp
@@ -63,14 +63,12 @@ currency AccountValue::CalculateSpecAmtFromStrategy
     ,mcenum_sa_strategy strategy
     ) const
 {
// This seems to be outdated because there is such an
// operator in currency class.
-    currency annualized_pmt = currency
-        // should there be currency::operator*=(int)?
-        (
+    currency annualized_pmt =
-->8---->8---->8---->8---->8---->8---->8---->8---->8--

But once they go stale, they probably become unuseful.
Still, if you like that much better, I'm willing to try.