dibs: I will implement exercise atbash-cipher #72

dvberkel · 2016-10-07T11:24:05Z

Work in progress

dvberkel · 2016-10-07T14:07:38Z

@stevejb71 I would love you feedback on this. I might have gone overboard and missing some Core library functions for this exercises.

Besides that, two test cases fail, and I can't figure out why? Would appreciate any pointers you might have.

stevejb71 · 2016-10-08T00:10:01Z

Happy to give feedback. I'll take a look later today (Hong Kong time).

stevejb71 · 2016-10-08T03:37:09Z

exercises/atbash-cipher/example.ml

+  | (_, []) -> []
+  | (_, _::xs) -> drop (n-1) xs
+
+let rec group n l =


This has a bug: if the length of the list is 0 then it returns [[]], it should return []. That should fix the broken tests.

Nice catch! Will definitely look into that.

I should have tested these methods as well, but I didn't want to enforce an implementation direction.

You were right not to publish tests for these internal function as they are implementation details. I just run adhoc tests in utop in this case.

stevejb71 · 2016-10-08T03:46:20Z

exercises/atbash-cipher/example.ml

+  | 'a' .. 'z' | '0' .. '9' -> true
+  | _ -> false
+
+let explode s =


The signature of this function is not immediately obvious (at least to me!). A Haskell convention is to provide type signatures on top level functions in a module - that gives useful, and compiler checked, documentation. I'd favour the same in Ocaml code.

I was not sure what the OCaml convention is. I myself would rather provide type signatures for everything, although this takes away the benefit of type inference.

I will provide the type signatures.

There a good summary of the benefits of type signatures on stackoverflow
http://stackoverflow.com/questions/23401698/what-is-a-good-reason-to-use-a-type-signature-for-functions-when-the-compiler-ca

I have troubles providing an inline signature because of the use of function. Because it is an implementation detail, I can't provide the signature in the mli-file.

Do you have a tip?

I don't know of a way of providing a signature in this case. Either rewrite if you think adding a signature is worth it, or leave as is.

stevejb71 · 2016-10-08T03:47:02Z

exercises/atbash-cipher/example.ml

+  else
+    (take n l) :: (group n (drop n l))
+
+let encode ?block_size:(block_size = 5) word =


Perhaps text is better than word - the input can be more than 1 word.

Also, the optional parameter can be provided more simply:
?(block_size = 5)

I agree, text is a better name.

I will adopt your improvement for the optional parameter

stevejb71 · 2016-10-08T03:52:00Z

exercises/atbash-cipher/example.ml

+  let filtered_characters = List.filter is_encodable characters in
+  let groups = group block_size filtered_characters in
+  let preprocessed_words = List.map implode groups in
+  let words = List.map (fun (word) -> (String.map substitute word)) preprocessed_words in


You might as well take advantage of currying here:
let words = List.map (String.map substitute) preprocessed_words in

In any event, you don't need the brackets around the word parameter.

Nice! That is a very good suggestion

stevejb71 · 2016-10-08T03:54:24Z

exercises/atbash-cipher/atbash_cipher.mli

@@ -0,0 +1,5 @@
+(* Encoding from English to atbash cipher *)


The default value block_size is 5 according to the README. You might want to specify that in the comment. (I don't think there is anyway to specify a default value for a parameter in the .mli code.)

stevejb71 · 2016-10-08T03:55:41Z

exercises/atbash-cipher/test.ml

+
+let ae exp got _test_ctxt = assert_equal ~printer:String.to_string exp got
+
+let tests =


I think you should add a couple of tests with a different block size.

stevejb71 · 2016-10-08T04:05:03Z

exercises/atbash-cipher/example.ml

@@ -0,0 +1,57 @@
+let substitute = function


You are not using Core here. That would make implementation a touch easier (no need to rewrite drop and take).

Do you prefer not to use it, or was it an oversight?

It was an oversight. Implementing take and drop distracts from the actual exercise.

Besides that, seeing that I made an incorrect implementation, I am in favour of _drop_ping (pun intended) the custom implementation for the ones in Core

stevejb71 · 2016-10-08T04:08:58Z

@dvberkel - I added a few comments - one of them includes how to fix the tests. Let me know when you are done making the changes you accept, and I can re-review.

dvberkel · 2016-10-08T06:21:01Z

@stevejb71 thanks for your review. They are very worthwhile. I will improve the code with your suggestions later today. I will drop you a line when I am finished

dvberkel · 2016-10-08T09:29:11Z

@stevejb71 I updated the exercise with your suggestion. I couldn't provide a type signature for the substitute function. The function keyword and my suggestion kept conflicting. Do you have a solution?

stevejb71 · 2016-10-08T13:24:25Z

exercises/atbash-cipher/example.ml

@@ -1,3 +1,5 @@
+open Core


open Core.Std instead.

Char.code becomes Char.to_int
Char.chr becomes Char.of_int_exn (functions throwing exceptions are documented like this consistently in Core).

stevejb71 · 2016-10-08T13:24:52Z

exercises/atbash-cipher/example.ml

    [l]
  else
-    (take n l) :: (group n (drop n l))
+    (Core_list.take l n) :: (group n (Core_list.drop l n))


If you open Core.Std, then you have List.take and List.drop instead.

stevejb71 · 2016-10-08T13:27:13Z

I added a few comments to your last commit, but they are just suggestions for improvement in the non-public part of the code.

Given you have the tests passing now, I'd be happy to merge if you can just add a few tests with different block sizes (I think that is important as it is on the public interface).

dvberkel · 2016-10-09T08:25:22Z

I adopted your suggestion. Even though the code is not part of the public interface, it is important to show idiomatic code.

dvberkel · 2016-10-13T11:32:18Z

@stevejb71 How are these changes looking? Care to give your go on them?

dvberkel · 2016-10-13T14:07:31Z

I rebased in order to incorporate the latest changes

stevejb71

Only remaining issuesl is about a test on a non standard block size. I saw a conversation about this on the canonical data, was it decided this test was not worth adding?

dvberkel · 2016-10-14T04:47:15Z

I opened that issue and it received not a lot of attention. The only comment was that it w as OK the way it is. That is why I did not add other tests.

We could decide otherwise and still test other block sizes. @stevejb71 what do you think?

stevejb71 · 2016-10-14T14:59:27Z

I think we should add a test. 1 seems a bit of an edge case which would be good to test.

dvberkel · 2016-10-16T17:33:06Z

I will add a test with a different block size

Two test cases fail, but I am uncertain why. To my, even on close inspection they seem the same. But I am dyslectic, so what do I know

There is one outstanding improvement. I.e. provide type signature for substitute. I had troubles providing the correct syntax with the `function` keyword

@stevejb71

As per @stevejb71 suggestion

dvberkel · 2016-10-16T18:00:37Z

@stevejb71 added a test with a different block size

stevejb71 suggested changes Oct 8, 2016

View reviewed changes

stevejb71 reviewed Oct 8, 2016

View reviewed changes

dvberkel force-pushed the atbash-cipher branch from 3ec266d to 22a117e Compare October 9, 2016 08:23

dvberkel force-pushed the atbash-cipher branch from 22a117e to e67088a Compare October 13, 2016 14:06

stevejb71 reviewed Oct 14, 2016

View reviewed changes

dvberkel added 5 commits October 16, 2016 19:54

Create atbash-cipher exercise

fef6ac6

Two test cases fail, but I am uncertain why. To my, even on close inspection they seem the same. But I am dyslectic, so what do I know

Register atbash exercise

bc4a97d

Adopt suggestion by stevejb71

15a3225

There is one outstanding improvement. I.e. provide type signature for substitute. I had troubles providing the correct syntax with the `function` keyword

open Core.Std instead of Core

f12c893

As per @stevejb71 suggestion

Add test with different block size

3bb8477

dvberkel force-pushed the atbash-cipher branch from e67088a to 3bb8477 Compare October 16, 2016 18:00

stevejb71 merged commit aadf01d into master Oct 18, 2016

dvberkel deleted the atbash-cipher branch October 19, 2016 07:50

jtigger mentioned this pull request Nov 2, 2016

README of atbash-ciper mentions group size, but there are no tests for it exercism/problem-specifications#398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dibs: I will implement exercise atbash-cipher #72

dibs: I will implement exercise atbash-cipher #72

dvberkel commented Oct 7, 2016

dvberkel commented Oct 7, 2016

stevejb71 commented Oct 8, 2016

stevejb71 Oct 8, 2016

dvberkel Oct 8, 2016

stevejb71 Oct 8, 2016

stevejb71 Oct 8, 2016

dvberkel Oct 8, 2016

stevejb71 Oct 8, 2016

dvberkel Oct 8, 2016

stevejb71 Oct 8, 2016

stevejb71 Oct 8, 2016 •

edited

dvberkel Oct 8, 2016

stevejb71 Oct 8, 2016

dvberkel Oct 8, 2016

stevejb71 Oct 8, 2016 •

edited

stevejb71 Oct 8, 2016

dvberkel Oct 8, 2016

stevejb71 Oct 8, 2016 •

edited

dvberkel Oct 8, 2016 •

edited

stevejb71 commented Oct 8, 2016

dvberkel commented Oct 8, 2016

dvberkel commented Oct 8, 2016 •

edited

stevejb71 Oct 8, 2016

stevejb71 Oct 8, 2016

stevejb71 commented Oct 8, 2016

dvberkel commented Oct 9, 2016

dvberkel commented Oct 13, 2016

dvberkel commented Oct 13, 2016

stevejb71 left a comment

dvberkel commented Oct 14, 2016

stevejb71 commented Oct 14, 2016

dvberkel commented Oct 16, 2016

dvberkel commented Oct 16, 2016


		let ae exp got _test_ctxt = assert_equal ~printer:String.to_string exp got

		let tests =

dibs: I will implement exercise atbash-cipher #72

dibs: I will implement exercise atbash-cipher #72

Conversation

dvberkel commented Oct 7, 2016

dvberkel commented Oct 7, 2016

stevejb71 commented Oct 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevejb71 Oct 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevejb71 Oct 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevejb71 Oct 8, 2016 • edited

Choose a reason for hiding this comment

dvberkel Oct 8, 2016 • edited

Choose a reason for hiding this comment

stevejb71 commented Oct 8, 2016

dvberkel commented Oct 8, 2016

dvberkel commented Oct 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevejb71 commented Oct 8, 2016

dvberkel commented Oct 9, 2016

dvberkel commented Oct 13, 2016

dvberkel commented Oct 13, 2016

stevejb71 left a comment

Choose a reason for hiding this comment

dvberkel commented Oct 14, 2016

stevejb71 commented Oct 14, 2016

dvberkel commented Oct 16, 2016

dvberkel commented Oct 16, 2016

stevejb71 Oct 8, 2016 •

edited

stevejb71 Oct 8, 2016 •

edited

stevejb71 Oct 8, 2016 •

edited

dvberkel Oct 8, 2016 •

edited

dvberkel commented Oct 8, 2016 •

edited