Re: [lmi] Preliminary emit_ledger code refactoring

[Greg Chicares]

On 2015-07-29 14:14, Vadim Zeitlin wrote:
> On Wed, 29 Jul 2015 10:14:23 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> Maybe that's in the eye of the beholder. Maybe this is a more modern way 
> of
> GC> doing things, and I've simply fallen behind the times. Maybe this is 
> clearer
> GC> from a functional-programming point of view, while I'm clinging to a 1950s
> GC> imperative style and understand C control structures more readily than 
> class
> GC> hierarchies. Whatever the reason, I've tried pretty hard to like this,
> 
>  This is definitely not new and as purely OO (and not FP) as it gets, but
> the main thing is that it is not about liking, or disliking, it at all
> because the benefits are perfectly objective and not subjective: when
> adding a new emitter kind ("inforce bill"?), you would have had to add code
> specific to it in 3 different places with the old approach: to
> pre_emit_ledger(), emit_ledger() and post_emit_ledger() (the latter is
> needed when producing any kind of reports with a footer, so it's really
> non-negotiable). With the new design, you add a single class implementing a
> well-defined (and checked by compiler) set of pure virtual methods and you
> modify the code in exactly one place to map a bit in mcenum_emission to
> this class. With a chain of "if"s it was trivially easy to forget to update
> one of them and even if you did find all places to update, your code was
> scattered among them. With the class-based approach you have a clear view
> of what needs to be done and it is done in a single place, inside the same
> class. Don't you like this?
> 
> GC> but I'm stuck because, to me, this way seems harder to understand.
> 
>  Superficially the old code was simpler to follow, but it was a deceptive
> simplicity. Looking at the code in emit_ledger() you could easily forget
> that it relied on something done in pre_emit_ledger() a long way away from
> the code you were looking at. Looking at it now, you see everything related
> to the given emission kind in one place.

In effect, we have a 9 by 2 table of function calls, to which we're adding
new code that'll make it 10 by 3 (with 16 empty cells):

                       old-----------------+--------------new
                             begin  middle | end
    mce_emit_pdf_file                 A1   |
    mce_emit_pdf_to_printer           B1   |
    mce_emit_pdf_to_viewer            C1   |
    mce_emit_test_data                D1   |
    mce_emit_spreadsheet       E0     E1   |
    mce_emit_group_roster      F0     F1   |
    mce_emit_text_stream              G1   |
    mce_emit_custom_0                 H1   |
    mce_emit_custom_1                 I1   |
old                                        |
-------------------------------------------+
new
    group premium quote        J0     J1     J2  [must be stateful]

and we're discussing different ways of implementing it.

Up to now, the implementation has been a middle() function with a series
of if-statements, one for each row, along with a similarly written begin()
function that doesn't do anything except for rows E and F.

To accommodate the new last row's statefulness, we decided to turn those
two free functions into class members and add a new member end(). I've
done that, and nothing more so far, on 20150804T1558Z (revision 6231).

Next we'll need to add the stateful {J0,J1,J2}. Knowing that we may soon
enough want to add another row {K0,K1,K2}, should we recast the entire
implementation at this time, substituting polymorphism for conditionals?
The benefit is that we're optimizing the addition of a future {K0,K1,K2}:
we just write a derived class K, and everything automatically works--and
we don't need to go back and understand the code we're adding it to. The
cost is adding a class hierarchy, and a vector of dynamically-allocated
base-class objects along with corresponding iterators, which roughly
doubles the size of the code and, by focusing on each row, makes the
underlying table harder to perceive (at least for me).

On the other hand, I'm responsible for making this whole system work,
and for fixing it if it's broken, so my chief concern is making sure I
can understand everything in this file and how it works together with
everything else in lmi--for example, here:

  http://lists.nongnu.org/archive/html/lmi/2015-08/msg00000.html
| the "custom" emissions overwrote the input files.
| Fixed 20150803T1704Z, revision 6229.

where I found that a couple of little chunks of code had to be moved
out of one file's conditionals into another--so I had to understand
both files and how they work together.

Maybe it's just me, but I find conditionals easier to understand than a
class hierarchy. When I look at conditional-based begin(), middle(), and
end(), I immediately perceive the table of function calls above. If I
write a new {K0,K1,K2}, I'm not at all concerned about being able to
plug the right code into the right spots in those conditionals.

It may well be that polymorphism will be clearer if the code becomes
much more complex. The original implementation
  
http://svn.savannah.nongnu.org/viewvc/lmi/trunk/emit_ledger.cpp?revision=2967&root=lmi&view=markup
had only the middle column and four rows of the table above:
  {B1, D1, E1, G1}
and I wouldn't write that any other way than as a free function with
conditionals. Adding more rows doesn't increase complexity or call
for a different design. Later, it was necessary to add another column,
and now we need another; that makes it more complex, as does the new
statefulness requirement. At some point, the complexity may become so
great as to impede comprehensibility, calling for a new design. Have
we crossed that refactoring threshold yet? Today, my judgment is "no".
Tomorrow, when we actually add {J0,J1,J2}, I'm thinking "probably no",
but tomorrow's another day.

At any rate, thanks very much for showing a different way of making
this work.