Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
(defun read-sim-file (filename)
  "Reads file.
File is presumed to be the output from a simulation.

Returns lines from the simulation as a list of strings

We throw an error if TRAP ET is detected.

Lines with DBG_INT are ignored."
  (let ((file-text-list (split "\\n"
                   (batteries:read-text-file filename)))
    (return_list '()))

    (loop for line in file-text-list do
     (cond ((scan "DBG_INT" line)
        )  ; intentional newline.
           ((scan "TRAP ET" line)
        (error "Trap ET detected"))
           (t
        ;; There's got to be a cleaner method here.
        (setf return_list (append return_list (list line))))))
    return_list))

This code is ugly. In particular, the (setf X (append X Y)) idiom feels hideously clunky.

Comments on how to make this more beautiful?

Possibly REMOVE-IF?

share|improve this question

1 Answer

At first glance, you're looking for loops collect clause.

(defun read-sim-file (filename)
  (let ((file-text-list (split "\\n" (batteries:read-text-file filename))))
    (loop for line in file-text-list 
      when (scan "TRAP ET" line)
        do (error "Trap ET detected o_o")
      unless (scan "DBG_INT" line)
        collect line)))

At second glance, you can do this much more efficiently using with-open-file instead of breaking your input file up into a list (this should save you two traversals of said file)

(defun read-sim-file (filename)
  (with-open-file (stream filename)
    (loop for line = (read-line stream nil 'eof) until (eq line 'eof)
      when (scan "TRAP ET" line)
        do (error "Trap ET detected ಠ_ಠ")
      unless (scan "DBG_INT" line)
        collect line)))

The first one is a fairly basic loop directive, the second one is almost verbatum an example out of the CL Cookbook "files" section (the whole cookbook contains many other useful recipes too).

Just as a sidenote, do try to indent things properly. It was more difficult than it should have been to read your initial program. If you ever catch yourself leaving comments like ;; intentional newline, think hard about what you just wrote.


Now that I've had some proper sleep, lets take the new one apart for educational purposes.

  (with-open-file (stream filename)

This line opens up a file and creates a handle for it named stream. Nothing has actually been read yet, you've just got a stream to pull from.

    (loop for line = (read-line stream nil 'eof) until (eq line 'eof)

loop has a couple of non-obvious directives. This is one of them (and coincidentally, the reason that some lispers prefer the non-standard iterate module; there's a feeling that infix notation isn't Lispy enough. I disagree, just like to point it out).

read-line takes a stream and a few options and returns the first line from that stream. (read-line stream nil 'eof) means "Read the next line from stream, don't error at the end of file marker (that's the nil) and return the symbol eof when you reach the end of the file.

The full line then reads "Loop through the lines in stream until you hit 'eof".

      when (scan "TRAP ET" line)
        do (error "Trap ET detected ಠ_ಠ")

loop has conditional directives too, so you don't need to resort to cond here. if, else, when and unless are all valid forms.

      unless (scan "DBG_INT" line)

Speaking of unless, you can use it to skip "DBG_INT" lines without devoting a separate clause to them.

        collect line))

This does the same thing as

     (setf tmp (append return_list (list line)))

which is to say, it creates a list where the elements are collected in order they're encountered (though there isn't an actual named variable that). The loop form also returns whatever you've collected if that's the last clause in the statement (so you don't need to explicitly return it).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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