views:

479

answers:

2

Hi, I am a C++ programmer, I wrote this code to see if I can think functionally :) Any hints to improve it ?

(define (append listOne listTwo)
  (cond
    ((null? listOne) listTwo)
    (else (cons (car listOne) (append (cdr listOne) listTwo)))))

(define (merge listOne listTwo)
  (cond
    ((null? listOne) listTwo)
    ((null? listTwo) listOne)
    ((< (car listOne) (car listTwo))
     (append (cons (car listOne) '())
             (merge (cdr listOne) listTwo)))
    ((= (car listOne) (car listTwo))
     (append (cons (car listOne) '())
             (merge (cdr listOne) listTwo)))
    (else (append (cons (car listTwo) '())
                  (merge listOne (cdr listTwo))))))

(define (mergesort lis)
  (cond
    ((null? lis) '())
    ((null? (cdr lis)) lis)
    (else (merge (cons (car lis) '())
                 (mergesort (cdr lis))))))

(mergesort '(99 77 88 66 44 55 33 11 22 0))
+2  A: 

There's only one small improvement that I see:

(append (cons (car listTwo) '())
              (merge listOne (cdr listTwo))))

can everywhere be simplified to

(cons (car listTwo)
      (merge listOne (cdr listTwo)))

I think you were thinking of something like (in Python-esque syntax):

[car(listTwo)] + merge(listOne, cdr(listTwo))

But cons adds an item directly to the front of a list, like a functional push, so it's like the following code:

push(car(listTwo), merge(listOne, cdr(listTwo)))

Ultimately the extra append only results in double cons cell allocation for each item, so it's not a big deal.

Also, I think you might get better performance if you made mergesort fancier so that it maintains the list length and sorts both halves of the list at each step. This is probably not appropriate for a learning example like this, though.

Something like:

(define (mergesort l)
  (let sort-loop ((l l) (len (length l)))
    (cond
      ((null? l) '())
      ((null? (cdr l)) l)
      (else (merge (sort-loop (take (/ len 2) l) (/ len 2)))
                   (sort-loop (drop (/ len 2) l) (/ len 2)))))))))
(define (take n l)
  (if (= n 0)
      '()
      (cons (car l) (take (sub1 n) (cdr l)))))
(define (drop n l)
  (if (= n 0)
      l
      (drop (sub1 n) (cdr l))))
Nathan Sanders
+1  A: 

In general, mergesort is doing a lot of list manipulations, so it is much better to do things destructively by sorting sub parts "in-place". You can see the implementation of sort in PLT Scheme for example of a common code, which originated in SLIB. (It might look intimidating on first sight, but if you read the comments and ignore the knobs and the optimizations, you'll see it all there.)

Also, you should look at SRFI-32 for an extended discussion of a sorting interface.

Eli Barzilay