My first D module - Critiques welcome.

[bearophile]

John Carter:

> I also attempted to do it in "Best Practice Test Driven 
> Development" form

I think it's important to write lot of unittests (and use other 
kind of tests, automatic testing like in Haskell, fuzzy testing, 
integration tests, smoke tests, etc), but I think Test Driven 
Development is mostly silly. It's better to actually think a good 
solution before writing your code. Thinking-Driven Development is 
better.


> I would welcome any critique, advice, flames, recommendations, 
> etc. etc.

This is a first pass, I have not studied the logic of your code 
nor other higher level things.

The importance of my comments is not uniform, some of them are 
important, while others are more based on my taste.


> import core.exception;
> import std.array;
> import std.algorithm;
> import std.stdio;
> import std.utf;
> import std.range;

I usually prefer to put the more general modules first. So stdio 
goes first.


> /++ Convert an unsigned integer to a roman numeral string. +/

For single-line ddoc comments the /// is enough.
For the other situations /** */ is enough.
/++/ is probably better left to comment out code.


> pure string toRoman( in uint i)

I suggest to never put a space after the opening (.


> /++ Convert an unsigned integer to a roman numeral string. +/
> pure string toRoman( in uint i)

This is a very simple to use function, but in general don't 
forget to list every argument and return in the function ddoc 
using the specific ddoc syntax.


>    assert( i != 0);

Sometimes you can also add a comment in the assert that explains 
why this precondition has failed.


>    return romanSystem[$-1].toRomansWithZero( i);

[$-1] is OK. But sometimes also the usage of ".back" is OK.


>    return fromRomanDigitArray( romanToDigitArray( roman));

More readable and less noisy:

>    return roman.romanToDigitArray.fromRomanDigitArray;

Also, the "array" suffix to those names is probably too much 
tying to the implementation, so better to remove the "array".


> struct romanSystemData {

In D struct/class/union names start with an uppercase.


>    dchar digit;       /// UTF32 representation of each roman 
> digit
>    uint  value;       /// Integer value of digit

The alignment is nice, but it burns a little of coding time (if 
it's the IDE to do it then I guess it's OK).


>    pure string toRomansWithZero( in uint i) const

Annotations like "pure" are in my opinion better on the right, 
after const.


>          return  toUTF8([digit]) ~ toRomansWithZero( i - value);

More missing UCSF.


>       if( 0 == previous) {

This Yoda Style is not needed in D, because D doesn't compile 
code like "if (previous = 0)".


>          return "";

Alternative is "return null;".


>          immutable auto delta = 
> (value-romanSystem[regionIndex].value);

auto is not needed here. And usually put a space before and after 
every operator, for readability:

            immutable delta = (value - 
romanSystem[regionIndex].value);


>             return toUTF8( [romanSystem[regionIndex].digit, 
> digit]) ~ romanSystem[regionIndex].toRomansWithZero(  i - 
> delta);

I have not studied the logic of this code, but are you sure 
toUFT8 is needed to convert Arabic numbers to Roman and the other 
way around?


> // Test the 'I''s
> unittest {

Some unittest system allows to put that comment "Test the 'I''s" 
in a more active role. So it's shown on screen when the relative 
unittest fails.

Also, I suggest to add some "ddoc unittests". It's a feature 
recently added to D.


> // The romans were crazy M
> unittest {
>    assert( "CMXCIX"  == toRoman( 999));
> }

Most of your unittests are in-out pairs. So you can shorten them 
with a in-out table.


>    return array(map!((a) => romanToDigit(a))(roman));

That's quite bad code. Better:

     return roman.map!romanToDigit.array;


>    if( 0 == digitArray.length) {

==>

     if (digitArray.empty) {


>    auto smallerStuff      = smallerThanLast( digitArray);
>    auto smallerStuffValue = fromRomanDigitArray( smallerStuff);
>    auto biggerStuff       = digitArray[ 
> 0..($-smallerStuff.length-1)];
>    auto biggerStuffValue  = fromRomanDigitArray( biggerStuff);

Perhaps you want some const annotations.


>    foreach( i; 1..2014) {

==>

     foreach (immutable i; 1 .. 2014) {

Bye,
bearophile