Re: [lmi] Currency class

[Vadim Zeitlin]

On Wed, 16 Sep 2020 21:00:42 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 2020-08-14 15:43, Vadim Zeitlin wrote:
GC> > On Tue, 11 Aug 2020 18:08:44 -0400 (EDT) Greg Chicares 
<gchicares@sbcglobal.net> wrote:
GC> > 
GC> > GC> branch: valyuta/000
GC> > GC> commit e9efb62472281faac7965276adc1eb148c20fc4a
GC> > GC> Author: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> > GC> Commit: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> > GC> 
GC> > GC>     See 'README.branch'
GC> > 
GC> >  I'm not sure if I'm even supposed to look at it already
GC> 
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.

 Let me start with the currency class itself. I don't have any real
comments here, i.e. I agree with the various comments in the header
explaining why things were done (or not done) this way, so I only have a
few nitpicks:


- 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?

- 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.

- 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.

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


 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};

  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, but I don't really see any benefit in repeating "currency" twice,
  it's, IMO, sufficiently explicit even if it occurs only once.


- 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.


- 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?


- 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?


- 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.


 I've actually had other comments while reading the commits in order of
their appearance, but about half of them got addressed in the latest
version, so this email is much shorter than it would have originally been,
thanks for fixing all the points that I don't mention any more!

 BTW, I know that you're not very enthusiastic, putting it mildly, about
web-based code review systems, but I really think it would have been more
convenient if all the comments above could be attached directly to the
particular code lines, as in any such system. I won't insist, but if you
ever would like to test using something like this, my offer to install it
here for you/us to you use still stands, so please let me know if you ever
change your mind.

VZ

pgp0rG483p_KM.pgp
Description: PGP signature