www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - My first D module - Critiques welcome.

reply John Carter <john.carter taitradio.com> writes:
So I resolved to learn D, and try code as idiomatically as I could.

So here is a trivial module that encodes and decodes roman numerals.

https://bitbucket.org/JohnCarter/roman/src/9ec5b36b9973426f35b0ab9dfd595cb3b305e39e/roman.d?at=default

I also attempted to do it in "Best Practice Test Driven Development" form
with commits on every change.

So you can see the blow by blow history of how it grew.

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

Thanks for your help and patience with various stupid newbie questions I
asked in the process!

-- 
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.
------------------------------
Dec 23 2013
next sibling parent reply "bearophile" <bearophileHUGS lycos.com> writes:
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
Dec 23 2013
next sibling parent "bearophile" <bearophileHUGS lycos.com> writes:
do you have negative tests that tests the code raises an 
assertion Error when you give a 0 to the function? Have you also 
tried to give it a uint.max?

Bye,
bearophile
Dec 23 2013
prev sibling parent reply John Carter <john.carter taitradio.com> writes:
Wow! Thanks for an excellent reply! I really appreciate it...



On Tue, Dec 24, 2013 at 11:38 AM, bearophile <bearophileHUGS lycos.com>wrote:

 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.
Part of the aim of the exercise was to explore that difference. Conclusions from that exploration? 1) Very fine grained steps resulted in fewer bugs and fewer backtracks. 2) Very fine grained steps resulted in better factored code. 3) Watching others do the same exercise led me to the attitude "adding more test cases is Bad". 4) Very fine grained steps slowed me down compared to my more usual somewhat loose style. 5) Thoughtfully adding Provocative test cases in the order of "What I need to Handle Next" is Very Very Good. 6) Replacing "Fine Grained Steps" with, whenever stuck, pull out a smaller chunk and TDD that first (with Provocative test cases) worked Very Very Well. Bottom Line? In future I will be doing... * Thoughtfully choosing a minimal number of provocative test cases. (Ones that direct me to growing the code correctly) * I will take the largest steps I can where I can go from test fail to test pass in a single change. * Whenever I misjudged and take too large a step, I will factor out a smaller function and test and develop that first via the same process.
    assert( i != 0);

 Sometimes you can also add a comment in the assert that explains why this
 precondition has failed
In the Ruby world the test frameworks (plus some of my own utility modules) give a very rich set of assertions and logging information on assert failures. I usually can just look at the output of an Ruby assert failure in my code and tell you exactly what the bug is. For this case I resolve not to get into that, but ultimately I would love to find a D module that will give that rich amount of info. Any recommendations? 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?
I'm storing the roman numerals as dchars. I have the vague hope this is more efficient than storing them as strings. My reasoning has more to do with "That's the way I did it with ASCII and C strings and chars" than profiling or deep knowledge. Earlier versions I uses strings, but when I want to iterate of a string in fromRoman I converted to dchars.
           return  toUTF8([digit]) ~ toRomansWithZero( i - value);

 More missing UCSF.
Alas, my google search results are cloudy by millions of hits for University of California San Francisco. (Wow! Google search is getting fast these days... Already getting a hit on your Post!) What do you mean by "missing UCSF"?
 do you have negative tests that tests the code raises an assertion Error
 when you give a 0 to the function?
Interestingly enough I did. In the somewhat ugly form of... unittest { try { toRoman( 0); assert( false); } catch( core.exception.AssertError assertError) { assert(true); } } But when I added the safe dmd said... roman.d(314): Error: can only catch class objects derived from Exception in safe code, not 'core.exception.AssertError' I figured safe was worth more to me than that test so I discarded it. I would love away around that. In Ruby I tend to have several flavours of AssertError, for example PreconditionFailure, PostConditionFailure and InvaraintFailure. return array(map!((a) => romanToDigit(a))(roman));

 That's quite bad code. Better:

     return roman.map!romanToDigit.array;
That certainly looks nicer, alas, dmd is unhappy.... dmd -unittest -main roman.d && ./roman /usr/include/dmd/phobos/std/algorithm.d(425): Error: function roman.romanToDigit is not accessible from module algorithm /usr/include/dmd/phobos/std/algorithm.d(459): Error: function roman.romanToDigit is not accessible from module algorithm /usr/include/dmd/phobos/std/algorithm.d(411): Error: template instance std.algorithm.MapResult!(romanToDigit, string) error instantiating roman.d(232): instantiated from here: map!string roman.d(232): Error: template instance std.algorithm.map!(romanToDigit).map!string error instantiating As a halfway point I have taken it to... /// Convert string of char code points to array of UTF32 so we can have random access. pure immutable uint[] romanToDigitArray( in string roman) { return roman.map!((a) => romanToDigit(a)).array; } Xinok <xinok live.com>said... There are a series of unittests which could be merged together. Keep the
 line comments to keep different tests separate, but there's no need to have
 multiple unittests which contain nothing more than a few simple asserts. Or
 as bearophile suggested, use an in-out table
Actually I do have a reason for keeping them separate. I have been taking on board what books like "xUnit Test Patterns by Gerard Meszaros" and various videos by JB Rainsberger say. I used to have long "Ramble On" unit tests, but found from painful experience that the above authors are dead right. Ramble on tests are fragile, hard to maintain and destroy defect localization. One of the prime values of unittesting is unlike an integrated test, a unit test failure in a well designed suite of unit tests, tells you exactly where the defect is. Rainsberger talks about having "Only one reason for a test to fail, only one test to fail if you have a defect". I'm not quite sure how to achieve that. My main gripe with my current set of tests is I have too many. Each test should be pinning exactly one behaviour of the problem, and I should have only one test for each behaviour. In terms of having them data driven, it comes back to what I was saying about needing a richer palette of asserts and better feed back from an assert failure. If I make them data driven I will lose which test case failed. Ideally I should make them data driven AND use a more informative assert facility that tells me what the expression was and what the value of the variables that went in to that expression were. // 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.
I don't quite understand this comment... I thought it meant just putting a ddoc /// extra / flag on the comment, but nothing new happened either on a test failure or on "dmd -D" output (version v2.064) Revised version with most comments worked in now up at... https://bitbucket.org/JohnCarter/roman/src/0362f3626c6490490136b097f23c67fc2970c214/roman.d?at=default Once again, thanks to everyone for fantastic feedback. -- 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. ------------------------------
Dec 23 2013
parent reply "bearophile" <bearophileHUGS lycos.com> writes:
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 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
Dec 23 2013
parent John Carter <john.carter taitradio.com> writes:
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 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



 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. ------------------------------
Dec 23 2013
prev sibling next sibling parent reply "Gary Willoughby" <dev nomad.so> writes:
On Monday, 23 December 2013 at 21:34:34 UTC, John Carter wrote:
 So I resolved to learn D, and try code as idiomatically as I 
 could.

 So here is a trivial module that encodes and decodes roman 
 numerals.

 https://bitbucket.org/JohnCarter/roman/src/9ec5b36b9973426f35b0ab9dfd595cb3b305e39e/roman.d?at=default

 I also attempted to do it in "Best Practice Test Driven 
 Development" form
 with commits on every change.

 So you can see the blow by blow history of how it grew.

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

 Thanks for your help and patience with various stupid newbie 
 questions I
 asked in the process!
I like it and it seems you are grasping D well. I wonder if the entire implementation could be done using templates and all done at compile time? It would be interesting to explore. int year = fromRoman!("DXLIX"); string year = toRoman!(2013); Very similar to how [octal][1] is implemented;
Dec 23 2013
parent "monarch_dodra" <monarchdodra gmail.com> writes:
On Monday, 23 December 2013 at 22:46:24 UTC, Gary Willoughby 
wrote:
 I like it and it seems you are grasping D well. I wonder if the 
 entire implementation could be done using templates and all 
 done at compile time? It would be interesting to explore.

 int year    = fromRoman!("DXLIX");
 string year = toRoman!(2013);

 Very similar to how [octal][1] is implemented;


I was about to submit an "eval" function which would render this kind of stuff obsolete. The idea is that since D has CTFE built-in, such templates are not needed, since you can do: enum year = fromRoman("DXLIX"); The "problem" is that is you want a *variable* that is initialized *to* a certain value, but don't want to pay for the initialization, you have to write: enum yearEnum = fromRoman("DXLIX"); int year = yearEnum; "eval" (which is documented in the docs, but isn't in phobos), would allow: int year = eval!(fromRoman!("DXLIX")); I think such a generic solution is a better approach, and generally useful regardless of what you are dealing with.
Dec 24 2013
prev sibling next sibling parent "Xinok" <xinok live.com> writes:
On Monday, 23 December 2013 at 21:34:34 UTC, John Carter wrote:
 So I resolved to learn D, and try code as idiomatically as I 
 could.

 So here is a trivial module that encodes and decodes roman 
 numerals.

 https://bitbucket.org/JohnCarter/roman/src/9ec5b36b9973426f35b0ab9dfd595cb3b305e39e/roman.d?at=default

 I also attempted to do it in "Best Practice Test Driven 
 Development" form
 with commits on every change.

 So you can see the blow by blow history of how it grew.

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

 Thanks for your help and patience with various stupid newbie 
 questions I
 asked in the process!
Overall, I say it's not bad. I just have a few comments on your coding style. Be consistent with your opening braces. Personally, I always place it on the next line, regardless if it's a function or something else. unittest { } instead of: unittest { } The standard in Phobos is that 1 tab = 4 spaces. Otherwise, when using spaces to align code, make sure it's consistent and everything lines up. (lines 145-155) There are a series of unittests which could be merged together. Keep the line comments to keep different tests separate, but there's no need to have multiple unittests which contain nothing more than a few simple asserts. Or as bearophile suggested, use an in-out table. Besides for grouping things together and making your code more organized, it's better for those of us who use IDEs, so we can collapse the unittest with a single click.
Dec 23 2013
prev sibling next sibling parent reply "Brad Anderson" <eco gnuk.net> writes:
On Monday, 23 December 2013 at 21:34:34 UTC, John Carter wrote:
 So I resolved to learn D, and try code as idiomatically as I 
 could.

 So here is a trivial module that encodes and decodes roman 
 numerals.

 https://bitbucket.org/JohnCarter/roman/src/9ec5b36b9973426f35b0ab9dfd595cb3b305e39e/roman.d?at=default

 I also attempted to do it in "Best Practice Test Driven 
 Development" form
 with commits on every change.

 So you can see the blow by blow history of how it grew.

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

 Thanks for your help and patience with various stupid newbie 
 questions I
 asked in the process!
You could also do some neat stuff with opDispatch. Someone actually wrote an article about using it with roman numerals: http://idorobots.org/2012/03/04/romans-rubies-and-the-d/
Dec 23 2013
parent reply Dejan Lekic <dejan.lekic gmail.com> writes:
 You could also do some neat stuff with opDispatch.  Someone
 actually
 wrote an article about using it with roman numerals:
 http://idorobots.org/2012/03/04/romans-rubies-and-the-d/
The idea is actually brilliant. :) I think I may use it in the future when I need to deal with roman numbers. -- Dejan Lekic dejan.lekic (a) gmail.com http://dejan.lekic.org
Dec 25 2013
parent Artur Skawina <art.08.09 gmail.com> writes:
On 12/25/13 15:07, Dejan Lekic wrote:
 You could also do some neat stuff with opDispatch.  Someone
 actually
 wrote an article about using it with roman numerals:
 http://idorobots.org/2012/03/04/romans-rubies-and-the-d/
The idea is actually brilliant. :) I think I may use it in the future when I need to deal with roman numbers.
Note that the "D’s version has no runtime overhead at all" part is not true - - there still is the overhead of a function call (which he admits to later). A truly overhead-less version would be: struct Roman { template opDispatch(string number) { enum num = number.replace("IV", "IIII") .replace("IX", "VIIII") .replace("XL", "XXXX") .replace("XC", "LXXXX"); enum opDispatch = num.count('I') + num.count('V') * 5 + num.count('X') * 10 + num.count('L') * 50 + num.count('C') * 100; } } artur
Dec 25 2013
prev sibling parent Timon Gehr <timon.gehr gmx.ch> writes:
On 12/23/2013 10:34 PM, John Carter wrote:
 So I resolved to learn D, and try code as idiomatically as I could.

 So here is a trivial module that encodes and decodes roman numerals.

 https://bitbucket.org/JohnCarter/roman/src/9ec5b36b9973426f35b0ab9dfd595cb3b305e39e/roman.d?at=default

 I also attempted to do it in "Best Practice Test Driven Development"
 form with commits on every change.

 So you can see the blow by blow history of how it grew.

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

 Thanks for your help and patience with various stupid newbie questions I
 asked in the process!
...
What is the intended behaviour? I think that eg. "XIIX" is not a valid roman numeral. pure immutable uint[] romanToDigitArray( in string roman) 'immutable' does not do anything here.
Dec 24 2013