lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[lmi] Preliminary emit_ledger code refactoring


From: Vadim Zeitlin
Subject: [lmi] Preliminary emit_ledger code refactoring
Date: Thu, 16 Jul 2015 22:41:55 +0200

 Hello,

 Here are some preliminary patches which don't do anything on their own but
which the group premium quotes code builds on. They are needed because the
PDF generation can't be done in a single pass, if only because of the page
numbers (but also for a few other reasons), and so the code needs to keep
its state between the calls to emit_ledger(). It also needs to know when
the last ledger is added, and while it could be done by adding a
post_emit_ledger() function or even just supposing that the call to
emit_ledger() with a composite ledger indicates the end of the generation,
adding a class with a separate end() method looked like the most natural
solution to me.

 Let me reiterate that these patches are not supposed to and, to the best
of my knowledge, do not change anything. I've tested the related GUI
functionality and also ran the unit tests which still pass.

 I hope that the commit messages explain the changes, here they are once
again (they're also part of the patch files) with some extra notes:

----------------------------------------------------------------------------
        Don't return elapsed time from [pre_]emit_ledger().

        Instead, compute the time used by it using a Timer object in the caller.

        Returning the time taken is completely orthogonal to the main task of 
the
        [pre_]emit_ledger() function, so don't do it in the function itself to 
respect
        the principle of single responsibility.

        This will also make upcoming refactoring of these functions simpler.

This patch makes the changes in the main patch of this series (#3) much
simpler, as the new methods introduced there can be just void instead of
having to return the time taken by them via a chain of calls to several
functions. It is also, IMO, much more natural to just measure the time
taken by the function execution instead of forcing each function to measure
it itself, it looks like a poster child example for the benefits of
separation of concerns.

As an aside, this is something that could be made much simpler with C++11
as it would be trivial to write a generic measure_time() template function
taking the function to call and its arguments using the variadic templates.
Unfortunately we can't do this yet...

----------------------------------------------------------------------------
        Don't combine different mcenum_emission elements together.

        Just call emit_ledger() twice instead, this has exactly the same effect 
as
        calling it with the combination of the two emission values but is more 
clear,
        especially as the second call is optional, and paves the way to making
        mcenum_emission a normal enum, and not a bit mask one, in the future.

This patch is not required by the group premium changes and so could be
omitted, especially if nothing is changed in how mcenum_emission enum is
used (see http://lists.nongnu.org/archive/html/lmi/2015-07/msg00000.html
for the concerns I have about it), but I believe the change is worth
applying because the new code is IMHO more clear. And if we do stop using
bit masks for the different emissions, it would be one less place to
update.

----------------------------------------------------------------------------
        Use ledger_emitter class instead of [pre_]emit_ledger() functions.

        Use of the class allows to preserve internal state between the calls to
        pre_emit_ledger() and emit_ledger() which will be important for the 
premiums
        PDF emission to be added soon.

        Preserve emit_ledger() function for compatibility and ease of use when 
only a
        single ledger is being emitted, but use the new class in 
group_values.cpp code.

This is the main part of the changes. I'm not sure about the need to
preserve emit_ledger() function, I mostly did in order to keep the changes
as minimal as possible but I think it might be better to switch to using
the class directly everywhere.

I also hesitated for some time between having a single add_ledger() taking
either non-composite or composite ledgers, having two methods add_ledger()
and add_composite() as I ended up by doing or having add_ledger() for the
non-composite ones and pass the composite ledger to end() (which would be
renamed to end_with_composite() or something like this). While I think I
understand the difference between non-composite and composite ledgers, I'm
still not sure what is the most natural way to use them, so if I chose
wrongly, please let me know.

----------------------------------------------------------------------------
        Add ledger_emitter::end() method.

        This is not used yet, but will allow to finish generating the output 
for the
        emission formats requiring this.

If you prefer, you could delay applying this patch until the patch with the
group premium generation code, which uses this end() method, itself is
submitted, then you could apply them together and avoid having an unused
method at this stage. But personally I prefer to keep the commits as atomic
as possible and, formally speaking, this commit is independent of the ones
to follow.
----------------------------------------------------------------------------

 As always, please let me know if you have any questions or comments about
these patches. Also, I plen to wait until they are applied (or discarded
and redone) before submitting the changes for the group premium generation
code itself, as it depends on them, but please let me know if I should
submit the latter changes sooner, e.g. if you'd like to already have a
preliminary look at them.

 Thanks in advance,
VZ

Attachment: 0001-Don-t-return-elapsed-time-from-pre_-emit_ledger.patch
Description: Text document

Attachment: 0002-Don-t-combine-different-mcenum_emission-elements-tog.patch
Description: Text document

Attachment: 0003-Use-ledger_emitter-class-instead-of-pre_-emit_ledger.patch
Description: Text document

Attachment: 0004-Add-ledger_emitter-end-method.patch
Description: Text document


reply via email to

[Prev in Thread] Current Thread [Next in Thread]