replacement of short functions by their definitions andother code cleanups

Dear Andreas,

You're right, you indeed did not replace function calls with their
(original) definitions; however, you did replace calls to short convenience
functions with explicit code.

My concern, apart from subtle differences between the old and new
implementation that requires thorough retesting of the tensor code (I don't
mind that; if the changes were otherwise warranted, this is what we're here
for) is that changes like this example:

-	   (setq dumx (concat $idummyx n))
+	 (setq dumx (intern (format nil "~a~d" $idummyx n)))

do not make the code more readable. On the contrary, whereas before,
subtleties of concat notwithstanding, I immediately understood that the code
was concatenating $idummyx and n to form a symbol for dummy indices, now I
do need to go and look up what the devil format does when its first argument
is nil, the definition of the a and d tags in the format string, and the
meaning of the intern function which is not at all evident from its name.

It also makes debugging harder, since the functionality is no longer
encapsulated in a (separately tested) function; now, when I am hunting for
subtle bugs, I need to explicitly test if the arguments to format are
correct, if format returns what I expect it to return, if intern does with
it what it is supposed to do.

This is precisely what functions are invented for: so that when, for
instance, I am writing tensor algebra code, I can concentrate on tensor
algebra, not LISP details like, say, misspellings of the format string.

I appreciate your feelings with regards to "old-style functions", but I'd
like to ask you to keep in mind that others may feel differently about them.
So if the old definition of concat was wrong (and I am certainly not
defending the way it was implemented; by all means, if you can improve it,
please do!), replace it with a correct definition. If it is wrong to call it
concat because, for instance, the name conflicts with a maclisp-like
function of similar but subtly different functionality, rename it. But
replacing a single call to concat with explicit code containing two function
calls and several additional parameters does not seem like a good idea to
me. Furthermore, making such changes all over the place means, in effect,
that you are enforcing your style preferences on everybody else's code.

In other words, while I don't want to make this issue bigger than it really
is, I respectfully disagree with you on the impact of these changes on
maintainability.

You also need to keep in mind that making changes of this nature means, in
effect, that the primary maintainer of the code finds it harder and harder
to recognize the code. We're all volunteers; I wish I had more time to work
on tensor algebra coding, but I don't. So when I do get a chance to work on
Maxima, I'd like to spend my time productively, and improve the _algebra_,
which is hard when I do not recognize the code anymore.

For these reasons, as the maintainer of the tensor code, I feel that the use
of functions like concat as shorthand for (intern (format nil "~a~d" arg1
arg2)) or equivalent code significantly improves readability, and it is my
intent to continue using them. I'd like to ask that they not be removed,
only replaced (if necessary) with improved implementations; otherwise, it'll
be necessary for me to maintain "private" versions of these functions in the
tensor code.


Viktor
 

-----Original Message-----
From: Andreas Eder [mailto:aeder at arcor.de] 
Sent: Saturday, May 05, 2007 4:39 AM
To: Viktor T. Toth
Cc: maxima at math.utexas.edu
Subject: Re: [Maxima] replacement of short functions by their definitions
andother code cleanups 


Viktor wrote:

> Thank you for raising this topic; I've certainly seen a number of recent
> changes in the tensor package that I maintain, which gave me cause for
> concern. Replacing simple functions like, most recently, concat, with
their
> definitions seriously impedes code readability without introducing any
> tangible benefits that I can see.

The issue is not replacing functions like concat by their
definitions (which I did *not* do), but contrary to what you say
to enhance readability. Do you know the (subtle?) differences
between concat, sconcat and concatenate? Right from the top of
your head, when reading code? concat was mostly used to do things
fow which there are perfectly suited other common lisp functions
like gentemp or format or intern. Concat also still used the old
(maclisp-like?) function calling convention for variable number of
arguments where again there is a working and idiomatic way in CL
with &rest and &optional args. And on top of that, instead of
using string handling concat used the implode/explode combo of
functions which misuses symbols for string handling.

> If there are no objections, I'd respectfully revert some recent changes in
> the tensor package, for the sake of continuing maintainability.

The other - older - changes to tensor were of a similar spirit, as
you can see in the cvs log messages: removing or changing maclisp
like code by CL idioms etc.

I cannot see where this impeded maintainability of the code.

> This means of course that I'd like to continue to rely on the existence of
these
> functions in the shared maxima code base, as the alternative, namely
keeping
> private copies of these functions in the tensor package, is clearly not
> preferable.

I very strongly think that we should not depend on these old style
functions (zl-member, concat, assq etc.).

Well this is just my 2 cents.

'Andreas