My first D module - Critiques welcome.

[John Carter]

Thanks.

Taken on board and fixes pushed.

I would love away around that.
>>
>
> http://dlang.org/phobos/std_exception.html
>

Tried using assertThown!AssertError but @safe still, umm, throws that out.


On Tue, Dec 24, 2013 at 3:22 PM, bearophile <bearophileHUGS at lycos.com>wrote:

> John Carter:
>
>
>  I'm storing the roman numerals as dchars. I have the vague hope this is
>> more efficient than storing them as strings.
>>
>
> From/to Roman numbers conversions are never performance-critical. It looks
> like premature optimization.
>
>
>
>  What do you mean by "missing UCSF"?
>>
>
> Sorry, it's UFCS (Universal Function Call Syntax), it means writing x.foo
> instead of foo(x).
>
>
>
>  I would love away around that.
>>
>
> http://dlang.org/phobos/std_exception.html
>
>
>  That's quite bad code. Better:
>>>
>>>     return roman.map!romanToDigit.array;
>>>
>>>
>>>  That certainly looks nicer, alas, dmd is unhappy....
>>
>
> When you have only one argument the () is not needed:
>
> return roman.map!(a => romanToDigit(a)).array;
>
> The cause of your error: romanToDigit is module-private. If you make
> romanToDigit public the error goes away. If you use a lambda the problem
> also goes away for different reasons. So in this case using a lambda is OK.
>
>
>
>  I thought it meant just putting a ddoc /// extra / flag on the comment,
>>
>
> Right. The Ddoc-produced html should show the unittest.
>
> You can also add a module-level ddoc to your module.
>
>
> return find!((a,b) =>
>
> In most cases it's better to put a space after every comma ==>
>
> return find!((a, b) =>
>
>
>  };
>>
>
> In D don't put a ; there for struct/classes/unions.
>
> I think the code density of the toRomansWithZero() function is too much
> low. I suggest to pack it vertically a little more. D is not C#.
>
>
> O suggest you to be more careful with size_t, your code doesn't compile on
> a 32 bit system:
>
> test.d(59): Error: cannot implicitly convert expression (this.regionIndex)
> of type const(ulong) to uint
> test.d(62): Error: cannot implicitly convert expression (this.regionIndex)
> of type const(ulong) to uint
> test.d(62): Error: cannot implicitly convert expression (this.regionIndex)
> of type const(ulong) to uint
> test.d(66): Error: cannot implicitly convert expression (this.previous) of
> type const(ulong) to uint
>
>
> So this could be better:
>
>    size_t regionIndex; /// Some digits in the system "dominate" over
> others.
>    size_t previous;    /// Hardcode link to previous digit.
>
>
> I also suggest to add an ending "." to comments.
>
> With this I think the surface review of your code is mostly done.
>
> Bye,
> bearophile
>



-- 
John Carter
Phone : (64)(3) 358 6639
Tait Electronics
PO Box 1645 Christchurch
New Zealand

-- 

------------------------------
This email, including any attachments, is only for the intended recipient. 
It is subject to copyright, is confidential and may be the subject of legal 
or other privilege, none of which is waived or lost by reason of this 
transmission.
If you are not an intended recipient, you may not use, disseminate, 
distribute or reproduce such email, any attachments, or any part thereof. 
If you have received a message in error, please notify the sender 
immediately and erase all copies of the message and any attachments.
Unfortunately, we cannot warrant that the email has not been altered or 
corrupted during transmission nor can we guarantee that any email or any 
attachments are free from computer viruses or other conditions which may 
damage or interfere with recipient data, hardware or software. The 
recipient relies upon its own procedures and assumes all risk of use and of 
opening any attachments.
------------------------------
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/digitalmars-d-learn/attachments/20131224/92b1725b/attachment.html>