HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Three implementations of mergesort in F#

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
threeimplementationsmergesort

Problem

I would appreciate some quick comments on this basic mergesort code.
Am I missing a big block in the langage?

First solution

open System
open System.Windows
open System.Collections.Generic

let shuffle (l:'a array) = 
   let ileft = LinkedList(seq { 0 .. (l.Length - 1)})
   let rec pick (ar:'a array)  r = 
      match ileft.Count with | 0 -> r
                             | n -> let ik =   ileft |> Seq.nth (rnd.Next(n))
                                    ileft.Remove(ik) |> ignore
                                    pick ar (ar.[ik]::r)
   pick l  []

let rec merge (ar1:'a array) (ar2:'a array)  = 
   let rec index (islastfromAr1, ilast, jlast) = seq {
      let inext, jnext = ilast + 1, jlast + 1 
      match inext  let indexnext = if ar1.[inext]  let indexnext = (false, ilast, jnext)
                        yield  Some(indexnext)
                        yield! index indexnext 
      | true , false -> let indexnext = (true, inext, jlast)
                        yield  Some(indexnext)
                        yield! index indexnext 
      | false, false -> yield  None
   }
   let mergeindex = index (false, -1, -1)
   [for (formar1, i,j) in  mergeindex |> Seq.choose (id) -> if formar1 then ar1.[i] else ar2.[j] ]

and mergesort  = function 
   | [| |]    -> [||]
   | [|a|]    -> [|a|]
   | ar       -> let ar1 = ar.[0 .. ar.Length / 2 - 1]
                 let ar2 = ar.[ar.Length / 2 .. ar.Length - 1]
                 merge (mergesort ar1) (mergesort ar2)  |> List.toArray
let testval = ( [|1 .. 100|] |> shuffle |> List.toArray)                 
let test4 = mergesort testval


Second solution

a shorter, mutable state version of it

```
let rec mergemutable (ar1:'a array) (ar2:'a array) =
let inext, jnext = ref 0 , ref 0

[ for k in [1 .. (ar1.Length + ar2.Length)] ->
match !inext if ar1.[!inext] jnext := !jnext + 1
ar2.[!jnext - 1]
| true , false -> inext := !inext + 1
ar1.[!inext - 1 ]

Solution

I'm just learning F# myself, but here is what I currently notice:

-
Your merge function is very large. It should be split into multiple small functions that are responsible for a single action.

-
match is preferred to if:

if ar1.[inext] < ar2.[jnext] then
    (true, inext, jlast)
else
    (false, ilast, jnext)


can be written as:

match ar1.[inext] with
| i when i  (true, inext, jlast)
| _ -> (false, ilast, jnext)


  • islastfromAr1 is never used. This parameter can probably be removed.

Code Snippets

if ar1.[inext] < ar2.[jnext] then
    (true, inext, jlast)
else
    (false, ilast, jnext)
match ar1.[inext] with
| i when i < ar2.[jnext] -> (true, inext, jlast)
| _ -> (false, ilast, jnext)

Context

StackExchange Code Review Q#12643, answer score: 3

Revisions (0)

No revisions yet.