|
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
0001-Don-t-return-elapsed-time-from-pre_-emit_ledger.patch
Description: Text document
0002-Don-t-combine-different-mcenum_emission-elements-tog.patch
Description: Text document
0003-Use-ledger_emitter-class-instead-of-pre_-emit_ledger.patch
Description: Text document
0004-Add-ledger_emitter-end-method.patch
Description: Text document
[Prev in Thread] | Current Thread | [Next in Thread] |