On Sunday, August 04, 2013 06:20:57 Andre Artus wrote:
> > Bearophile:
> > If not already present this array should go in std.datetime or
> > 
> > core.time:
> >     static immutable string[] monthNames = [
> > 
> >         "January", "February", "March", "April", "May", "June",
> >         "July", "August", "September", "October", "November",
> > 
> > "December"
> > 
> >     ];
> 
> It should probably be picked up from the OS, to support localization.

std.datetime has something like that internally for some of what it does (in particular, toSimpleString, which I wouldn't even put in there now if I could go back), but we explicitly didn't make anything like that public, because it's English-specific.

- Jonathan M Davis

On Sun, Aug 04, 2013 at 05:02:05AM +0200, bearophile wrote: [...]
> A bit improved chunkBy could go in Phobos.

Yeah I'll look into that sometime. It'll definitely be a useful thing to have, I think.


> ---------------
> 
> >For our purposes, though, we can't just do this in a loop, because it has to interface with the other components, which do not have a matching structure to a loop over dates.<
> 
> It's just because D doesn't yet have a yield designed like C#. "yield" for coroutines is a very nice kind of glue.

Don't fibres already have yield()?


> ---------------
> 
> auto datesInYear(int year) {
>     return Date(year, 1, 1)
>         .recurrence!((a,n) => a[n-1] + dur!"days"(1))
>         .until!(a => a.year > year);
> }
> 
> 
> ===>
> 
> 
> auto datesInYear(in uint year) pure /*nothrow*/

I think "in uint" is redundant. Uints don't need to be const.

The Date ctor isn't nothrow, unfortunately.

Also, std.datetime docs explicitly state that years can be negative (in which case they represent B.C..


> in {
>     assert(year > 1900);

Good idea, I should be using contracts. :)

But in this case, there's no good reason to restrict it to >1900, because std.datetime can handle years past 0 AD.


> } body {
>     return Date(year, 1, 1)
>            .recurrence!((a, n) => a[n - 1] + dur!"days"(1))
>            .until!(d => d.year > year);
> }
> 
> 
> I suggest to align the dots vertically like that. And generally _all_ variables/arguments that don't need to mutate should be const or immutable (or enum), unless holes in Phobos or in the type system or other factors prevent you to do it.

Hmm. OK, but I still think "in uint" is redundant.


> ---------------
> 
> return chunkBy!"a.month()"(dates);
> 
> ===>
> 
> return dates.chunkBy!q{ a.month };

I don't really like using q{} here. But the UFCS thing probably looks better, considering the rest of the code.


> ---------------
> 
> byWeek() is not so simple. Most of its code is boilerplate code. Perhaps using a "yield" it becomes simpler.

I think Phobos needs better primitives for constructing chunked ranges. :)  I was thinking if there's a common core that can be factored out of chunkBy and byWeek, to make a generic range chunking function that can handle both.


> ---------------
> 
> string spaces(size_t n) {
>     return repeat(' ').take(n).array.to!string;
> }
> 
> ===>
> 
> string spaces(in size_t n) pure nothrow {
>     return std.array.replicate(" ", n);
> }

Thanks! I didn't know about std.array.replicate. :)


> Currently in programs that import both std.range and std.array you have to qualify the module for replicate.
> 
> In Python this is just:
> 
> ' ' * n

The closest I ever got to that in D was to define:

	string x(string s, size_t n) { ... }

so you could write " ".x(10) as a visual approximation to " "*10.  But
this is not a good idea for other reasons ("x" is too confusing a name
and likely to clash with other symbols).


> ---------------
> 
> auto buf = appender!string();
> 
> Perhaps this suffices:
> 
> appender!string buf;

Actually, that doesn't compile.


> ---------------
> 
> string[] days = map!((Date d) => " %2d".format(d.day))(r.front)
>                 .array;
> 
> (not tested) ==>
> 
> const days = r.front.map!(d => " %2d".format(d.day)).array;

I wanted to make the return type explicit for the reader's benefit.


[...]
> If you put the days inside buf, do you really need to turn days into
> an array with array()?

The reason is actually so that I can assert on its length. Otherwise you're right, I could just put it directly into buf.


[...]
> ---------------
> 
> If not already present this array should go in std.datetime or core.time:
> 
>     static immutable string[] monthNames = [
>         "January", "February", "March", "April", "May", "June",
>         "July", "August", "September", "October", "November",
> "December"
>     ];

Well, once we have a proper i18n module...


> ---------------
> 
> return to!string(spaces(before) ~ name ~ spaces(after));
> 
> ==> (untested)
> 
> return text(before.spaces, name, after.spaces);
> 
> Or maybe even just (untested):
> 
> return before.spaces ~ name ~ after.spaces;

You're right, there's no need to call to!string. I think that was left over from older code when some of the pieces were char[].


> ---------------
> 
> auto formatMonth(Range)(Range monthDays)
>     if (isInputRange!Range && is(ElementType!Range == Date))
> {
>     assert(!monthDays.empty);
>     assert(monthDays.front.day == 1);
> 
>     return chain(
>         [ monthTitle(monthDays.front.month) ],
>         monthDays.byWeek().formatWeek());
> }
> 
> ===> (untested)
> 
> auto formatMonth(R)(R monthDays)
>     if (isInputRange!R && is(ElementType!R == Date))
> in {
>     assert(!monthDays.empty);
>     assert(monthDays.front.day == 1);
> } body {
>     return [monthDays.front.month.monthTitle]
>            .chain(monthDays.byWeek.formatWeek);

I don't like overusing UFCS when it makes the code harder to read.


> }
> 
> 
> Generally I suggest to use pre- and post conditions.

Good idea. We should use contracts more in D... in part so that there's enough code out there to pressure people into fixing DbC-related issues. ;-)


> ---------------
> 
> return months.map!((month) => month.formatMonth());
> 
> ===> (untested)
> 
> return months.map!formatMonth;

Good idea!


> ---------------
> 
> .map!((r) =>
> 
> ===>
> 
> .map!(r =>

Heh, I didn't know you could omit the parens there!


> ---------------
> 
> int year = to!int(args[1]);
> 
> ===>
> 
> int year = args[1].to!int;

I'm on the fence about this one. I still think to!int(args[1]) reads better. But your version is not bad, either.


> ---------------
> 
> On Rosettacode there is a shorter calendar: http://rosettacode.org/wiki/Calendar#D
> 
> If you want we can put, as second D entry, your calendar code
> (without unittests) in that page too.
[...]

Well, I didn't write my version to be short. :)  The goal was to use it as an example of the kind of component-style programming Walter was talking about.

As for putting it up on Rosettacode, sure, go ahead. :) Just make sure you test the result first, though. Some of your suggestions actually don't compile.

I'm going to incorporate some of your suggestions in the code and update the article.  Thanks for the feedback!


T

-- 
It is widely believed that reinventing the wheel is a waste of time; but I disagree: without wheel reinventers, we would be still be stuck with wooden horse-cart wheels.

On Tue, Aug 06, 2013 at 06:20:23PM +0200, bearophile wrote:
> H. S. Teoh:
> 
> >It looks like I may have to sort out some issues with compiler bugs before officially posting this article, though, since the code apparently fails to compile with many versions of DMD. :-(
> 
> If not already present, I suggest you to put a reduced version of
> the problems you have found (with map or something else) in
> Bugzilla. (Otherwise I'll try to do it later).
[...]

Actually, the problem has been fixed in git HEAD, it's just that 2.063 doesn't have the fix. The basic cause is that std.range.chunks in 2.063 and earlier requires slicing, but I need to use it with a forward range that doesn't have slicing. This limitation has been removed in git HEAD (upcoming 2.064).

I've already solved the problem in the code by using "static if (__VERSION__ < 2064L)" and providing a bare-bones replacement of std.range.chunks that does support forward ranges.


T

-- 
Customer support: the art of getting your clients to pay for your own incompetence.

On 2013-08-04 06:53, Jonathan M Davis wrote:

> std.datetime has something like that internally for some of what it does (in
> particular, toSimpleString, which I wouldn't even put in there now if I could
> go back), but we explicitly didn't make anything like that public, because
> it's English-specific.

The solution for that is to make it possible to plug in support for new languages and make the default English.

-- 
/Jacob Carlborg