digitalmars.D.learn - My first D module - Critiques welcome.
- John Carter (32/32) Dec 23 2013 So I resolved to learn D, and try code as idiomatically as I could.
- bearophile (58/103) Dec 23 2013 I think it's important to write lot of unittests (and use other
- bearophile (5/5) Dec 23 2013 do you have negative tests that tests the code raises an
- John Carter (133/163) Dec 23 2013 Part of the aim of the exercise was to explore that difference.
- bearophile (39/52) Dec 23 2013 From/to Roman numbers conversions are never performance-critical.
- John Carter (28/85) Dec 23 2013 Thanks.
- Gary Willoughby (8/22) Dec 23 2013 I like it and it seems you are grasping D well. I wonder if the
- monarch_dodra (16/23) Dec 24 2013 I was about to submit an "eval" function which would render this
- Xinok (22/36) Dec 23 2013 Overall, I say it's not bad. I just have a few comments on your
- Brad Anderson (5/19) Dec 23 2013 You could also do some neat stuff with opDispatch. Someone
- Dejan Lekic (6/10) Dec 25 2013 The idea is actually brilliant. :)
- Artur Skawina (18/25) Dec 25 2013 Note that the "D’s version has no runtime overhead at all" part is not...
- Timon Gehr (5/15) Dec 24 2013 What is the intended behaviour? I think that eg. "XIIX" is not a valid
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
John Carter:I also attempted to do it in "Best Practice Test Driven Development" formI 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 digitThe 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) constAnnotations 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
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
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" formPart 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.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.assert( i != 0);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 neededSometimes you can also add a comment in the assert that explains why this precondition has failedto 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);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"?More 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 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 theThat's quite bad code. Better: return roman.map!romanToDigit.array;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 tableActually 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''sI 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. ------------------------------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.
Dec 23 2013
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.htmlWhen 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.That's quite bad code. Better: return roman.map!romanToDigit.array;That certainly looks nicer, alas, dmd is unhappy....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
Thanks. Taken on board and fixes pushed. I would love away around that.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:http://dlang.org/phobos/std_exception.htmlJohn Carter: I'm storing the roman numerals as dchars. I have the vague hope this is-- 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. ------------------------------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: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,return roman.map!romanToDigit.array; That certainly looks nicer, alas, dmd is unhappy....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
Dec 23 2013
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
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
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
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
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
On 12/25/13 15:07, Dejan Lekic wrote: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; } } arturYou 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.
Dec 25 2013
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