[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [STUMP] removed duplicate code from group, screen and window.lisp
From: |
Shawn |
Subject: |
Re: [STUMP] removed duplicate code from group, screen and window.lisp |
Date: |
Sun, 27 Jul 2008 17:38:00 -0700 |
User-agent: |
Gnus/5.11 (Gnus v5.11) Emacs/22.1 (gnu/linux) |
"Patrick Pippen" <address@hidden> writes:
Hi Patrick,
Your patch was not quite what I meant. I left some comments below.
> +(defmacro set-list (l &body body)
> + `(setf ,l ,@body))
This doesn't actually do anything. Why use set-list over setf?
> +(defmacro push-list (l &body body)
> + `(push ,l ,@body))
Same here. Push already pushes an element to a list. Why use push-list
instead of push?
> +(defun move-to-head (&key screen group window)
> + "Moves a screen, group, or window to head based
> +on condition of keyword arguments :screen :group :window and it defaults to
> nil if not supplied."
> + (if (and (not (null screen)) (null group) (null window))
(not (null screen)) can be simplified to just screen.
> + (progn
> + (set-list *screen-list* (remove screen *screen-list*))
> + (push-list screen *screen-list*))
> + (cond
> + ((and (not (null screen)) (not (null group)) (null window))
> + (set-list (screen-groups screen) (delete group (screen-groups screen)))
> + (push-list group (screen-groups screen)))
> + ((and (null screen) (not (null group)) (not (null window)))
> + (set-list (group-windows group) (delete window (group-windows group)))
> + (push-list window (group-windows group))))))
The problem with this function is that it actually makes things more
complicated. Instead of having 3 functions that use similar code to do
a very similar thing, you have 1 function with 3 copies of of similar
code that do similar things.
What I was thinking was either a macro like this:
(defmacro move-to-head (list elt)
"Move the specified element in in LIST to the head of the list."
`(progn
(setf ,list (remove ,elt ,list))
(push ,elt ,list)))
which you could use this way: (move-to-head *screen-list* screen) But
maybe you'd want something more specialized such as a macro that
generates those the move-*-to-head functions so you could replace them
with:
(define-move-to-head screen *screen-list*)
However, having looked at those three functions they're actually
different enough that it doesn't really make much sense to have such a
macro. I only included it to help you see the direction I was thinking
in.
-Shawn