[lmi] Commentary on certain progress-meter changes

[Greg Chicares]

Not long ago we noticed that a progress meter didn't seem right in one
particular set of circumstances. In run_census_in_series::operator(),
the progress meter increments up to the number of input cells only,
excluding the composite. That's perfect for the "Census | Run case"
command, which produces the composite as a side effect that takes no
extra time per se. But now we contemplate adding a command to print
each cell along with the composite; in that case, the composite takes
at least as long to print as any cell. It would be trivial to increase
the limit by one (patch [1] below), but we resisted the temptation to
commit that: adding "1 + " to "fix" a symptom without careful analysis
is always a mistake. We hadn't considered other commands, or given any
thought to the parallel code in run_census_in_parallel::operator().
And we hadn't asked ourselves whether the progress meter would ever
increment to its limit--shouldn't that be a postcondition? and if we
enforced it, wouldn't it have told us to rethink this patch?

Validating postconditions is often a really good idea, and well worth
considering in this case. Here, though, rather than evaluating the
idea's merits, I want only to discuss how it was implemented in HEAD,
as an exercise in applying an "agile methodology". We've already
identified an anomaly
  http://extremeprogramming.org/rules/bugs.html
and come up with a spike solution, but
  http://extremeprogramming.org/rules/spike.html
| most spikes are not good enough to keep, so expect to throw it away.
We'll want to set up tests
  http://extremeprogramming.org/rules/testfirst.html
and we'll need to refactor often
  http://extremeprogramming.org/rules/refactor.html
in order to achieve the simplest code
  http://extremeprogramming.org/rules/simple.html
Agile craftsmanship boils down to this:

http://lists.nongnu.org/archive/html/lmi/2006-09/msg00003.html
|
| DoTheSimplestThingThatCouldPossiblyWork entails two (2) steps:
| first, implement the thing in a simple straightforward way;
| second, refactor the code to produce the simplest system
| including the new thing.

and we must heed this advice:

  http://c2.com/cgi/wiki?FixBrokenWindows
  http://www.codinghorror.com/blog/archives/000593.html
  http://butunclebob.com/ArticleS.MichaelFeathers.WorkingClean

in order to "produce the simplest system"--and simple is hard. But
this is the level of craftsmanship we must strive for. Consider the
all-too-common alternative: "make the smallest change that isn't
blatantly incorrect, and fix it only if someone complains". That
inevitably leads to a worse endgame--the system deteriorates until it
needs to be scrapped and rewritten completely from scratch every few
years, at which time many old defects are reinvented and new ones
added. That costs more while producing poorer results; and we'd rather
build something to be proud of, anyway.

With those preliminaries in mind, I present an extensively-annotated
'ChangeLog'. I've marked the few places where insurance expertise or
non-local knowledge of the system is required; aside from those parts,
this is pretty generic stuff. Although the work described spans three
days, I was often distracted by other matters, so it's more like one
full day's work in total; writing down the thinking behind each step
took longer than doing it.

See
  http://lists.nongnu.org/archive/html/lmi/2007-01/msg00011.html
for a similar discussion that illuminates similar principles.

20070708T1225Z <address@hidden> [1224]

  group_values.cpp
Refactor.

Associated with a progress meter is a loop whose body cries out for
simplification. This change makes it easier to see the nature of the
loop:

    // ...create progress meter...
    for(...;...;...)
        try
            if(...)
                continue;
            // ...do real work...
        catch(...) {...}
        meter->reflect_progress();
    // Now what can we assert about the loop counter?

which we need to bear in mind as we work on class progress_meter:
could 'continue' or 'try...catch' prevent the postcondition from
being satisfied?

This change requires some knowledge of insurance calculations and
longer-term evolutionary plans evident in recent work to consolidate
all calculations in a few high-level facilities. Although it isn't
strictly necessary for progress_meter changes, this is the best time
to do it; and it eliminates one marked defect.

BTW, there's another loop later in this same file that has a different
sort of complication:
    goto restart;
and we'd better pay careful attention to that when the time comes.
(A question for the thoughtful reader: did we?)

20070708T1413Z <address@hidden> [1224]

  group_values.cpp
Invert logic of cell_should_be_ignored() conditionals. This affects
only the progress meter, which now should count all the way up to its
maximum.

Compare patch [2] below, which applies this idea to one loop; this
change applies it to another one in the same file, too. It's necessary
to grep all the source to make sure we've covered every similar
instance, and right now is the best time to do that.

20070708T1948Z <address@hidden> [1223]

  group_values.cpp
Eliminate try...catch, revealing a design problem.

See Sutter and Alexandrescu, _C++ Coding Standards_, Item 62: "Using
catch(...) [except where idiomatically required] is often a sign of
bad design.... A good program doesn't have many catch-alls and,
indeed, not many try/catch statements at all".

This change, in combination with the preceding one, transforms the
first loop in this file from this (cited in the 20070708T1225Z
commentary above):

    // ...create progress meter...
    for(...;...;...)
        try
            if(...)
                continue;
            // ...do real work...
        catch(...) {...}
        meter->reflect_progress();

into this:

    // ...create progress meter...
    for(...;...;...)
        if(...)
            // ...do real work...
        meter->reflect_progress();

This is another change that requires knowledge of the evolutionary
plans for consolidating insurance calculations (and removes another
marked defect). Without it, however, it might be difficult to be sure
the intended progress_meter changes are correct; and if we aren't
highly confident that our changes are correct, then we mustn't commit
them. Thus, attacking an apparently-unrelated problem simplifies the
progress_meter task.

This change demands careful testing. Clearly the behavior is different
when an exception is thrown in the loop body: previously, the current
iteration ended, an error message was displayed, and the loop resumed;
now, the loop ends, and control passes to an upstream catch-clause.
Three try...catch constructs have been removed; each must be tested.

I tested this in the GUI, and the behavior seemed obviously correct to
me. I neglected to explain it to Kim (or anyone else), though, so she
spent extra time finding the behavior change and describing it to me;
I should have told her what to expect. As it turned out, she tested
this change with 'offer_hobsons_choice' in 'configurable_settings.xml'
set to '0', rather than '1' as I had been using: I hadn't realized
that the system is distributed with '0'. Consider the behavior with
'0' for a census with ten individuals, of which one has an error that
is trapped by an assertion: formerly, a composite comprising only the
nine valid individuals was created; now, no composite is created--and
we decided that this is preferable according to the principle of least
astonishment, especially because some users click away error messages
heedlessly, yet assume the system won't give them invalid output. This
experience offers several object lessons:

 - Don't lightly disregard the best textbooks' expert advice. Beware
   the catch-all idiom, which is appropriate only in certain well-
   defined situations. Removing an inappropriate use of that idiom
   here improved the program; might there be other inappropriate uses
   that should be removed? (That question is left to the thoughtful
   reader.) If we don't carry that work out now, then when will a
   more opportune occasion arise? Is our goal merely to finish the
   task at hand in the most expeditious way, or is it really to
   produce the best program we can?

 - Even a change we consider good will be perceived as a defect unless
   it's explained to users. Explaining that once, carefully and in
   advance, takes more time up front, but involves the least possible
   effort overall. Slow down, do it right the first time, and you'll
   reach the ultimate destination sooner.

 - Don't trust comments. Examine the diff: the block comment removed
   by this change did not accord with the behavior of the legacy
   system.

 - Mention substantive changes to testers. This doesn't mean writing
   a novel: it just means flagging deliberate behavior changes. If I
   seem to be writing a novel here, it's for a different purpose.

The "design problem" mentioned in the commit message is a different
matter, which was revealed by testing: examine the diff and read the
block comment that was added. A change that breaks 'make system_test'
breaks the system. The symptom may appear to be "only" cosmetic, but
it mustn't casually be dismissed as such; the underlying problem is
actually substantive. It's okay to commit the change with a temporary
workaround to avert breakage, but only as long as we attack the real
problem right away--as is done in the next eleven commits.

20070709T0947Z <address@hidden> [1223]

  progress_meter_test.cpp
Refactor to make a defect easier to notice.

Find a defect--create a test. Find a defect in a system test that
takes minutes to run--create a unit test that takes seconds to run.

But first scan the existing unit test for obvious errors.

20070709T0955Z <address@hidden> [1223]

  progress_meter_test.cpp
Fix the defect noted 20070709T0947Z.

I had a reason for not combining this with the preceding commit. The
proximate cause of the immediate defect is simply that a mistake was
made, but resolving to make fewer mistakes is a lame comeback. Let us
instead seek the ultimate cause by asking productive questions like
"how could this code have been written so that the mistake would have
been more obvious?".

Here's an open question for the reader to ponder: is similar code
elsewhere written in such a way as to make such mistakes obvious? I
didn't look into that, though I have a strong suspicion that someone
should, because I don't want to get too far afield; but the obvious
mistake right in front of us cannot be ignored.

20070709T1003Z <address@hidden> [1223]

  progress_meter_test.cpp
Eliminate a useless test.

Oh, and, BTW, what purpose could this test possibly serve? If there's
none, then the testsuite is better without it.

20070709T1035Z <address@hidden> [1223]

  progress_meter_test.cpp
Demonstrate the design problem identified 20070708T1948Z.

http://www.extremeprogramming.org/rules/testfirst.html

Now the problem is embodied in a (failing) unit test; only now are we
ready to start solving it.

20070709T1043Z <address@hidden> [1223]

  progress_meter.hpp
Remove an untrue comment. No special member function is implicitly
declared.

Read comments before changing code. If that doesn't save time, then
the comments might need work. In this case, which of the four
functions in C++2003 12/1 is missing?

20070709T1054Z <address@hidden> [1223]

  progress_meter.hpp
Rearrange member documentation in declaration order.

I found it harder to follow because it wasn't in that order. And we're
going to need to add members, so let's clean it up first. It would be
wrong to leave this alone and just pick the least-unreasonable place
to add documentation of new members. We don't want the smallest or
cheapest change; our goal is the simplest code.

I committed this separately on purpose. It's a relatively large change
("+16 -16 lines" according to 'viewvc') that can easily be proven to
have no effect on the compiled code, so keeping it separate makes it
easier to review other changes that are more substantive.

20070709T1102Z <address@hidden> [1223]

  progress_meter.hpp
Correct a comment that doesn't parse.

After addressing higher-level issues with the inline documentation, I
went back and read it in detail, and noticed this.

20070709T1219Z <address@hidden> [1223]

  progress_meter_cgi.cpp
  progress_meter_cli.cpp
  progress_meter_wx.cpp
Distinguish required implementation from optional overrides.

Required implementation of pure virtuals is distinguished from
overrides elsewhere, so it should be treated the same way here. See:

http://boost.cvs.sourceforge.net/*checkout*/boost/boost/more/coding_guidelines.html?revision=1.1.2.6#override_group

Again, look for obvious shortcomings and clean them up before changing
a file. I looked for similar problems elsewhere, and would have fixed
them right away if I had found any. Question for the reader: did I get
that right everywhere?

20070709T1241Z <address@hidden> [1223]

  progress_meter.hpp
  progress_meter_cgi.cpp
  progress_meter_cli.cpp
  progress_meter_wx.cpp
Implement const-correctness less strangely.

We're going to add some members. We know that const-correctness is
valuable, and that it costs far less to get it right up front, so we
ask: is it satisfactory as it stands? No. Here's a protected virtual
that's const, and it's called only by a public non-const function, so
constness confers little benefit. Yet it modifies two derived-class
data members that have to be mutable to honor overly-aggressive
constness, so the cost is too high--so fix it now before the problem
grows worse: that costs more up front, but less in the long run.

20070709T1405Z <address@hidden> [1223]

  progress_meter.cpp
  progress_meter.hpp
  progress_meter_cgi.cpp
  progress_meter_cli.cpp
  progress_meter_wx.cpp
Add new members that don't yet actually do anything.

For library-level code, designing the interface can be harder than
writing the implementation. Implementation details are easy to change,
but interface mistakes are hard to fix later. This is reason enough to
commit interface changes in a separate step; another reason is that
interface changes call for separate review. There's a practical reason
as well: header changes trigger lengthy recompiles, and we can work
faster if we stabilize the header first--especially since we're about
to make a substantive change to observable behavior, which calls for
careful testing, which might reveal problems with the intended
implementation. Examine the diffs to see that the next step planned is
explained in a comment--again, to make the change easier to review.

Examining the diffs, note that one member function's documentation
changed slightly when another function was added, the purpose being to
clarify how work is divided between the two. Our goal is not just to
add a new function and document it well; what we want is the clearest
documentation overall.

20070709T1657Z <address@hidden> [1223]

  group_values.cpp
  main_wx.cpp
  progress_meter_cli.cpp
  progress_meter_test.cpp
Remove the defect noted 20070708T1948Z and 20070709T1035Z.

I made this a separate step because the code changes are minimal and
easy to review, whereas the behavior change requires serious testing.
Note that the unit test must change because the progress meter's
culmination is now determinate: it no longer depends on when the dtor
happens to get called.

Question for the reader: did you see the necessity of such a change
before modifying the unit-test file? I didn't foresee it, so when the
test didn't succeed the first time, I stepped back and studied the
problem carefully.

I'm convinced that there's only one right way to do testing (and not
just unit testing):
 - study the change you're about to apply
 - predict the effect it will have on test results
 - where easily practicable, alter the tests in advance to express
     your prediction exactly (usually very practicable for unit tests)
 - make the change and run the test; try to run it in a way that gives
     you only a boolean indication of whether your prediction was
     perfect, and doesn't show you any details
 - if it wasn't perfect, repeat the preceding few steps, and revise
     your prediction without peeking at the answer
 - if it's still not perfect, then look at the test results in detail,
     but only to learn what mistakes you made
 - walk away from the computer and meditate on those mistakes; later,
     repeat the first few steps without looking at the test results,
     trying to get it perfect in one shot
 - if a very few iterations of the last step don't yield perfection,
     then step back and think hard, because you are certainly doing
     something very wrong
I recognize the temptation to copy and paste the observed result into
the unit test without understanding it thoroughly. Fight it.

20070709T2348Z <address@hidden> [1223]

  progress_meter.hpp
Improve and correct documentation. For example, reflect_progress() per
se doesn't offer an opportunity to cancel.

It's troubling that, right before the 20070709T1657Z commit, I didn't
foresee a change in unit-test results. "The purpose of computing is
insight, not numbers", as Hamming said, so I wanted to make sure the
documentation reflects the insight I gained while changing the unit
test. Even though I had read every word already in a previous step,
it still wasn't perfect. This should raise a question for the reader,
which seems too obvious to pose explicitly.

20070710T0033Z <address@hidden> [1223]

  progress_meter.cpp
  progress_meter.hpp
Add a data member to support eventual postcondition testing. Ensure
that all data members are documented, for uniformity.

Study the diffs. One data member hadn't been documented. Maybe that
was okay when we had fewer of them (and there's little to say that
this member's name doesn't already express), but now we have more data
members and it feels more defective to omit documenting any.

As above, an interface change is separated from a behavior change:
separate commits make code review and testing easier.

20070710T1253Z <address@hidden> [1223]

  progress_meter.cpp
Correct a mistake in an error message.

I was about to commit the change that follows this one when I saw this
obvious mistake. Others might argue that such a trivial change needn't
be committed separately--doesn't that just slow things down? My reply
is that I mustn't have been going slow enough, because I made this
mistake. And recording it separately makes it easier for me to scan
'ChangeLog' to see what mistakes I've made. The goal is not to fix
mistakes as we realize them; the goal is to learn from each mistake so
that we don't make it again. I regard it as axiomatic that it costs
less not to make a mistake than to fix it, so I need to treat every
mistake as a learning opportunity.

http://www.technologyreview.com/Infotech/11960/page2/

| He [Knuth] takes this error business very seriously. Engraved
| in the entryway to his home are the words of Danish poet
| Piet Hein:
|
| The road to wisdom?
| Well it's plain
| and simple to express:
|
| Err
| and err
| and err again
| but less
| and less
| and less.

For more about Knuth, see:

http://lists.nongnu.org/archive/html/lmi/2007-06/msg00026.html

20070711T0105Z <address@hidden> [1224]

  progress_meter.cpp
  progress_meter_test.cpp
Replace standard exceptions with 'alert' facility. Soon another type
of 'alert' will be used, and mixing the two would violate concinnity.
Mark as a defect another violation of concinnity.

Question for the reader: did you anticipate the necessary change to
the unit test? If not, what can you learn from that mistake?

In some other library-level code, I prefer to use standard exceptions
only, in order to increase portability. Here, though, the derived
classes were already using 'alert' instead.

As for the last line of the commit message:
  grep progress_meter *.?pp |head --lines=1
Of course, if we don't do 'grep progress_meter', then we shouldn't be
changing this code at all.

20070711T1233Z <address@hidden> [1224]

  progress_meter.cpp
  progress_meter.hpp
  progress_meter_test.cpp
Test a postcondition.

With careful prework, setting the capstone should be easy and safe.
Here, the code was much easier to write than the documentation.

---------

[1] Patch 1 [topic 6506128 in codestriker]

RCS file: /sources/lmi/lmi/group_values.cpp,v
retrieving revision 1.75
diff -u -r1.75 group_values.cpp
--- group_values.cpp    3 Jul 2007 00:21:55 -0000   1.75
+++ group_values.cpp    4 Jul 2007 15:12:39 -0000
@@ -112,7 +112,7 @@
     census_run_result result;
     boost::shared_ptr<progress_meter> meter
         (create_progress_meter
-            (cells.size()
+            (cells.size() + 1 // +1 accounts for the final composite output
             ,"Calculating all cells"
             ,progress_meter_mode(emission)
             )


[2] Patch 2 [topic 7545772 in codestriker]

--- group_values.copy.cpp   2007-07-06 14:18:26.000000000 +0200
+++ group_values.cpp    2007-07-06 14:44:07.000000000 +0200
@@ -118,14 +118,14 @@
             )
         );

-    for(unsigned int j = 0; j < cells.size(); ++j)
+    bool cancelled_by_user = false;
+
+    for(unsigned int j = 0; j < cells.size() && !cancelled_by_user; ++j)
         {
         try
             {
-            if(cell_should_be_ignored(cells[j]))
+            if(!cell_should_be_ignored(cells[j]))
                 {
-                continue;
-                }
 /*
 // TODO ?? Why use class IllusVal at all? What's the advantage over
 // the account-value class?
@@ -153,26 +153,26 @@
                 ,emission
                 );
             }
+            }
         catch(...)
             {
             report_exception();
             }
-
-        if(!meter->reflect_progress())
-            {
-            result.completed_normally_ = false;
-            goto done;
-            }
+        cancelled_by_user = !meter->reflect_progress();
         }

+    if(!cancelled_by_user)
+        {
     result.usec_for_output_ += emit_ledger
         (file
         ,-1
         ,composite
         ,emission
         );
+        }
+
+    result.completed_normally_ = !cancelled_by_user;

-  done:
     double total_usec = timer.stop().elapsed_usec();
     status() << Timer::elapsed_msec_str(total_usec) << std::flush;
     result.usec_for_calculations_ = total_usec - result.usec_for_output_;
@@ -770,4 +770,3 @@
         }
 }

-