Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

So I took a stab at this task to begin learning common lisp.

The idea is that you give it a written representation of a number such as "three thousand and forty nine" and it will output 3049.

I was looking for some input on my lisp and where I'm being daft or even what parts are good.

So without further a due, my code:

(defmacro do-push (result value)
 `(if (not (eq ,value 0))
   (progn (setf ,result (append ,result (cons ,value nil)))
      (setf ,value 0))))

(defun should-push (string)
 (or (equalp string "and")) (not (should-multiply string)))

(defmacro sum-list (L)
 `(reduce '+ ,L))

(defun get-input ()
 (princ "Please enter a number: ")
 (read-line))

(defun tokenize (string)
 (loop for i = 0 then (incf j)
       as j = (position #\Space string :start i)
       collect (subseq string i j)
       while j))

(defun should-multiply (string)
 (or (equalp string "hundred") (equalp string "thousand") (equalp string "million"))) 

(defun get-needed-operation (string)
 (cond ((should-multiply string) (symbol-function '*))
       ((equalp string "and") (symbol-function '+))
       (t nil)))

(defun string-value (string)
 (cond ((equalp string "one") 1)
       ((equalp string "two") 2)
       ((equalp string "three") 3)
       ((equalp string "four") 4)
       ((equalp string "five") 5)
       ((equalp string "six") 6)
       ((equalp string "seven") 7)
       ((equalp string "eight") 8)
       ((equalp string "nine") 9)
       ((equalp string "ten") 10)
       ((equalp string "eleven") 11)
       ((equalp string "twelve") 12)
       ((equalp string "thirteen") 13)
       ((equalp string "fourteen") 14)
       ((equalp string "fifteen") 15)
       ((equalp string "sixteen") 16)
       ((equalp string "seventeen") 17)
       ((equalp string "eighteen") 18)
       ((equalp string "nineteen") 19)
       ((equalp string "twenty") 20)
       ((equalp string "thirty") 30)
       ((equalp string "forty") 40)
       ((equalp string "fifty") 50)
       ((equalp string "sixty") 60)
       ((equalp string "seventy") 70)
       ((equalp string "eighty") 80)
       ((equalp string "ninety") 90)
       ((equalp string "hundred") 100)
       ((equalp string "thousand") 1000)
       ((equalp string "million") 1000000)))

(let ((result '())
      (current 0))
 (loop for word in (tokenize "one million three hundred thousand eighteen thousand six hundred and seventy three") do
  (let ((operation (get-needed-operation word))
        (value (string-value word)))
   (progn (when (and (eq value nil) (eq operation nil))
       (format t "Unexpected token: ~a~c" word #\newline))

      (if (eq operation nil) (setq operation (symbol-function '+)))

      (when (should-push word)
       (do-push result current))

      (when (not (eq value nil))
       (setq current (funcall operation value current)))))
       finally (do-push result current))
 (format t "Result: ~a" (sum-list result)))

My logic is to have a list that contains all the detected "sub-numbers" which are just added together to get the final number.

Certain words (hundred, thousand, etc) imply a multiply operation to the current number such that three hundred becomes 3 * 100 = 300

Certain words (and, one, two, etc) imply an addition operation such that sixty seven becomes 60 + 7 = 67

Some words cause the current number to be pushed to the result list, resetting the current number.

share|improve this question
add comment

1 Answer

up vote 0 down vote accepted

There are a couple of issues with your code. Let me describe a few:

  • numbers are not EQ, use EQL or =.

  • Common Lisp has documentation strings. Use them.

  • if not progn can be replaced with a single unless

The next function does not work as expected. I have inserted a newline and you see that the parentheses are wrong.

(defun should-push (string)
 (or (equalp string "and"))
 (not (should-multiply string)))

Don't write trivial macros. If you want to have a name for a trivial function, then use a function - not a macro. If the calling overhead bothers you, then declare it inline.

(defmacro sum-list (L)
  `(reduce '+ ,L))

The next may or may not work as you think. Since Common Lisp output streams can be buffered, call FORCE-OUTPUT when you actually want to see the output.

(defun get-input ()
  (princ "Please enter a number: ")
  (read-line))
  • or equalp can be replaced with (member foo '("a" "b" "c") :test #'equalp). Get the data out of the logic.

  • instead of (symbol-function '+) use #'+

  • instead of COND EQUALP use ASSOC with the :test keyword. Get the data out of the code. Alternatively use a macro similar to CASE, which works for strings.

  • for the last form, write a function, too. Always provide functions. Don't print the result. Just return the result.

  • inside a LET you don't need a PROGN

  • EQ nil is better replaced with the NULL predicate.

  • format t "~c" #\newline is just format t "~%"

  • when not is unless

share|improve this answer
1  
Thanks a bunch! Unfortunately I can't +1 you because I don't have enough rep. –  Lerp Feb 21 '13 at 17:33
add comment

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.