Re: Rosetta code added in OCaml, please check
"Manfred Lotz [email protected] [ocaml_beginners]" <[email protected]> Tue, 27 Dec 2016 19:19:30 +0100
| Newsgroups | gmane.comp.lang.ocaml.beginners |
|---|---|
| Message-ID | <[email protected]> |
--dD2lzK1KrONxj7pEIJRv84JevOUkAj0ow1ZvMZF Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Hi Gabriel, First, thanks a lot for looking into this. On Tue, 27 Dec 2016 11:00:23 -0500 "Gabriel Scherer [email protected] [ocaml_beginners]" <[email protected]> wrote: > Hi Manfred, > > Thanks for the interesting question. Having your code reviewed is an > excellent way to make progress. > > For history variables, your code is fine, but you should consider > using the Stack module instead of re-implementing yourself using > mutable lists. You are right. I changed the implementation using the Stack module. > (Independently, when you have a record with a single > mutable field, you could consider using the standard "ref" type > instead; but sometimes having your own record type actually helps > readability.) > I chose a record because if something gets added (time stamp or so) then it is just about adding a field. > For the enumeration of ordinal numbers, there are two things that I > am not fond of in your code: > > 1. I think that "when" is a dangerous construction to use and its use > should be de-emphasized, because it encourages code that relies on > ordering in non-trivial way, diminishing a lot of the value of > pattern-matching. The fact that, apart from the "when" clause, you > are not using expressive patterns in any way is the sign of a code > smell in my book. When I teach beginners, I ask them to only > pattern-match on algebraic datatypes. I would encourage you to > rewrite this part with an explicit "if ... else if ..." cascade. > This is a very interesting comment. I wasn't aware of this. Thinking about it (I will look into this more closely) it makes sense. So, I chose your suggestion of a cascaded "if...else if...". > You could also decide to factorize the three cases with a function > > let non_th n = > let d = n mod 10 in > d >= 1 && d <= 3 && n mod 100 <> (10 + d) in > > 2. The creation of the array in "f" seems unnecessary to me. Using a > "for" loop would result in simpler code, whose structure would be > more directly apparent to readers. > You are right, of course. I changed it accordingly. I also changed the name of 'f' to 'show_numbers' which is surely a better name than just 'f'. > (If you like functional pipelines with higher-order iterator, you may > be pleased by the following reformulation of your code, that I think > is still not as good as a for loop: > > Array.init ... > |> Array.iter ... > |> print_newline > ) > I like it very much in some situations. But your suggestion of using for loop is preferable without question. > In languages with powerful features, I think it is good to keep a > principle of local simplicity: use the simplest feature that lets you > express the problem without undue redundancy. (This rule of thumb is > in tension with the other rules that the parts of a given block of > code should be at roughly the same abstraction level: if you need to > do X, Y, Z in sequence, and only Y can be expressed simply with a > for-loop, it's better to keep a homogeneous style.) > > Cheers > Thanks again for looking so closely into my code. If I happen to meet you face-to-face I owe you a wine, beer or whatever you like best. :-) -- Best, Manfred > On Tue, Dec 27, 2016 at 10:40 AM, Manfred Lotz [email protected] > [ocaml_beginners] <[email protected]> wrote: > > > Hi there, > > I have added OCaml solutions for two simple tasks at Rosettacode. > > > > As a mere beginner in OCaml I don't want to add bad quality stuff at > > Rosettacode, and thus (worst case) in the end make people believe > > OCaml were a bad language. > > > > Therefore, I would be happy if some of the more knowledgeable people > > around here could check if the code I added is OK. > > > > http://rosettacode.org/wiki/History_variables#OCaml > > http://rosettacode.org/wiki/N%27th#OCaml > > > > > > Thanks a lot. > > > > Manfred > > > > > > ------------------------------------ > > Posted by: Manfred Lotz <[email protected]> > > ------------------------------------ > > > > Archives up to December 31, 2011 are also downloadable at > > http://www.connettivo.net/cntprojects/ocaml_beginners > > The archives of the very official ocaml list (the seniors' one) can > > be found at http://caml.inria.fr > > Attachments are banned and you're asked to be polite, avoid flames > > etc. ------------------------------------ > > > > Yahoo Groups Links > > > > > > > > --dD2lzK1KrONxj7pEIJRv84JevOUkAj0ow1ZvMZF Content-Type: text/html; charset=US-ASCII Content-Transfer-Encoding: 7bit <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> <html> <head> </head> <body style="background-color: #fff;"> <span style="display:none"> </span> <!--~-|**|PrettyHtmlStartT|**|-~--> <div id="ygrp-mlmsg" style="position:relative;"> <div id="ygrp-msg" style="z-index: 1;"> <!--~-|**|PrettyHtmlEndT|**|-~--> <div id="ygrp-text" > <p>Hi Gabriel,<br> First, thanks a lot for looking into this.<br> <br> On Tue, 27 Dec 2016 11:00:23 -0500<br> "Gabriel Scherer [email protected] [ocaml_beginners]"<br> <[email protected]> wrote:<br> <br> > Hi Manfred,<br> > <br> > Thanks for the interesting question. Having your code reviewed is an<br> > excellent way to make progress.<br> > <br> > For history variables, your code is fine, but you should consider<br> > using the Stack module instead of re-implementing yourself using<br> > mutable lists. <br> <br> You are right. I changed the implementation using the Stack module.<br> <br> > (Independently, when you have a record with a single<br> > mutable field, you could consider using the standard "ref" type<br> > instead; but sometimes having your own record type actually helps<br> > readability.)<br> > <br> <br> I chose a record because if something gets added (time stamp or so)<br> then it is just about adding a field. <br> <br> > For the enumeration of ordinal numbers, there are two things that I<br> > am not fond of in your code:<br> > <br> > 1. I think that "when" is a dangerous construction to use and its use<br> > should be de-emphasized, because it encourages code that relies on<br> > ordering in non-trivial way, diminishing a lot of the value of<br> > pattern-matching. The fact that, apart from the "when" clause, you<br> > are not using expressive patterns in any way is the sign of a code<br> > smell in my book. When I teach beginners, I ask them to only<br> > pattern-match on algebraic datatypes. I would encourage you to<br> > rewrite this part with an explicit "if ... else if ..." cascade.<br> > <br> <br> This is a very interesting comment. I wasn't aware of this. Thinking<br> about it (I will look into this more closely) it makes sense. So, I<br> chose your suggestion of a cascaded "if...else if...".<br> <br> > You could also decide to factorize the three cases with a function<br> > <br> > let non_th n =<br> > let d = n mod 10 in<br> > d >= 1 && d <= 3 && n mod 100 <> (10 + d) in<br> > <br> > 2. The creation of the array in "f" seems unnecessary to me. Using a<br> > "for" loop would result in simpler code, whose structure would be<br> > more directly apparent to readers.<br> > <br> <br> You are right, of course. I changed it accordingly. I also changed the<br> name of 'f' to 'show_numbers' which is surely a better name than<br> just 'f'.<br> <br> > (If you like functional pipelines with higher-order iterator, you may<br> > be pleased by the following reformulation of your code, that I think<br> > is still not as good as a for loop:<br> > <br> > Array.init ...<br> > |> Array.iter ...<br> > |> print_newline <br> > )<br> > <br> <br> I like it very much in some situations. But your suggestion of using<br> for loop is preferable without question.<br> <br> > In languages with powerful features, I think it is good to keep a<br> > principle of local simplicity: use the simplest feature that lets you<br> > express the problem without undue redundancy. (This rule of thumb is<br> > in tension with the other rules that the parts of a given block of<br> > code should be at roughly the same abstraction level: if you need to<br> > do X, Y, Z in sequence, and only Y can be expressed simply with a<br> > for-loop, it's better to keep a homogeneous style.)<br> > <br> > Cheers<br> > <br> <br> Thanks again for looking so closely into my code. If I happen to meet<br> you face-to-face I owe you a wine, beer or whatever you like best. :-)<br> <br> -- <br> Best, Manfred<br> <br> > On Tue, Dec 27, 2016 at 10:40 AM, Manfred Lotz [email protected]<br> > [ocaml_beginners] <[email protected]> wrote:<br> > <br> > > Hi there,<br> > > I have added OCaml solutions for two simple tasks at Rosettacode.<br> > ><br> > > As a mere beginner in OCaml I don't want to add bad quality stuff at<br> > > Rosettacode, and thus (worst case) in the end make people believe<br> > > OCaml were a bad language.<br> > ><br> > > Therefore, I would be happy if some of the more knowledgeable people<br> > > around here could check if the code I added is OK.<br> > ><br> > > http://rosettacode.org/wiki/History_variables#OCaml<br> > > http://rosettacode.org/wiki/N%27th#OCaml<br> > ><br> > ><br> > > Thanks a lot.<br> > ><br> > > Manfred<br> > ><br> > ><br> > > ------------------------------------<br> > > Posted by: Manfred Lotz <[email protected]><br> > > ------------------------------------<br> > ><br> > > Archives up to December 31, 2011 are also downloadable at<br> > > http://www.connettivo.net/cntprojects/ocaml_beginners<br> > > The archives of the very official ocaml list (the seniors' one) can<br> > > be found at http://caml.inria.fr<br> > > Attachments are banned and you're asked to be polite, avoid flames<br> > > etc. ------------------------------------<br> > ><br> > > Yahoo Groups Links<br> > ><br> > ><br> > ><br> > > <br> <br> </p> </div> <!--~-|**|PrettyHtmlStart|**|-~--> <div style="color: #fff; height: 0;">__._,_.___</div> <div style="clear:both"> </div> <div id="fromDMARC" style="margin-top: 10px;"> <hr style="height:2px ; border-width:0; color:#E3E3E3; background-color:#E3E3E3;"> Posted by: Manfred Lotz <[email protected]> <hr style="height:2px ; border-width:0; color:#E3E3E3; background-color:#E3E3E3;"> </div> <div style="clear:both"> </div> <table cellspacing=4px style="margin-top: 10px; margin-bottom: 10px; color: #2D50FD;"> <tbody> <tr> <td style="font-size: 12px; font-family: arial; font-weight: bold; padding: 7px 5px 5px;" > <a style="text-decoration: none; color: #2D50FD" href="https://groups.yahoo.com/neo/groups/ocaml_beginners/conversations/messages/14727;_ylc=X3oDMTJxNGR0YXJoBF9TAzk3MzU5NzE0BGdycElkAzQ5OTkxOTQEZ3Jwc3BJZAMxNzA1MDA2NzY0BG1zZ0lkAzE0NzI3BHNlYwNmdHIEc2xrA3JwbHkEc3RpbWUDMTQ4Mjg2Mjc3Mw--?act=reply&messageNum=14727">Reply via web post</a> </td> <td>•</td> <td style="font-size: 12px; font-family: arial; padding: 7px 5px 5px;" > <a href="mailto:[email protected]?subject=Re%3A%20%22ocaml_beginners%22%3A%3A%5B%5D%20Rosetta%20code%20added%20in%20OCaml%2C%20please%20check" style="text-decoration: none; color: #2D50FD;"> Reply to sender </a> </td> <td>•</td> <td style="font-size: 12px; font-family: arial; padding: 7px 5px 5px;"> <a href="mailto:[email protected]?subject=Re%3A%20%22ocaml_beginners%22%3A%3A%5B%5D%20Rosetta%20code%20added%20in%20OCaml%2C%20please%20check" style="text-decoration: none; color: #2D50FD"> Reply to group </a> </td> <td>•</td> <td style="font-size: 12px; font-family: arial; padding: 7px 5px 5px;" > <a href="https://groups.yahoo.com/neo/groups/ocaml_beginners/conversations/newtopic;_ylc=X3oDMTJlbDRlNTJqBF9TAzk3MzU5NzE0BGdycElkAzQ5OTkxOTQEZ3Jwc3BJZAMxNzA1MDA2NzY0BHNlYwNmdHIEc2xrA250cGMEc3RpbWUDMTQ4Mjg2Mjc3Mw--" style="text-decoration: none; color: #2D50FD">Start a New Topic</a> </td> <td>•</td> <td style="font-size: 12px; font-family: arial; padding: 7px 5px 5px;color: #2D50FD;" > <a href="https://groups.yahoo.com/neo/groups/ocaml_beginners/conversations/topics/14725;_ylc=X3oDMTM2ZHFrMjVxBF9TAzk3MzU5NzE0BGdycElkAzQ5OTkxOTQEZ3Jwc3BJZAMxNzA1MDA2NzY0BG1zZ0lkAzE0NzI3BHNlYwNmdHIEc2xrA3Z0cGMEc3RpbWUDMTQ4Mjg2Mjc3MwR0cGNJZAMxNDcyNQ--" style="text-decoration: none; color: #2D50FD;">Messages in this topic</a> (3) </td> </tr> </tbody> </table> <div id="megaphoneModule"> <hr style="height:2px ; border-width:0; color:#E3E3E3; background-color:#E3E3E3;"> <div> <div class="stream" style="margin-bottom:10px;"> <div style="background-color:white;"> <div class="sn-img" style="display:inline;"><img name="tn_file" style="padding:0px 10px;vertical-align:top;margin-top:5px;" src="https://s.yimg.com/ru/static/images/yg/img/megaphone/1464031581_phpFA8bON" height="82" width="82"></div> <div class="mod-txt" style="display:inline-block;"> <a rel="nofollow" name="sub_url" target="_blank" href="https://yho.com/1wwmgg" style="color:#0000FF;display:block;margin-left:5px;text-decoration:none;"><span style="font-size:15px;">Have you tried the highest rated email app?</span></a> <div style="max-width:530px;padding:2px 5px;">With 4.5 stars in iTunes, the Yahoo Mail app is the highest rated email app on the market. What are you waiting for? Now you can access all your inboxes (Gmail, Outlook, AOL and more) in one place. Never delete an email again with 1000GB of free cloud storage.</div> </div> </div> </div> </div> <hr style="height:2px ; border-width:0; color:#E3E3E3; background-color:#E3E3E3;"> </div> <!------- Start Nav Bar ------> <div id="ygrp-grfd" style="font-family: Verdana; font-size: 12px; padding: 15px 0;"> <!-- |**|begin egp html banner|**| --> Archives up to December 31, 2011 are also downloadable at <a href="http://www.connettivo.net/cntprojects/ocaml_beginners">http://www.connettivo.net/cntprojects/ocaml_beginners</a><BR> The archives of the very official ocaml list (the seniors' one) can be found at <a href="http://caml.inria.fr">http://caml.inria.fr</a><BR> Attachments are banned and you're asked to be polite, avoid flames etc. <!-- |**|end egp html banner|**| --> </div> <!-- |**|begin egp html banner|**| --> <div id="ygrp-vital" style="background-color: #f2f2f2; font-family: Verdana; font-size: 10px; margin-bottom: 10px; padding: 10px;"> <span id="vithd" style="font-weight: bold; color: #333; text-transform: uppercase; "><a href="https://groups.yahoo.com/neo/groups/ocaml_beginners/info;_ylc=X3oDMTJlYzE2djk1BF9TAzk3MzU5NzE0BGdycElkAzQ5OTkxOTQEZ3Jwc3BJZAMxNzA1MDA2NzY0BHNlYwN2dGwEc2xrA3ZnaHAEc3RpbWUDMTQ4Mjg2Mjc3Mw--" style="text-decoration: none;">Visit Your Group</a></span> <ul style="list-style-type: none; margin: 0; padding: 0; display: inline;"> <li style="border-right: 1px solid #000; font-weight: 700; display: inline; padding: 0 5px; margin-left: 0;"> <span class="cat"><a href="https://groups.yahoo.com/neo/groups/ocaml_beginners/members/all;_ylc=X3oDMTJmMzFyc2dmBF9TAzk3MzU5NzE0BGdycElkAzQ5OTkxOTQEZ3Jwc3BJZAMxNzA1MDA2NzY0BHNlYwN2dGwEc2xrA3ZtYnJzBHN0aW1lAzE0ODI4NjI3NzM-" style="text-decoration: none;">New Members</a></span> <span class="ct" style="color: #ff7900;">1</span> </li> </ul> </div> <div id="ft" style="font-family: Arial; font-size: 11px; margin-top: 5px; padding: 0 2px 0 0; clear: both;"> <a href="https://groups.yahoo.com/neo;_ylc=X3oDMTJkOGdzY2hzBF9TAzk3MzU5NzE0BGdycElkAzQ5OTkxOTQEZ3Jwc3BJZAMxNzA1MDA2NzY0BHNlYwNmdHIEc2xrA2dmcARzdGltZQMxNDgyODYyNzcz" style="float: left;"><img src="http://l.yimg.com/ru/static/images/yg/img/email/new_logo/logo-groups-137x15.png" height="15" width="137" alt="Yahoo! Groups" style="border: 0;"/></a> <div style="color: #747575; float: right;"> • <a href="https://info.yahoo.com/privacy/us/yahoo/groups/details.html" style="text-decoration: none;">Privacy</a> • <a href="mailto:[email protected]?subject=Unsubscribe" style="text-decoration: none;">Unsubscribe</a> • <a href="https://info.yahoo.com/legal/us/yahoo/utos/terms/" style="text-decoration: none;">Terms of Use</a> </div> </div> <br> <!-- |**|end egp html banner|**| --> </div> <!-- ygrp-msg --> <!-- Sponsor --> <!-- |**|begin egp html banner|**| --> <div id="ygrp-sponsor" style="width:160px; float:right; clear:none; margin:0 0 25px 0; background: #fff;"> <!-- Start Recommendations --> <div id="ygrp-reco"> </div> <!-- End Recommendations --> </div> <!-- |**|end egp html banner|**| --> <div style="clear:both; color: #FFF; font-size:1px;">.</div> </div> <img src="http://geo.yahoo.com/serv?s=97359714/grpId=4999194/grpspId=1705006764/msgId=14727/stime=1482862773" width="1" height="1"> <br> <img src="http://y.analytics.yahoo.com/fpc.pl?ywarid=515FB27823A7407E&a=10001310322279&js=no&resp=img&cf12=CP" width="1" height="1"> <div style="color: #fff; height: 0;">__,_._,___</div> <!--~-|**|PrettyHtmlEnd|**|-~--> </body> <!--~-|**|PrettyHtmlStart|**|-~--> <head> <style type="text/css"> <!-- #ygrp-mkp { border: 1px solid #d8d8d8; font-family: Arial; margin: 10px 0; padding: 0 10px; } #ygrp-mkp hr { border: 1px solid #d8d8d8; } #ygrp-mkp #hd { color: #628c2a; font-size: 85%; font-weight: 700; line-height: 122%; margin: 10px 0; } #ygrp-mkp #ads { margin-bottom: 10px; } #ygrp-mkp .ad { padding: 0 0; } #ygrp-mkp .ad p { margin: 0; } #ygrp-mkp .ad a { color: #0000ff; text-decoration: none; } #ygrp-sponsor #ygrp-lc { font-family: Arial; } #ygrp-sponsor #ygrp-lc #hd { margin: 10px 0px; font-weight: 700; font-size: 78%; line-height: 122%; } #ygrp-sponsor #ygrp-lc .ad { margin-bottom: 10px; padding: 0 0; } #actions { font-family: Verdana; font-size: 11px; padding: 10px 0; } #activity { background-color: #e0ecee; float: left; font-family: Verdana; font-size: 10px; padding: 10px; } #activity span { font-weight: 700; } #activity span:first-child { text-transform: uppercase; } #activity span a { color: #5085b6; text-decoration: none; } #activity span span { color: #ff7900; } #activity span .underline { text-decoration: underline; } .attach { clear: both; display: table; font-family: Arial; font-size: 12px; padding: 10px 0; width: 400px; } .attach div a { text-decoration: none; } .attach img { border: none; padding-right: 5px; } .attach label { display: block; margin-bottom: 5px; } .attach label a { text-decoration: none; } blockquote { margin: 0 0 0 4px; } .bold { font-family: Arial; font-size: 13px; font-weight: 700; } .bold a { text-decoration: none; } dd.last p a { font-family: Verdana; font-weight: 700; } dd.last p span { margin-right: 10px; font-family: Verdana; font-weight: 700; } dd.last p span.yshortcuts { margin-right: 0; } div.attach-table div div a { text-decoration: none; } div.attach-table { width: 400px; } div.file-title a, div.file-title a:active, div.file-title a:hover, div.file-title a:visited { text-decoration: none; } div.photo-title a, div.photo-title a:active, div.photo-title a:hover, div.photo-title a:visited { text-decoration: none; } div#ygrp-mlmsg #ygrp-msg p a span.yshortcuts { font-family: Verdana; font-size: 10px; font-weight: normal; } .green { color: #628c2a; } .MsoNormal { margin: 0 0 0 0; } o { font-size: 0; } #photos div { float: left; width: 72px; } #photos div div { border: 1px solid #666666; height: 62px; overflow: hidden; width: 62px; } #photos div label { color: #666666; font-size: 10px; overflow: hidden; text-align: center; white-space: nowrap; width: 64px; } #reco-category { font-size: 77%; } #reco-desc { font-size: 77%; } .replbq { margin: 4px; } #ygrp-actbar div a:first-child { /* border-right: 0px solid #000;*/ margin-right: 2px; padding-right: 5px; } #ygrp-mlmsg { font-size: 13px; font-family: Arial, helvetica,clean, sans-serif; *font-size: small; *font: x-small; } #ygrp-mlmsg table { font-size: inherit; font: 100%; } #ygrp-mlmsg select, input, textarea { font: 99% Arial, Helvetica, clean, sans-serif; } #ygrp-mlmsg pre, code { font:115% monospace; *font-size:100%; } #ygrp-mlmsg * { line-height: 1.22em; } #ygrp-mlmsg #logo { padding-bottom: 10px; } #ygrp-msg p a { font-family: Verdana; } #ygrp-msg p#attach-count span { color: #1E66AE; font-weight: 700; } #ygrp-reco #reco-head { color: #ff7900; font-weight: 700; } #ygrp-reco { margin-bottom: 20px; padding: 0px; } #ygrp-sponsor #ov li a { font-size: 130%; text-decoration: none; } #ygrp-sponsor #ov li { font-size: 77%; list-style-type: square; padding: 6px 0; } #ygrp-sponsor #ov ul { margin: 0; padding: 0 0 0 8px; } #ygrp-text { font-family: Georgia; } #ygrp-text p { margin: 0 0 1em 0; } #ygrp-text tt { font-size: 120%; } #ygrp-vital ul li:last-child { border-right: none !important; } --> </style> </head> <!--~-|**|PrettyHtmlEnd|**|-~--> </html> <!-- end group email --> --dD2lzK1KrONxj7pEIJRv84JevOUkAj0ow1ZvMZF--