```Subject: Re: resonable use of internal functions
From: Erik Naggum <erik@naggum.no>
Date: 1999/11/18
Newsgroups: comp.lang.lisp
Message-ID: <3151901604345632@naggum.no>

[ I have reindented the quoted code according to more standard conventions. ]

* Friedrich Dominicus <Friedrich.Dominicus@inka.de>
| So I came up with this:
|
| (defun next-val (add-to nom-factor denom-factor alternating)
|	         (/ nom-factor
|		    denom-factor))))
|
| of course one can argue about add-to. should it be in that function or
| not. I put it here. Now sine looks like
|
| (defun next-sine-val (val x n)
|   (let ((alternating (expt -1 n))
|	  (nom-factor (expt x (+ (* 2 n) 1)))
|	  (denom-factor (fact (+ (* 2 n) 1))))
|     (next-val val nom-factor denom-factor alternating)))
|
| the thing is as easy for cos now
| (defun next-cosine-val (val x n)
|   (let ((alternating (expt -1 n))
|	  (nom-factor (expt x (* 2 n)))
|	  (denom-factor (fact (* 2 n))))
|     (next-val val nom-factor denom-factor alternating)))
|
| and so on. So it may be that next-val should be a global function and
| that I just internalise nexe-sine-val in sine or the like.

in this particular case, very little is gained by a global function, and
some performance opportunity may easily be lost, so for such a simple
function, I would recommend an FLET binding around the two functions that
use it.  incidentally, your compiler may not do common subexpression
elimination on (+ (* 2 n) 1), and in the absence of declarations they
might be quite expensive.

| And I think as simple as next-val is, it's nevertheless a useful
| abstraction worth capturing in a own function.

that doesn't necessarily imply _global_ function.  it would also make
sense to write this with a MACROLET (instead of FLET) to capture the
abstraction without the function call overhead and need to re-declare
everything.  you could condense your code quite a bit with a macro:

(macrolet ((define-next-val-function (name cfse)
"Define a helper next-val function, NAME, with common factor
subexpression CFSE.  NAME takes arguments VAL, X, N, which may be used by
CFSE.  given C = the value of CFSE, NAME returns
n  x^c
val + (-1) -------
c!"
`(defun ,name (val x n)
(declare (fixnum n))		;ASSUMPTION!
(let* ((cfse ,expression)
(increment (/ (expt x cse) (fact cse))))
(if (evenp n)
(+ val increment)
(- val increment))))))
(define-next-val-function next-sine-val (+ (* 2 n) 1))
(define-next-val-function next-cosine-val (* 2 n)))

the use of simpler subexpressions, no extra function calls and avoiding a
call to EXPT of -1 should result in significantly tighter code, even
without declarations.  this may not be your concern, of course, but I
also consider the documentation of the above to be far superior to your
use of several temporary variables.  that this captures the mathematical
abstraction in one place, rather than a trivial abstraction with the
mathematical abstraction in two different places, would at least to make
make it much more readable to me.  the simple reason is that I want to
avoid looking at the internals of a function once it has been written and
debugged.  for that reason, I might also use FLETs instead of global
functions since these functions are used only inside your SIN and COS.
there's no point in externalizing what should not be externally used.

if the above flagged assumption is wrong, replace FIXNUM with INTEGER.

(also note that when the scope of a macro is carefully constrained, you
don't need as much "cover" as when it is used globally.  the above macro