[plt-scheme] Scheme Program

From: Danny Yoo (dyoo at hkn.eecs.berkeley.edu)
Date: Mon Oct 2 17:29:16 EDT 2006

> However, this section of the code is not working properly and giving me 
> the following error: apply: bad procedure: #[undefined]

Hi Haider,

The following is a code review of the code.  Please don't take any of the 
comments as condemnation against your own person. I'll try to keep 
criticism directed at the code.


Code review comments:

     * I tried reading your program.  I suppose I'm not be a Schemer yet,
       but I think the program is, frankly, hard to read.  It's way too
       nested for me to read comfortably, and the indentation is in places
       that does not seem natural.  Double-spacing Scheme code is not
       common.

     * There are superfluous expressions.  Doing a test like:

           (not (equal? (pdg-vertex-ids-killed v) #f))

       can be more simply expressed as:

           (pdg-vertex-ids-killed v)

     * There's a LOT of mutation going on in the code.  Are you sure you
       want to do that?  It's really making the code more complicated than
       it needs to be.

     * There are subexpressions that basically look like big copy-and-paste
       blocks.  Don't copy-and-paste: abstract!  Concretely, the only
       difference I see between

                   (if (pdg-vertex-ids-killed v)
                       (abs-loc-set-traverse
                        (pdg-vertex-ids-killed v)
                        (lambda (y)
                          (let*
                              ((line-num
                                (file-get-line-num fileuid offset)))
                            (set! linelist
                                  (cons (list line-num y v)
                                        linelist)))
                          #t)))

       and:

                   (if (pdg-vertex-ids-used v)
                       (abs-loc-set-traverse
                        (pdg-vertex-ids-used v)
                        (lambda (y)
                          (let*
                              ((line-num
                                (file-get-line-num fileuid offset)))
                            (set! linelist
                                  (cons (list line-num y v)
                                        linelist)))
                          #t)))

       is just one minor subexpression.

     * Everything seems to be stuffed in one gargantuan function.  Consider
       writing helper functions where it will clarify the code's intent.

     * I see no direct use of APPLY in your code, so something else must be
       calling it.  Actually, I see a few type errors in the code.

       Concretely: what's the "contract" of ABS-LOC-SET-TRAVERSE?  Check
       your usages of that function: the code is definitely violating its use of
       ABS-LOC-SET-TRAVERSE in a few places.



You made the comment:

> However, it gave me wrong output, some had TO and FROM both listed 
> together:

What in the program prevents from happening? From the sequence of 
subexpressions:

     (if (pdg-vertex-ids-killed (caddr z))
         (abs-loc-set-traverse ...))
     (if (pdg-vertex-ids-used (caddr z))
         (abs-loc-set-traverse ...))

there's nothing in the code that excludes both PDG-VERTEX-IDS-KILLED and 
PDG-VERTEX-IDS-USED to return true values.


If you are going to write more Scheme code, you might also want to 
consider doing a read-through a textbook like HTDP:

     http://htdp.org

If you do so, your programs will tend to have a better design.



I've reformatted your code as best as I can in the context of a PLT Scheme 
module, to be able to use the Syntax Check tool as well as better see the 
structure of the program.

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(module pdg mzscheme
   ;; Unknown: the following definitions are here only so I can run
   ;; check syntax and see where values are coming from.
   (define (pdg-vertex-set-traverse l f) (void))
   (define (int-pair-set-traverse l f) (void))
   (define (abs-loc-set-traverse l f) (void))
   (define (pdg-vertices pdg) (void))
   (define (pdg-compilation-uid pdg) (void))
   (define (pdg-vertex-pdg pdg) (void))
   (define (pdg-kind pdg) (void))
   (define (pdg-vertex-charpos pdg) (void))
   (define (pdg-vertex-ids-killed pdg) (void))
   (define (pdg-vertex-ids-used pdg) (void))
   (define (file-get-line-num x y) (void))
   (define (sort l cmp) (void))
   (define (sdg-pdgs) (void))


   (define (assignments)

     (define filenumber 1)

     (define (process-pdg pdg)
       (when (eqv? (pdg-kind pdg) 'user-defined)
         (let* ((out-port
                 (open-output-file
                  (string-append "Fun"
                                 (number->string filenumber)
                                 "_V.dat")))
                (linelist ()))
           (pdg-vertex-set-traverse
            (pdg-vertices pdg)
            (lambda (v)
              (let* ((fileuid
                      (pdg-compilation-uid
                       (pdg-vertex-pdg v))))
                (int-pair-set-traverse
                 (pdg-vertex-charpos v)
                 (lambda (offset w)
                   (if (pdg-vertex-ids-killed v)
                       (abs-loc-set-traverse
                        (pdg-vertex-ids-killed v)
                        (lambda (y)
                          (let*
                              ((line-num
                                (file-get-line-num
                                 fileuid offset)))
                            (set! linelist
                                  (cons (list line-num y v)
                                        linelist)))
                          #t)))

                   (if (pdg-vertex-ids-used v)
                       (abs-loc-set-traverse
                        (pdg-vertex-ids-used v)
                        (lambda (y)
                          (let*
                              ((line-num
                                (file-get-line-num
                                 fileuid offset)))
                            (set! linelist
                                  (cons (list line-num y v)
                                        linelist)))
                          #t)))
                   #t)))
              #t))

           (set! linelist
                 (sort linelist
                       (lambda (a b)
                         (< (cadar a) (cadar b)))))

           (for-each (lambda (z)
                       (let
                           ((linenum (car z)))
                         (define line-num (car z))
                         (display (cdr line-num) out-port)
                         (write-char #\tab out-port)

                         (if (pdg-vertex-ids-killed (caddr z))
                             (abs-loc-set-traverse
                              (pdg-vertex-ids-killed (caddr z))
                              (display "to" out-port)))

                         (if (pdg-vertex-ids-used (caddr z))
                             (abs-loc-set-traverse
                              (pdg-vertex-ids-used (caddr z))
                              (display "from" out-port)))

                         (newline out-port)))
                     linelist)

           (close-output-port out-port))

         (set! filenumber (+ filenumber 1))))

     (for-each process-pdg (sdg-pdgs)))

   (assignments))


Posted on the users mailing list.