2
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

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))
|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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