[dev] [patch] Bug#698 -- visual event status cues in Kronolith views

[Derek P. Moore]

>> Here's a resubmittal of my kronolith/lib/Driver.php patch.
>
> I take it this is the version you uploaded to the ticket?

I'm pretty sure I uploaded both versions of the patch to the ticket.

>> I suppose this amounts to a minor performance improvement, short
>> circuiting test cases for a majority of events.
>
> Please justify these with benchmarks.

If we were to process 200,000 events at once through the getLink() function,
then we might notice an improvement from the handful of clock cycles saved in
processing each event.  But in real world situations, it's probably not
measurable.

In my first patch, $status was set to empty then all confirmed events went
through two hasStatus() tests regardless.  If the event was something other
than confirmed, then $status gets set twice, first to empty, then to an
appropriate value, and, of course, the code goes through one or two 
hasStatus()
tests, depending.

The second patch, all confirmed events are tested once with hasStatus(), then
$status is set to empty, and the rest of the tests are skipped.  That's two to
three tests that get to be skipped for a majority of events in a calendar,
depending on if it's a local or remote calendar (remote calendars get tested
for KRONOLITH_STATUS_NONE, but only in the second patch).

So, the worst case for the first patch is:  $status gets set twice and 
the event
goes through two hasStatus() tests.

The worst case for the second patch is:  $status gets set once and the event
goes through three to four hasStatus() tests (depending on local or remote,
respectively).

Personally, I find both patches equally readable.  But I tend to think the
second patch is more logically thorough.  The second patch lays all of 
the test
cases out on the line for future authors.  Whereas, in the first patch, two of
the test cases (CONFIRMED and NONE) are implicit in first setting $status to
empty.  You and I may know that those two test cases are implicit in first
setting $status to empty, but I doubt future authors will.  In which case,
maybe the second patch is more readable and understable to future authors.

I'd lobby on behalf of the second patch, myself.  But it's entirely up to you.