Skip to main content
Code Review

Return to Answer

Correct missing radix to reject digits 8 and 9. Correct missing final close paren. Change doc string to satisfy lame "Edits must be over six characters" rule.
Source Link

First of all a note about variables: You're using setq on previously undefined and undeclared variables. This is actually undefined behavior and in most implementations will create global dynamic/special variables. To create local variables (which you presumably intended) use let.

(defun oct-string-to-number 
 (string)
 "Converts an octal string to a number. Only digits from 0 - 7 are accepted; sign or decimal point symbols will cause oct-to-number to fail"

This whole function can be implemented as (parse-integer string :radix 8). However for the sake of learning let's go through the function anyway:

(setq digits '(#0円 #1円 #2円 #3円 #4円 #5円 #6円 #7円))
...
(setq pos (position char digits))

Iterating through the list of digits to convert a digit to an integer, is neither the most efficient nor the most succinct to do it. You can use the digit-char-p function which returns the digit's int value, if the given char is a digit and nil otherwise. (The same applies in slurp-octal-digits where you do the same thing).

(loop for char across (reverse string)
 do 
 (setq pos (position char digits))
 (setq result (+ result (* pos place)))
 (setq place (* 8 place)))

Instead of reversing the string and keeping track of a place variable, you can just iterate over the string from the beginning and multiply the result by 8 each step:

(loop for char across string
 do 
 (setq result (+ (* 8 result) (digit-char-p char))))

That being said, I'd rather use reduce then loop here (that is, if I weren't using parse-integer) - especially as higher order functions are one of the most useful things in lisp that a newcomer should get acquainted to. Using reduce the function would be written like this:

(defun oct-string-to-number (string)
 "Converts an octal string to a number. Only digits from 0 - 7 are accepted; sign, non-octal digit or decimal point symbols will cause oct-to-number tosignal fail"error."
 (reduce
 (lambda (sum digit)
 (+ (* 8 sum) (digit-char-p digit 8)))
 string :initial-value 0))

First of all a note about variables: You're using setq on previously undefined and undeclared variables. This is actually undefined behavior and in most implementations will create global dynamic/special variables. To create local variables (which you presumably intended) use let.

(defun oct-string-to-number 
 (string)
 "Converts an octal string to a number. Only digits from 0 - 7 are accepted; sign or decimal point symbols will cause oct-to-number to fail"

This whole function can be implemented as (parse-integer string :radix 8). However for the sake of learning let's go through the function anyway:

(setq digits '(#0円 #1円 #2円 #3円 #4円 #5円 #6円 #7円))
...
(setq pos (position char digits))

Iterating through the list of digits to convert a digit to an integer, is neither the most efficient nor the most succinct to do it. You can use the digit-char-p function which returns the digit's int value, if the given char is a digit and nil otherwise. (The same applies in slurp-octal-digits where you do the same thing).

(loop for char across (reverse string)
 do 
 (setq pos (position char digits))
 (setq result (+ result (* pos place)))
 (setq place (* 8 place)))

Instead of reversing the string and keeping track of a place variable, you can just iterate over the string from the beginning and multiply the result by 8 each step:

(loop for char across string
 do 
 (setq result (+ (* 8 result) (digit-char-p char))))

That being said, I'd rather use reduce then loop here (that is, if I weren't using parse-integer) - especially as higher order functions are one of the most useful things in lisp that a newcomer should get acquainted to. Using reduce the function would be written like this:

(defun oct-string-to-number (string)
 "Converts an octal string to a number. Only digits from 0 - 7 are accepted; sign or decimal point symbols will cause oct-to-number to fail"
 (reduce
 (lambda (sum digit)
 (+ (* 8 sum) (digit-char-p digit)))
 string :initial-value 0)

First of all a note about variables: You're using setq on previously undefined and undeclared variables. This is actually undefined behavior and in most implementations will create global dynamic/special variables. To create local variables (which you presumably intended) use let.

(defun oct-string-to-number 
 (string)
 "Converts an octal string to a number. Only digits from 0 - 7 are accepted; sign or decimal point symbols will cause oct-to-number to fail"

This whole function can be implemented as (parse-integer string :radix 8). However for the sake of learning let's go through the function anyway:

(setq digits '(#0円 #1円 #2円 #3円 #4円 #5円 #6円 #7円))
...
(setq pos (position char digits))

Iterating through the list of digits to convert a digit to an integer, is neither the most efficient nor the most succinct to do it. You can use the digit-char-p function which returns the digit's int value, if the given char is a digit and nil otherwise. (The same applies in slurp-octal-digits where you do the same thing).

(loop for char across (reverse string)
 do 
 (setq pos (position char digits))
 (setq result (+ result (* pos place)))
 (setq place (* 8 place)))

Instead of reversing the string and keeping track of a place variable, you can just iterate over the string from the beginning and multiply the result by 8 each step:

(loop for char across string
 do 
 (setq result (+ (* 8 result) (digit-char-p char))))

That being said, I'd rather use reduce then loop here (that is, if I weren't using parse-integer) - especially as higher order functions are one of the most useful things in lisp that a newcomer should get acquainted to. Using reduce the function would be written like this:

(defun oct-string-to-number (string)
 "Converts an octal string to a number. Only digits from 0 - 7 are accepted; sign, non-octal digit or decimal point will signal error."
 (reduce
 (lambda (sum digit)
 (+ (* 8 sum) (digit-char-p digit 8)))
 string :initial-value 0))
Source Link
sepp2k
  • 9k
  • 2
  • 39
  • 51

First of all a note about variables: You're using setq on previously undefined and undeclared variables. This is actually undefined behavior and in most implementations will create global dynamic/special variables. To create local variables (which you presumably intended) use let.

(defun oct-string-to-number 
 (string)
 "Converts an octal string to a number. Only digits from 0 - 7 are accepted; sign or decimal point symbols will cause oct-to-number to fail"

This whole function can be implemented as (parse-integer string :radix 8). However for the sake of learning let's go through the function anyway:

(setq digits '(#0円 #1円 #2円 #3円 #4円 #5円 #6円 #7円))
...
(setq pos (position char digits))

Iterating through the list of digits to convert a digit to an integer, is neither the most efficient nor the most succinct to do it. You can use the digit-char-p function which returns the digit's int value, if the given char is a digit and nil otherwise. (The same applies in slurp-octal-digits where you do the same thing).

(loop for char across (reverse string)
 do 
 (setq pos (position char digits))
 (setq result (+ result (* pos place)))
 (setq place (* 8 place)))

Instead of reversing the string and keeping track of a place variable, you can just iterate over the string from the beginning and multiply the result by 8 each step:

(loop for char across string
 do 
 (setq result (+ (* 8 result) (digit-char-p char))))

That being said, I'd rather use reduce then loop here (that is, if I weren't using parse-integer) - especially as higher order functions are one of the most useful things in lisp that a newcomer should get acquainted to. Using reduce the function would be written like this:

(defun oct-string-to-number 
 (string)
 "Converts an octal string to a number. Only digits from 0 - 7 are accepted; sign or decimal point symbols will cause oct-to-number to fail"
 (reduce
 (lambda (sum digit)
 (+ (* 8 sum) (digit-char-p digit)))
 string :initial-value 0)
lang-lisp

AltStyle によって変換されたページ (->オリジナル) /