Re: bug in ztrftime(): '%e' and '%f' specifiers swapped

[Greg Klanderman <gak@klanderman.net>]

> Interesting how subjective this is, I find it significantly less so

Yep!  Not to try to sway you, but I'll give you my reasons just for
the fun of it..

I guess the biggest is that I really don't like cases that do
something then fall through, it's just very hard to reason about, and
usually makes further modifications difficult and error prone.

Also, this style very rarely is able to extend beyond two cases - case
in point when you added the 'H' format you had to violate your first
three points below.

> - there is no immediate visual cue that the formats do different things

I would always assume if there are multiple cases sharing the same
logic that the logic itself will differentiate between them, not that
they all function identically.

> - the test for the format letter is repeated without it being obvious it
>   is the same test
> - it's made even less obvious because the second test has to advance
>   back down the format string since the key letter isn't saved in a
>   variable [we do this sort of thing all the time elsewhere, though]

I certainly would have saved the switch'd on character in a variable
if I were writing this, and probably if I'd had a working build system
I might have done more significant cleanup.  On the other hand, when
submitting patches to a project like this I usually go for the minimal
change because otherwise it's a larger burden on whoever reviews and
commits the change.  Seeing that pattern (fmt[-1] == 'x') already in
use in that function I figured it was considered not so bad.

> - the fact that 'f' sets the "strip" feature unconditionally is obscured

strip = strip || (fmtchar == 'f') is pretty clear to me :-)

> - I'm not that keen on "||" and "&&" in assignments anyway [again, the
>   shell is full of stuff I'm not that keen on]

> so I've committed the first one.

thank you!

greg