Re: [lmi] [lmi-commits] master 783f4dd 5/9: Consolidate and revise documentation

[Vadim Zeitlin]

On Sat, 28 Apr 2018 01:22:34 +0000 Greg Chicares <address@hidden> wrote:

GC> I have a 25x99 editor screen, so:

 I can see how reading code could be a problem with so little of it being
seen at once. Would using a bigger monitor, or two of them in the "span"
mode, be an option? Because I really think that, completely independently
of this thread and its conclusions, this would significantly increase both
your comfort and productivity. The above is hardly more than I can fit on
my phone screen and I wouldn't dream of working all day long on a phone (I
can use it to make an urgent trivial change, perhaps, but that's all).

GC> Okay, I've tried to do that for class wx_table_generator::column_info
GC> in commit c9f958f6cb904.

 Sorry, I don't see any such commit. Is it not pushed yet or did you mean a
different commit perhaps?

GC> Every time I open 'wx_table_generator.cpp' I go back to this nested class
GC> to refresh my still-evolving understanding.

 I think we have a real problem with this class and no amount of
documentation is going to change this because there must be something
fundamentally very wrong with it to require spending so much time and
effort on something so trivial. I don't know how to solve it though because
while I see that this class might not be very elegant, I don't see how to
avoid it considering the requirements we have for the different tables in
the group quotes and the illustrations and, most of all, I just don't see
any complexity in it: it's just information about a column table, including
its title, width and whether this width is computed dynamically or is
fixed.

 Sometimes I think that maybe all my attempts to explain it only make
things worse because it's really that simple.

 What I really wish to know is what would you do instead. I.e. clearly
wx_table_generator needs to know about its columns. So what would you use
to store this information if not something similar to column_info?


GC> > I.e. for me there is always one comment describing the function interface
GC> > from a public, external point of view and 0 to N comments inside its body
GC> > for things that are important internally but don't affect the public
GC> > contract.
GC> 
GC> ...so, for example, here:
GC> 
GC>     // Return true if this column should be centered, rather than
GC>     // left-aligned. Notice that this is ignored for globally right-aligned
GC>     // tables.
GC>     bool is_centered() const
GC>     {
GC>         // Fixed width columns are centered by default, variable width ones
GC>         // are not as long strings look better with the default left
GC>         // alignment.
GC>         return !is_variable_width_;
GC>     }
[...]
GC> Maybe this isn't a good example--I don't really see a separation of 
interface
GC> from implementation in those two comments.

 I do: the comment before the function describes what it does from the
caller point of view. The comment inside it describes how it does it. If
tomorrow we decided to add a separate rule saying that all columns whose
headers contain more than 2 words need to be centered (no, I don't know why
would we do it, but then, why not), only the latter comment would need to
change, but not the former one.

GC> But I already understand what the comments say. What I don't understand
GC> is how the various concepts in this class work together to form a
GC> single whole.

 They don't really form anything. This class is a collection of column
attributes and accessors for them. It doesn't actually do anything. Maybe
it should have a been struct to make this more clear.

GC> Distilling the class definition down this way now leads me to a question
GC> that I hadn't thought to ask before:
GC> 
GC>     bool is_variable_width() const {return  is_variable_width_;}
GC>     bool needs_clipping()    const {return  is_variable_width_;}
GC> 
GC> Aren't VariableWidth and NeedsClipping two facets of a single concept?

 I don't think so. Of course, right now they obviously are, but I don't see
why we couldn't decide to clip contents of a fixed width column in the
future.

 I also think that the code using needs_clipping() is more self-documenting
than the code using is_variable_width().

 Finally, to avoid this particular problem, we could just remove
needs_clipping() and always clip the output. This would have 2 minor
drawbacks: first, not clipping is more efficient, so I wanted to avoid
doing it needlessly and, second, clipping fixed width columns would hide
the problem if their contents doesn't fit, instead of making them clearly
visible. However the latter could be seen as an advantage too and I didn't
measure the overhead of the former, so maybe it's not really significant.

GC> AIUI, a column with this property has no predetermined width, expands to
GC> occupy free space not wanted by other columns, and is clipped if that
GC> expanded width is insufficient to show all its contents.

 This establishes the implication in one direction: all variable width
columns should be clipped. It doesn't show that fixed width columns can't
be clipped.

 Regards,
VZ