Re: Beginner - Function Critique

crystal1 <crystal1@xxxxxxxxxxxx> writes:

> See below. These functions have been created a thousand times and are
> trivial, but I wrote these instances myself and am a pretty proud
> beginner. What do you think?
>
> A week ago recursion was still pretty hazy, but a after reading
> "Common Lisp, An Interactive Approach" the progress has been
> steady. What a great book.
>
> ;;;;;;;;;;;;;;;;
>
> (defpackage :string-utils
> (:use :common-lisp))
>
> (in-package :string-utils)
>
> (defun line-split1 (s d)
> "Helper function for LINE-SPLIT. Return a list with elements of
> supplied string S up to the first instance of delimiter D as :STRING
> and the remaining string after the delimiter as :REMAINDER."
> (if (position d s) (list :string (subseq s 0 (position d s))
> :remainder (subseq s (1+ (position d s))))
> (list :string s :remainder "")))


(defun line-split1 (s d)
(if (position d s)
(values (subseq s 0 (position d s))
(subseq s (1+ (position d s))))
(values s "")))


> (defun line-split (s d)
> "Return a list of substrings in string S when delimited by delimiter D."
> (check-type s string)
> (check-type d character)

These check-types are done by called functions, so you're doing them twice.

> (if (= (length s) 0) '()
> (cons (getf (line-split1 s d) :string)
> (line-split (getf (line-split1 s d) :remainder) d))))

Indent it this way:

(if condition
then
else)

otherwise we will believe your else is a then!

Note that you're calling twice line-split1 with the same argument!


(defun line-split (s d)
"Return a list of substrings in string S when delimited by delimiter D."
(if (= (length s) 0)
'()
(multiple-value-bind (string remainder) (line-split1 s d)
(cons string (line-split remainder d)))))

Note that this is still a non-terminal recursive function, so you'll
need a lot of stack space.

You could try to write a recursive function that is tail-recursive, to
give a chance to a TCO implementation. Note that not all CL
implementations do TCO, so you still may want to write an iterative solution.


> (defun read-file (filename delimiter)
> "Return a list of lists containing strings in pathname FILENAME
> delimited by character DELIMITER."
> (let ((output '()))
> (with-open-file (in filename)
> (do ((line (read-line in nil) (read-line in nil)))
> ((null line))
> (push (line-split line delimiter) output))
> (nreverse output))))


Good. Eventually, I switched to loop:

(defun read-file (filename delimiter)
"Return a list of lists containing strings in pathname FILENAME
delimited by character DELIMITER."
(with-open-file (in filename)
(loop
:for line = (read-line in nil)
:while line
:collect (line-split line delimiter))))



> (defmacro join-strings (&rest strings)
> "Return the result of concatenating the supplied strings."
> `(concatenate 'string ,@strings))

You don't need a macro here. With a macro, you cannot write:

(mapcar 'join-string '("A " "The " "Some ")
'("cat " "dog " "guy ")
'("eats " "bites " "runs ")
'("." "." "."))

Write it as a function. If you want, you can declare it INLINE.


> (defun rjust (s w)
> "Right justify string S by W spaces."
> (rjust1 s (- w (length s))))
>
> (defun rjust1 (s w)
> "Helper function for RJUST."
> (if (<= w 0) s
> (join-strings (rjust1 s (1- w)) " ")))

You can use local functions:

(defun rjust (s w)
"Right justify string S by W spaces."
(labels ((rjust1 (s w)
(if (<= w 0)
s
(join-strings (rjust1 s (1- w)) " "))))
(rjust1 s (- w (length s)))))


> (defun ljust (s w)
> "Left justify string S by W spaces."
> (ljust1 s (- w (length s))))
>
> (defun zfill (s w)
> "Left justify string S by W 0 characters."
> (ljust1 s (- w (length s)) "0"))
>
> (defun ljust1 (s w &optional (c " "))
> "Helper function for LJUST."
> (if (<= w 0) s
> (join-strings c (ljust1 s (1- w) c))))

Don't you feel that there's some redundancy here?

What about an API such as:

(defun string-pad (string width &key (padchar " ") (justification :left))
...)

(string-pad "123" 10 :padchar #\0 :justification :right)
(string-pad "Hello" 10 :padchar #\!)
(string-pad "BIG TITILE" 20 :justification :center)



Overall, congratulations for your lisping! Go on on this journey!

--
__Pascal Bourguignon__ http://www.informatimago.com/

Nobody can fix the economy. Nobody can be trusted with their finger
on the button. Nobody's perfect. VOTE FOR NOBODY.
.

Quantcast