Re: [lmi] Control flow in page_with_tabular_report::render()

[Greg Chicares]

On 2018-02-12 18:47, Vadim Zeitlin wrote:
> On Sun, 11 Feb 2018 20:23:36 +0000 Greg Chicares <address@hidden> wrote:

[...snip concrete proposal...]

>  I think representing the outputs of this algorithm as something the code
> using it could iterate over is indeed a good idea,

Up to this point we seem to agree.

> i.e. I'd make it return
> a collection (so vector, in C++) of pages and each page would provide an
> iterator over either groups (but then we'd need to pass it the number of
> rows in the group, which could be less than maximum) or rows (but then we'd
> need to have a way to distinguish between blank and non-blank rows).

I prefer to use the subset of C that looks like, say, ansi C, fortran,
or algol, for problems that can be easily solved in that subset, because
that produces the most comprehensible solution. I'm already apprehensive
when you speak of providing an iterator, because I don't see that as
necessary.

Suppose we're printing the third page, and we know it has four groups
of five rows each, with spaces between them: nineteen data rows that
fill twenty-two lines. It took only a few minutes to write this:

#include <iostream>
#include <string>
#include <vector>

void write_line(std::string const& s) {std::cout << s << std::endl;}

void test()
{
  std::vector<std::string> data =
    
{"A","B","C","D","E","F","G","H","I","J","K","L","M","N","O","P","Q","R","S","T"};

  // Information from pagination class:
  int n_groups       =  4; // counted by 'h'
  int rows_per_group =  5; // counted by 'i'
  int n_data_rows    = 19; // counted by 'j'
  int n_output_lines = 22; // counted by 'k'

  int j = 0;
  int k = 0;

  for(int h = 0; h < n_groups; ++h)
    {
    for(int i = 0; i < rows_per_group; ++i)
      {
      if(j < n_data_rows) {write_line(data[j]); ++j; ++k; }
      }
    if  (j < n_data_rows) {write_line(" ");          ++k; }
    }

  std::cout << std::endl;
  std::cout << j << " should equal " << n_data_rows    << std::endl;
  std::cout << k << " should equal " << n_output_lines << std::endl;
}

It's just a handful of lines of code, all self-contained.
No fancy design patterns needed. No iterators. No lambdas.

>  However any such approach would have 2 fundamental (AFAICS) problems:
> first, the code using it would be more complicated than currently because
> we need to render the fixed part of the page in order to compute its height
> and feed it to this algorithm so that it could have the number of rows per
> page, yet we also need to do it during iteration. This just can't be not
> cumbersome (a bit like this sentence).

int P = get_page_height();
int H = get_header_height();
int F = get_footer_height();
int available_output_rows = P - H - F;

Where's the difficulty? I'm assuming that the header and footer sections
are the same (except for the page number) for all pages across which a
data-table is spread, so that H and F above are constant; and that the
functions that write the header and footer can be called with a
"measure_only" argument to calculate their sizes before rendering them.

>  Second, this completely separates the handling of the row and its vertical
> position, as one of them is (implicitly, now) in the general part, while
> the other one is still in PDF code. I find this problematic because they
> must be in sync, and while it should be simple to notice if this is not the
> case, it's still not automatic and I don't like that.

The invariant that they remain in sync could be asserted at every step.
Once the code is debugged, how could such assertions ever possible fail?

>  By contrast, in a "template method" approach, we'd have some paginator
> class with virtual methods (or callbacks, for which we could use any
> callable, such as std::function<> or a lambda, if you prefer) that would be
> called when
> 
> 1. The new page starts, and return the size of the fixed part (and render
>    it if necessary).
> 2. A row is output, and return the height of the row (and render it). Do
>    nota that this method is called with the vertical position of the row
>    that would be updated by paginator itself.
> 3. A row is skipped (i.e. a blank row is output): this could be merged
>    with (2) if desired, but I think it's nicer to keep it separate.

This idea intertwines the pieces that I'm endeavoring to separate, namely:
  (1) calculate page-layout parameters like h,i,j,k above
  (2) use the result of (1) to render the data
My thesis is that:
  - they can be separated
  - each of the two separate pieces is small, simple, and cohesive
  - the interface between (1) and (2) is just a struct containing four ints
and the clearest way to write that is in a small C subset of C++.

>  This would still allow us to keep the algorithm entirely generic and
> easily testable; make the code using it very simple and without any loops
> at all; keep the row index and vertical position in sync automatically.
> It wouldn't make the data structures apparent, which is, again, a bad thing
> in theory, but I have trouble convincing myself that this is really the
> case in this particular example, where the data structures are so simple,
> while the code is not, and making the data structures explicit complicates
> the code rather than simplifies it.
> 
>  So what do you think of this approach?

I'd say lambdas, function<>, and "design patterns" are unnecessary
impediments to comprehension here: whatever can be written well and
clearly in the C subset of C++ is simplest and clearest when written
that way, and introducing unnecessarily fancy techniques can only
make it less clear.

> GC> What I'm trying to say is: could we please just call B::foo()
> GC> instead of C::foo(), because it's much easier for me to read
> GC> and I don't think we'll ever want to implement a C::foo()?
> 
>  This is completely unrelated to the rest of this message and is a much
> more narrow, and so less important, issue, but I'd still like to re-state
> that I strongly disagree with the proposal above.
Okay, I understand, and I won't change that; I may simply add a
comment explaining this sometime.