Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm currently attempting to learn Ocaml, and I'm working thought the Project Euler problems to do so. Here's some code I knocked together for problem 10

(* 
The sum of the primes below 10 is 2 + 3 + 5 + 7 = 17.

Find the sum of all the primes below two million.

MODIFYING CODE FROM PROBLEM 7 

*) 


(* first thing we are going to do is write a bit of code that checks to see if a number is prime   *) 

let rec isPrimeRec number start = if (start*start)>number then 1 else  if number mod start = 0 then -1 else isPrimeRec number (start+1);;

let i = ref 2;;
let sum = ref 0 in 
while !i <2000000 do 
if (isPrimeRec !i 2) = 1 then 
begin  
sum:= !sum + !i; i:= !i + 1; 
Printf.printf "%d is prime, it is prime number %d\n" !i !sum; 
end 
else 
i:= !i + 1
done;;

let temp = ref 0;;
temp:= isPrimeRec 19 3;
Printf.printf "The value is %d\n" !temp

Now - I almost purposesly didn't right this to be efficient algorithmically (for example I am aware that prime numbers can't be even) and there are a few other things that i would change for efficiency - but I'm interested in style feedback - so I'd like the code critiqued much more on the level of "In ocaml, one would normaly bracket expression X for readability" or "Ocaml let's you use this, clearly syntax instead" - rather than "it's a property of prime numbers that"

share|improve this question

2 Answers 2

up vote 3 down vote accepted

Here are some comments and an example rewrite of your code.

Indentation and line breaks are not idiomatic to OCaml, it's good practice in any language.

A more intuitive type for is_prime would be to have only one argument, so let's encapsulate is_prime_rec:

let is_prime =
  let rec is_prime_rec number start =
    if start * start > number then true (* OCaml provides a type `bool`, distinct from `int`, so it's better to return `true` instead of `1`. *)
    else if number mod start = 0 then false
    else is_prime_rec number (start+1) in
  fun n -> is_prime_rec n 2;;

which yields val is_prime : int -> bool = <fun>.

You know the number of iterations in the loop, so better use for rather than while. This also avoid manually incrementing i.

let sum = ref 0 in
for i = 2 to 2_000_000 do (* detail: OCaml parses `2_000_000` as `2000000`, which is more readable. *)
  if is_prime i then (* no need for parentheses around the if clause *)
    begin
      sum:= !sum + i;
      Printf.printf "%d is prime, it is prime number %d\n" i !sum;
    end
done;;

I can't help but add an algorithmic remark: consider using a sieve.

share|improve this answer

Hello from Haskell world ;) Consider using some kind of memoization. For example with help of infinte cached sequence through Seq.cache in F#:

let rec is_prime x =
    primes
    |> Seq.takeWhile (fun p -> p*p <= x)
    |> Seq.exists (fun p -> x % p = 0)
    |> not
and primes = 
    seq {
        yield 2;
        yield 3;
        yield!
            Seq.initInfinite (fun i -> i) 
            |> Seq.skipWhile (fun i -> i <= 3)
            |> Seq.filter is_prime
    }
    |> Seq.cache
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.