4

I am learning Jason Hickey's Introduction to Objective Caml.

There is an exercise like this:

Exercise 4.3 Suppose we have a crypto-system based on the following substitution cipher, where each plain letter is encrypted according to the following table.

Plain     | A B C D
--------------------
Encrypted | C A D B

For example, the string BAD would be encrypted as ACB.

Write a function check that, given a plaintext string s1 and a ciphertext string s2, returns true if, and only if, s2 is the ciphertext for s1. Your function should raise an exception if s1 is not a plaintext string. You may wish to refer to the string operations on page 8. How does your code scale as the alphabet gets larger? [emphasis added]


Basically, I wrote two functions with might-be-stupid-naive ways for this exercise.

I would like to ask for advice on my solutions first.

Then I would like to ask for hints for the scaled solution as highlighted in the exercise.


Using if else

let check_cipher_1 s1 s2 = 
    let len1 = String.length s1 in
        let len2 = String.length s2 in              
            if len1 = len2 then
                    let rec check pos =
                        if pos = -1 then
                            true
                        else
                            let sub1 = s1.[pos] in
                                let sub2 = s2.[pos] in
                                    match sub1 with
                                        | 'A' -> (match sub2 with
                                                    |'C' -> check (pos-1)
                                                    | _ -> false)
                                        | 'B' -> (match sub2 with
                                                    |'A' -> check (pos-1)
                                                    | _ -> false)
                                        | 'C' -> (match sub2 with
                                                    |'D' -> check (pos-1)
                                                    | _ -> false)
                                        | 'D' -> (match sub2 with
                                                    |'B' -> check (pos-1)
                                                    | _ -> false)
                                        | _ -> false;
                                            in
                                                check (len1-1)
            else
                false

Using pure match everywhere

let check_cipher_2 s1 s2 = 
    let len1 = String.length s1 in
        let len2 = String.length s2 in
            match () with
                | () when len1 = len2 -> 
                        let rec check pos =
                            match pos with
                                | -1 -> true
                                | _ -> 
                                    let sub1 = s1.[pos] in
                                        let sub2 = s2.[pos] in
                                            (*http://stackoverflow.com/questions/257605/ocaml-match-expression-inside-another-one*)
                                            match sub1 with
                                                | 'A' -> (match sub2 with
                                                            |'C' -> check (pos-1)
                                                            | _ -> false)
                                                | 'B' -> (match sub2 with
                                                            |'A' -> check (pos-1)
                                                            | _ -> false)
                                                | 'C' -> (match sub2 with
                                                            |'D' -> check (pos-1)
                                                            | _ -> false)
                                                | 'D' -> (match sub2 with
                                                            |'B' -> check (pos-1)
                                                            | _ -> false)
                                                | _ -> false
                                                    in
                                                        check (len1-1)
                | () -> false

Ok. The above two solutions are similar.

I produced these two, because in here http://www.quora.com/OCaml/What-is-the-syntax-for-nested-IF-statements-in-OCaml, some people say that if else is not prefered.

This is essentially the first time I ever wrote a not-that-simple function in my whole life. So I am really hungry for suggestions here.

For exmaple,

  • how can I improve these solutions?
  • should I prefer match over if else?
  • Am I designing the rec or use the rec correctly?
  • if that in check (len1-1) correct?

Scale it

The exercise asks How does your code scale as the alphabet gets larger?. I really don't have a clue for now. In Java, I would say I will have a map, then for each char in s1, I am looking s2 for the according char and to see whether it is the value in the map.

Any suggestions on this?

Community
  • 1
  • 1
Jackson Tale
  • 25,428
  • 34
  • 149
  • 271
  • 2
    Just to be specific, the main problem that makes your code look weird is that you're indenting after every `let`. You definitely don't want to do that. Some suggestions for formatting OCaml can be found in the [Caml Programming Guidelines](http://caml.inria.fr/resources/doc/guides/guidelines.en.html)--see especially the section "how to indent let ... in constructs". – Jeffrey Scofield Jan 15 '13 at 18:38
  • @JeffreyScofield this is also what need. – Jackson Tale Jan 15 '13 at 20:31

3 Answers3

5

Here's a simple solution:

let tr = function
  | 'A' -> 'C'
  | 'B' -> 'A'
  | 'C' -> 'D'
  | 'D' -> 'B'
  | _ -> failwith "not a plaintext"

let check ~tr s1 s2 = (String.map tr s1) = s2

check ~tr "BAD" "ACD"

you can add more letters by composing with tr. I.e.

let comp c1 c2 x = try (c1 x) with _ -> (c2 x)
let tr2 = comp tr (function | 'X' -> 'Y')
rgrinberg
  • 9,638
  • 7
  • 27
  • 44
  • ~tr is a labeled argument. hickey explains in his book rather early(section 3.3) so you should review it if you forgot. In this particular example it's the translator function per character. We'd like to pass that function in instead of hard coding so that we can build different translation functions. Example: `check ~tr:(comp (function 'X' -> 'Y') (function 'A' -> 'B')) "XAA" "YBB"` – rgrinberg Jan 15 '13 at 20:55
  • yes, I still remember `~something` is a labeled argument. But still, I am not sure I understand your point here. `~tr` is not a label here? – Jackson Tale Jan 16 '13 at 09:15
  • Ok, I revised `~label` in 3.3 as you mentioned. So, if we defined `tr` before, why we need `~tr' in `let check ~tr s1 s2 = (String.map tr s1) = s2`? can we just use `let check s1 s2 = (String.map tr s1) = s2`? – Jackson Tale Jan 16 '13 at 09:27
3
  • how can I improve these solutions?

You misuse indentation which makes the program much harder to read. Eliminating unnecessary tabs and move check to outer scope for readability:

let check_cipher_1 s1 s2 = 
    let rec check pos =
        if pos = -1 then
            true
        else
            let sub1 = s1.[pos] in
            let sub2 = s2.[pos] in
            match sub1 with
            | 'A' -> (match sub2 with
                      |'C' -> check (pos-1)
                      | _ -> false)
            | 'B' -> (match sub2 with
                      |'A' -> check (pos-1)
                      | _ -> false)
            | 'C' -> (match sub2 with
                      |'D' -> check (pos-1)
                      | _ -> false)
            | 'D' -> (match sub2 with
                      |'B' -> check (pos-1)
                      | _ -> false)
            | _ -> false in
    let len1 = String.length s1 in
    let len2 = String.length s2 in              
    if len1 = len2 then
            check (len1-1)
    else false
  • should I prefer match over if else?

It depends on situations. If pattern matching is superficial as you demonstrate in the 2nd function (match () with | () when len1 = len2) then it brings no value compared to a simple if/else construct. If you pattern match on values, it is better than if/else and potentially shorter when you make use of advanced constructs. For example, you can shorten the function by matching on tuples:

let check_cipher_1 s1 s2 =  
    let rec check pos =
        if pos = -1 then
           true
        else
            match s1.[pos], s2.[pos] with
            | 'A', 'C' | 'B', 'A' 
            | 'C', 'D' | 'D', 'B' -> check (pos-1)
            | _ -> false in
    let len1 = String.length s1 in
    let len2 = String.length s2 in 
    len1 = len2 && check (len1 - 1)

Here we also use Or pattern to group patterns having the same output actions and replace an unnecessary if/else block by &&.

  • Am I designing the rec or use the rec correctly?
  • if that in check (len1-1) correct?

Your function looks nice. There's no better way than testing with a few inputs on OCaml top-level.

Scale it

The number of patterns grows linearly with the size of the alphabet. It's pretty nice IMO.

pad
  • 41,040
  • 7
  • 92
  • 166
  • thanks, what do you mean by " It's pretty nice"? you mean the existing solution grows linearly with the size of the alphabet is nice? – Jackson Tale Jan 15 '13 at 18:12
  • I mean you don't have to add much code when the alphabet grows. Of course, a generic solution is to use a `map` as you described. – pad Jan 15 '13 at 18:14
  • about the `indentation`, should you use a `indent` before the `match expression` in your `2nd segment` of code? – Jackson Tale Jan 16 '13 at 09:29
  • Yes, I should. It's fixed now. – pad Jan 16 '13 at 09:56
2

The simplest solution seems to be to just cipher the text and compare the result:

let cipher_char = function
   | 'A' -> 'C'
   | 'B' -> 'A'
   | 'C' -> 'D'
   | 'D' -> 'B'
   | _ -> failwith "cipher_char"
let cipher = String.map cipher_char
let check_cipher s1 s2 = (cipher s1 = s2)

The cipher_char function scales linearly with the size of the alphabet. To make it a bit more compact and generic you could use a lookup table of some form, e.g.

(* Assume that only letters are needed *)
let cipher_mapping = "CADB"
let cipher_char c =
   try cipher_mapping.[Char.code c - Char.code 'A']
   with Invalid_argument _ -> failwith "cipher_char"
Andreas Rossberg
  • 34,518
  • 3
  • 61
  • 72
  • for `cipher s1 = s2`, basically I can understand in this way that `cipher s1` returns a list and we then see whether this list equals to `s2` or not, right? – Jackson Tale Jan 16 '13 at 09:55
  • Also could you please explain more about your second segment of code? what is the purpose of `try cipher_mapping.[Char.code c - Char.code 'A']`? – Jackson Tale Jan 16 '13 at 09:59
  • Yes, except that `cipher s1` returns a string not a list, the cipher of s1. In the second part, `cipher_mapping.[...]` looks up the character corresponding to `c` in `cipher_mapping`, which is assumed to be a string of cipher characters, one for each clear text character, the first one being for `'A'`. It hence can be indexed by character codes. The `try ... with` around it just is for exception handling, in case the string access is out of bounds (i.e., the original character isn't supported). – Andreas Rossberg Jan 16 '13 at 13:41