Re: [lmi] Preliminary emit_ledger code refactoring

[Vadim Zeitlin]

On Tue, 04 Aug 2015 19:23:59 +0000 Greg Chicares <address@hidden> wrote:

GC> I don't want to inconvenience you. Tell you what. Don't merge my changes
GC> yet. Just finish (once specifications stabilize) the work you're doing
GC> elsewhere to generate PDF files, and hand it to me with the class derived
GC> from ledger_emitter_single_output_base that you would have added to
GC> 'emit_ledger.cpp'...and I'll glue it all together.

 I really think it would be better if I glued it all. I have no doubt that
you can do it, but without using git merge capabilities it's bound to be
more difficult for you. It doesn't bother me to wait as, first, I still
have the other project (table utilities) to finish and, second, to the best
of my knowledge I already did everything I could with the current
specifications, so I'm also waiting for their final form to do the rest.

GC> >  In particular I wonder about
GC> > 
GC> >   /// Each member function (except the lightweight ctor and dtor)
GC> >   /// returns time spent, which is almost always wanted.
GC> > 
GC> > comment. Does it really mean that you are not going to apply the patch
GC> > refactoring the timers neither? For me this one seemed to be even more
GC> > obviously the right thing to do as the existing code could be used as a
GC> > nice example of why separation of concerns is a good thing to have, so I'd
GC> > be rather surprised if you didn't agree with it. But then, of course, I'm
GC> > already surprised, so this is not a good argument neither...
GC> 
GC> Here I guess we'll have to agree to disagree. Suppose I have a function
GC>   void calculate();
GC> (that does exactly one job), and I always want that calculation to be
GC> instrumented...so I write
GC> 
GC>   double calculate_with_instrumentation()
GC>     {
GC>     start-timer
GC>     call calculate() [which becomes non-public]
GC>     stop-timer
GC>     return timing
GC>     }
GC> 
GC> and use that instead of calculate() everywhere.
GC> 
GC> Pros:
GC>   less code to write everywhere the function is called
GC>   can't forget to write instrumentation for each call
GC>   unifies two concerns that are intimately linked
GC> Cons:
GC>   combines two concerns that might conceivably be separated
GC> 
GC> For me, the "Pros" win, hands down.

 I don't know if it makes sense to argue, but for me this is not the case
at all as I see:

Pros:
 - A tiny bit (a single simple statement) code less to write.

Not pros:
 - Can't forget to write instrumentation: you can still forget to use the
   returned value and I'd actually argue that it's much simpler to forget
   to output (or otherwise use) the time than if you have a timer right in
   front of you.
 - Unifies two concerns that are linked: no, they're really not, timing can
   be applied to just about any operation, and saying that it's linked to
   everything is exactly equivalent to saying that it's not linked to
   anything.

Cons:
 - Either mixing unrelated things in a single function or duplicating the
   number of functions (which was just deemed to be a big problem in
   another context!) if you split them in a public and private halves.
 - Lack of clarity in the code using the function: it's absolutely not
   obvious that a function frobnicate_widget() returns the time taken while
   it's blinding obviously that starting a timer before calling it and
   getting its time afterwards does the obvious thing.
 - Extra burden on the maintainer: whenever a new function is added you
   need to think about whether it should return the time taken by it or
   not.
 - Somewhat related: this can't be generalized to non-void functions,
   unless you're ready to return pairs or tuples or do something even more
   horrible to accommodate both the natural function return value and the
   time taken by it.
 - Lack of flexibility: if you need to compute the total time taken by two
   function calls you need to sum them up together manually instead of just
   creating the timer before calling the first one and getting its value
   after calling the second one.

 I think it's the combination of the first two cons that bothers me so
much. The code really becomes confusing to read, when looking at the
function itself you have no idea why does it need to measure the time taken
by it when it seems to have nothing to do with the function job (and,
indeed, it doesn't!) and when calling it you have no idea what does it
return because this is so unexpected from the function semantics.

 Honestly, I think you're not seeing it only because you're used to it by
now, but this is very confusing to the first time reader of this code.

 Regards,
VZ