Replacing elements from a list and its sublists - part II

Question

This is sort of a follow-up to Replacing elements from a list and its sublists but now there are arbitrary numbers of words that would be replaced stored in a list.

Now write substitute2 that takes a list, a list of old words, and a list of new words; the last two lists should be the same length. It should return a copy of the first argument, but with each word that occurs in the second argument replaced by the corresponding word of the third argument:

source

Please review my code.

(define (substitute2 lst oldlst newlst)
 (define (maybe-swap elem oldlst newlst) 
 (cond ((null? oldlst) elem)
 ((equal? (car oldlst) elem) (car newlst))
 (else (maybe-swap elem (cdr oldlst) (cdr newlst)))))
 (if (list? lst)
 (map (lambda (elem) (substitute2 elem oldlst newlst)) lst)
 (if (= (length oldlst) (length newlst)) (maybe-swap lst oldlst newlst) (error "invalid length"))))

How can I make this code better and more efficient? Did I overuse define?

Answer 1

This looks pretty good. I only see one issue and it's more code appearance than functional

Fix your spacing

You indentation isn't quite right and makes it a little hard to see what clauses line up where. Here's your same code slightly altered:

(define (substitute2 lst oldlst newlst)
 (define (maybe-swap elem oldlst newlst) 
 (cond ((null? oldlst) elem)
 ((equal? (car oldlst) elem) (car newlst))
 (else (maybe-swap elem (cdr oldlst) (cdr newlst)))))
 (if (list? lst)
 (map (lambda (elem) (substitute2 elem oldlst newlst)) lst)
 (maybe-swap lst oldlst newlst)))

I lined up the three cond clauses - and cleared up the fact that the map and maybe-swap both belong to the if. As-is, it kind of looks life the if is part of maybe-swap.

Make a swapper function

This is more for fun than it is necessarily a valuable suggestion (I have no idea if this is good or not). But one thing we can do is take oldlst and newlst and turn those into a swapper function - one that takes an elem and returns the correct substituted result. The code to build up such a swapper is pretty similar to your maybe-swap already:

(define (make-swapper oldlst newlst)
 (if (null? oldlst)
 (lambda (elem) elem)
 (lambda (elem)
 (if (equal? elem (car oldlst))
 (car newlst)
 ((make-swapper (cdr oldlst) (cdr newlst)) elem)))))

So then in substitute2, we just make one up front and call it:

(define (substitute2 lst oldlst newlst)
 (define swapper (make-swapper oldlst newlst))
 (define (substitute-impl lst)
 (if (list? lst)
 (map substitute-impl lst)
 (swapper lst)))
 (substitute-impl lst))