[lnkForumImage]
TotalShareware - Download Free Software

Confronta i prezzi di migliaia di prodotti.
Asp Forum
 Home | Login | Register | Search 


 

Forums >

comp.lang.lisp

Looking for code review

Antsan

10/12/2015 7:34:00 PM

I'm trying to formalize a PnP RPG by implementing its rules in Common Lisp. For
this I need an actor object. Every actor object has the same attributes and I
wanted easy access to those attributes by name without having to hardcode
everything.
What I came up with was this (I'm using fset):


;;;;; attributes.lisp
(in-package #:skyscrapers)

(defparameter *attributes*
(empty-map))

(defmacro add-attr (name counter)
(declare (type symbol name))
`(setf *attributes*
(with (with *attributes*
',name ',counter)
',counter ',name)))

(defmacro add-attrs (&body names)
`(progn
,@(loop for (name counter)
on names by #'cddr
collect
`(add-attr ,name ,counter))))

(add-attrs
strength intelligence
resistance dexterity
empathy fortitude
charisma secrecy
)

;;;; actors.lisp
(in-package #:skyscrapers)

#.(let ((attrs (mapcar (lambda (attr)
`(,attr .
,(gensym (string-downcase (symbol-name attr)))))
(convert 'list
(domain *attributes*)))))
`(progn
(defclass actor ()
((,(gensym "exhaustion")
:type (integer 0)
:initarg :exhaustion
:initform 0
:reader exhaustion)
,@(mapcar (lambda (attr/sym)
`(,(cdr attr/sym)
:type (integer 1)
:initarg ,(intern (symbol-name (car attr/sym))
:keyword)
:initform 1))
attrs)))
(defgeneric get-attr (actor attr))
,@(mapcar (lambda (attr/sym)
`(defmethod get-attr ((actor actor)
(attr (eql ',(car attr/sym))))
(slot-value actor ',(cdr attr/sym))))
attrs)))

This way I don't pollute my function namespace with accessors, which has been a
problem in a similar project before. Another boon is that I now can use GET-ATTR
safely with perfect knowledge that I'm not reading something which is not an
attribute. Using a function which expects an attribute and supplying EXHAUSTION
won't work as it would if I'd be using SLOT-VALUE.

But I can't help but think that this is ugly. I'm not quite sure why, though.
Does anybody see something wrong with what I did here?
9 Answers

Pascal J. Bourguignon

10/12/2015 7:55:00 PM

0

Antsan <thomas.bartscher@gmail.com> writes:

> I'm trying to formalize a PnP RPG by implementing its rules in Common Lisp. For
> this I need an actor object. Every actor object has the same attributes and I
> wanted easy access to those attributes by name without having to hardcode
> everything.
> What I came up with was this (I'm using fset):
>
>
> ;;;;; attributes.lisp
> (in-package #:skyscrapers)
>
> (defparameter *attributes*
> (empty-map))
>
> (defmacro add-attr (name counter)
> (declare (type symbol name))
> `(setf *attributes*
> (with (with *attributes*
> ',name ',counter)
> ',counter ',name)))
>
> (defmacro add-attrs (&body names)
> `(progn
> ,@(loop for (name counter)
> on names by #'cddr
> collect
> `(add-attr ,name ,counter))))
>
> (add-attrs
> strength intelligence
> resistance dexterity
> empathy fortitude
> charisma secrecy
> )
>
> ;;;; actors.lisp
> (in-package #:skyscrapers)
>
> #.(let ((attrs (mapcar (lambda (attr)
> `(,attr .
> ,(gensym (string-downcase (symbol-name attr)))))
> (convert 'list
> (domain *attributes*)))))
> `(progn
> (defclass actor ()
> ((,(gensym "exhaustion")
> :type (integer 0)
> :initarg :exhaustion
> :initform 0
> :reader exhaustion)
> ,@(mapcar (lambda (attr/sym)
> `(,(cdr attr/sym)
> :type (integer 1)
> :initarg ,(intern (symbol-name (car attr/sym))
> :keyword)
> :initform 1))
> attrs)))
> (defgeneric get-attr (actor attr))
> ,@(mapcar (lambda (attr/sym)
> `(defmethod get-attr ((actor actor)
> (attr (eql ',(car attr/sym))))
> (slot-value actor ',(cdr attr/sym))))
> attrs)))
>
> This way I don't pollute my function namespace with accessors, which has been a
> problem in a similar project before. Another boon is that I now can use GET-ATTR
> safely with perfect knowledge that I'm not reading something which is not an
> attribute. Using a function which expects an attribute and supplying EXHAUSTION
> won't work as it would if I'd be using SLOT-VALUE.
>
> But I can't help but think that this is ugly. I'm not quite sure why, though.
> Does anybody see something wrong with what I did here?

I don't see any justification for using a global variable to hold the
atribute of a single actor class.

At the very least, you could write a single macro

(define-class-with-attributes actor
strength intelligence
resistance dexterity
empathy fortitude
charisma secrecy)


But I don't see either any worthwhile justification for using get-attr
instead of slot-value. You can still define it as a "functional
abstration", but since you don't signal any more specific error than
slot-value, you could just define it as:

(defun get-attribute (actor attribute) (slot-value actor attribute))


--
__Pascal Bourguignon__ http://www.informat...
â??The factory of the future will have only two employees, a man and a
dog. The man will be there to feed the dog. The dog will be there to
keep the man from touching the equipment.� -- Carl Bass CEO Autodesk

Antsan

10/12/2015 9:17:00 PM

0

Am Montag, 12. Oktober 2015 21:55:26 UTC+2 schrieb informatimago:
> Antsan <thomas.bartscher@gmail.com> writes:
>
> > I'm trying to formalize a PnP RPG by implementing its rules in Common Lisp. For
> > this I need an actor object. Every actor object has the same attributes and I
> > wanted easy access to those attributes by name without having to hardcode
> > everything.
> > What I came up with was this (I'm using fset):
> >
> >
> > ;;;;; attributes.lisp
> > (in-package #:skyscrapers)
> >
> > (defparameter *attributes*
> > (empty-map))
> >
> > (defmacro add-attr (name counter)
> > (declare (type symbol name))
> > `(setf *attributes*
> > (with (with *attributes*
> > ',name ',counter)
> > ',counter ',name)))
> >
> > (defmacro add-attrs (&body names)
> > `(progn
> > ,@(loop for (name counter)
> > on names by #'cddr
> > collect
> > `(add-attr ,name ,counter))))
> >
> > (add-attrs
> > strength intelligence
> > resistance dexterity
> > empathy fortitude
> > charisma secrecy
> > )
> >
> > ;;;; actors.lisp
> > (in-package #:skyscrapers)
> >
> > #.(let ((attrs (mapcar (lambda (attr)
> > `(,attr .
> > ,(gensym (string-downcase (symbol-name attr)))))
> > (convert 'list
> > (domain *attributes*)))))
> > `(progn
> > (defclass actor ()
> > ((,(gensym "exhaustion")
> > :type (integer 0)
> > :initarg :exhaustion
> > :initform 0
> > :reader exhaustion)
> > ,@(mapcar (lambda (attr/sym)
> > `(,(cdr attr/sym)
> > :type (integer 1)
> > :initarg ,(intern (symbol-name (car attr/sym))
> > :keyword)
> > :initform 1))
> > attrs)))
> > (defgeneric get-attr (actor attr))
> > ,@(mapcar (lambda (attr/sym)
> > `(defmethod get-attr ((actor actor)
> > (attr (eql ',(car attr/sym))))
> > (slot-value actor ',(cdr attr/sym))))
> > attrs)))
> >
> > This way I don't pollute my function namespace with accessors, which has been a
> > problem in a similar project before. Another boon is that I now can use GET-ATTR
> > safely with perfect knowledge that I'm not reading something which is not an
> > attribute. Using a function which expects an attribute and supplying EXHAUSTION
> > won't work as it would if I'd be using SLOT-VALUE.
> >
> > But I can't help but think that this is ugly. I'm not quite sure why, though.
> > Does anybody see something wrong with what I did here?
>
> I don't see any justification for using a global variable to hold the
> atribute of a single actor class.
The global variable has another purpose regarding the rules. It maps attributes
to their opposite.
If I coded this by hand I'd need to write down every attribute twice, once when
defining opposites and once for defining the actor class.

> At the very least, you could write a single macro
>
> (define-class-with-attributes actor
> strength intelligence
> resistance dexterity
> empathy fortitude
> charisma secrecy)
>
>
> But I don't see either any worthwhile justification for using get-attr
> instead of slot-value. You can still define it as a "functional
> abstration", but since you don't signal any more specific error than
> slot-value, you could just define it as:
>
> (defun get-attribute (actor attribute) (slot-value actor attribute))
That wouldn't throw an error on
(slot-value actor 'exhaustion)
or whichever other slots not representing attributes I happen to add to the
class.

But moving the whole thing into a macro makes sense.

#.`(define-class-with-hidden actor ()
((exhaustion ...))
(get-attr ,@(convert 'list (domain *attributes*))))


More generally the intent of using something other than slot-value is to mark
slots with their purpose.

Carlos

10/12/2015 9:59:00 PM

0

[Antsan <thomas.bartscher@gmail.com>, 2015-10-12 14:17]
> Am Montag, 12. Oktober 2015 21:55:26 UTC+2 schrieb informatimago:
> > Antsan <thomas.bartscher@gmail.com> writes:
[...]
> > At the very least, you could write a single macro
> >
> > (define-class-with-attributes actor
> > strength intelligence
> > resistance dexterity
> > empathy fortitude
> > charisma secrecy)
> >
> >
> > But I don't see either any worthwhile justification for using
> > get-attr instead of slot-value. You can still define it as a
> > "functional abstration", but since you don't signal any more
> > specific error than slot-value, you could just define it as:
> >
> > (defun get-attribute (actor attribute) (slot-value actor
> > attribute))
> That wouldn't throw an error on
> (slot-value actor 'exhaustion)
> or whichever other slots not representing attributes I happen to add
> to the class.
>
> But moving the whole thing into a macro makes sense.
>
> #.`(define-class-with-hidden actor ()
> ((exhaustion ...))
> (get-attr ,@(convert 'list (domain *attributes*))))
>
>
> More generally the intent of using something other than slot-value is
> to mark slots with their purpose.

If you want to hide "attributes" from slot-value and hide other slots
from #'get-attr, the simplest way IMO, is to have the attributes in a
container (a hashmap, alist, plist, struct, whatever) bound to a slot.

#'get-attr would retrieve the attribute value from that container.

--

Antsan

10/12/2015 10:03:00 PM

0

Am Montag, 12. Oktober 2015 21:55:26 UTC+2 schrieb informatimago:
> But I don't see either any worthwhile justification for using get-attr
> instead of slot-value. You can still define it as a "functional
> abstration", but since you don't signal any more specific error than
> slot-value, you could just define it as:
>
> (defun get-attribute (actor attribute) (slot-value actor attribute))

I found something I find to be quite useful: Using DEFINE-CLASS-WITH-HIDDEN
allows you to use class-local namespaces (or something like that.
I realized I needed an additional slot for every attribute for leveling purposes.
I don't need to come up with new slot names, I can just do this:

#.`(define-class-with-hidden actor ()
((exhaustion :type (integer 0)
:initarg :exhaustion
:initform 0
:reader exhaustion))
(attribute
,@(mapcar (lambda (slot)
`(,slot :type (integer 0)
:initform 1))
(convert 'list
(domain *attributes*))))
(attribute-learned
,@(mapcar (lambda (slot)
`(,slot :type (integer 0)
:initform 0))
(convert 'list
(domain *attributes*)))))

And it works, just like that.
The STRENGTH attribute can be accessed via
(attribute actor 'strength)
and how much has been learned about it via
(attribute-learned actor 'strength)

Antsan

10/12/2015 10:11:00 PM

0

Am Montag, 12. Oktober 2015 23:59:43 UTC+2 schrieb Carlos:
> [Antsan <thomas.bartscher@gmail.com>, 2015-10-12 14:17]
> > Am Montag, 12. Oktober 2015 21:55:26 UTC+2 schrieb informatimago:
> > > Antsan <thomas.bartscher@gmail.com> writes:
> [...]
> > > At the very least, you could write a single macro
> > >
> > > (define-class-with-attributes actor
> > > strength intelligence
> > > resistance dexterity
> > > empathy fortitude
> > > charisma secrecy)
> > >
> > >
> > > But I don't see either any worthwhile justification for using
> > > get-attr instead of slot-value. You can still define it as a
> > > "functional abstration", but since you don't signal any more
> > > specific error than slot-value, you could just define it as:
> > >
> > > (defun get-attribute (actor attribute) (slot-value actor
> > > attribute))
> > That wouldn't throw an error on
> > (slot-value actor 'exhaustion)
> > or whichever other slots not representing attributes I happen to add
> > to the class.
> >
> > But moving the whole thing into a macro makes sense.
> >
> > #.`(define-class-with-hidden actor ()
> > ((exhaustion ...))
> > (get-attr ,@(convert 'list (domain *attributes*))))
> >
> >
> > More generally the intent of using something other than slot-value is
> > to mark slots with their purpose.
>
> If you want to hide "attributes" from slot-value and hide other slots
> from #'get-attr, the simplest way IMO, is to have the attributes in a
> container (a hashmap, alist, plist, struct, whatever) bound to a slot.
>
> #'get-attr would retrieve the attribute value from that container.
>
> --

That doesn't guarantee that the set of attributes isn't changed at runtime.

After realizing the unexpected use of DEFINE-CLASS-WITH-HIDDEN I'm really
happy with the concept.
Thanks, everyone!

Antsan

10/12/2015 10:14:00 PM

0

Here's the macro:

(defmacro define-class-with-hidden (name (&rest superclasses)
(&rest slots)
&body hidden)
(labels ((transform-slot (slot)
(let* ((slot-name (if (consp slot)
(car slot)
slot))
(slot-options (list* :initarg
(intern (symbol-name slot-name)
:keyword)
(and (consp slot)
(cdr slot))))
(slot-sym (gensym (string-downcase (symbol-name
slot-name)))))
(list slot-sym slot-name slot-options))))
(let ((transformed-hidden
(mapcar (lambda (h)
(cons (first h)
(mapcar #'transform-slot
(rest h))))
hidden)))
`(progn
(defclass ,name (,@superclasses)
(,@slots
,@(loop for h in transformed-hidden
append
(loop for slot in (rest h)
collect
`(,(first slot)
,@(third slot))))))
,@(loop for (method . slots)
in transformed-hidden
append
(loop for slot in slots
collect
`(defmethod ,method ((obj ,name)
(slot (eql ',(second slot))))
(slot-value obj ',(first slot)))))))))

Pascal J. Bourguignon

10/12/2015 11:20:00 PM

0

Antsan <thomas.bartscher@gmail.com> writes:

>> (defun get-attribute (actor attribute) (slot-value actor attribute))
> That wouldn't throw an error on
> (slot-value actor 'exhaustion)
> or whichever other slots not representing attributes I happen to add to the
> class.

Of course it would, since you you don't have slot named exhaustion.
(you used a gensym).


> But moving the whole thing into a macro makes sense.
>
> #.`(define-class-with-hidden actor ()

In general, try to avoid #. In this case, you can do with a normal
macro, so don't do #.


> ((exhaustion ...))

Why stopping using uninterned symbols?

((#:exhaustion â?¦)

> (get-attr ,@(convert 'list (domain *attributes*))))
>
>
> More generally the intent of using something other than slot-value is to mark
> slots with their purpose.

--
__Pascal Bourguignon__ http://www.informat...
â??The factory of the future will have only two employees, a man and a
dog. The man will be there to feed the dog. The dog will be there to
keep the man from touching the equipment.� -- Carl Bass CEO Autodesk

Carlos

10/13/2015 5:32:00 AM

0

[Antsan <thomas.bartscher@gmail.com>, 2015-10-12 15:10]
> Am Montag, 12. Oktober 2015 23:59:43 UTC+2 schrieb Carlos:
> > [Antsan <thomas.bartscher@gmail.com>, 2015-10-12 14:17]
> > > Am Montag, 12. Oktober 2015 21:55:26 UTC+2 schrieb informatimago:
> > > > Antsan <thomas.bartscher@gmail.com> writes:
> > [...]
> > > > At the very least, you could write a single macro
> > > >
> > > > (define-class-with-attributes actor
> > > > strength intelligence
> > > > resistance dexterity
> > > > empathy fortitude
> > > > charisma secrecy)
> > > >
> > > >
> > > > But I don't see either any worthwhile justification for using
> > > > get-attr instead of slot-value. You can still define it as a
> > > > "functional abstration", but since you don't signal any more
> > > > specific error than slot-value, you could just define it as:
> > > >
> > > > (defun get-attribute (actor attribute) (slot-value actor
> > > > attribute))
> > > That wouldn't throw an error on
> > > (slot-value actor 'exhaustion)
> > > or whichever other slots not representing attributes I happen to
> > > add to the class.
> > >
> > > But moving the whole thing into a macro makes sense.
> > >
> > > #.`(define-class-with-hidden actor ()
> > > ((exhaustion ...))
> > > (get-attr ,@(convert 'list (domain *attributes*))))
> > >
> > >
> > > More generally the intent of using something other than
> > > slot-value is to mark slots with their purpose.
> >
> > If you want to hide "attributes" from slot-value and hide other
> > slots from #'get-attr, the simplest way IMO, is to have the
> > attributes in a container (a hashmap, alist, plist, struct,
> > whatever) bound to a slot.
> >
> > #'get-attr would retrieve the attribute value from that container.
> >
> > --
>
> That doesn't guarantee that the set of attributes isn't changed at
> runtime.

You can hide the slot with your gensym trick. Then, you can only access
the container through your method.

> After realizing the unexpected use of DEFINE-CLASS-WITH-HIDDEN I'm
> really happy with the concept.
> Thanks, everyone!

Here you have to repeat

,@(mapcar (lambda (slot)
`(,slot :type (integer 0)
:initform 0))
(convert 'list
(domain *attributes*)))

every time you want to reuse the set of attributes (and you end up
having duplicated initargs -- how does that work?).

--

Antsan

10/13/2015 12:21:00 PM

0

Am Dienstag, 13. Oktober 2015 07:32:08 UTC+2 schrieb Carlos:
> You can hide the slot with your gensym trick. Then, you can only access
> the container through your method.
Hm, yes.

> > After realizing the unexpected use of DEFINE-CLASS-WITH-HIDDEN I'm
> > really happy with the concept.
> > Thanks, everyone!
>
> Here you have to repeat
>
> ,@(mapcar (lambda (slot)
> `(,slot :type (integer 0)
> :initform 0))
> (convert 'list
> (domain *attributes*)))
I added functionality for that, now it can be done with
#.`(define-class-with-hidden* actor ()
...
((learned exhaustion)
,@(mapcar (lambda (slot)
`(,slot :type (integer 0)
:initform 0))
#1#)))

The slots for ATTRIBUTE have an initform of 1, the slots for LEARNED and
EXHAUSTION have 0.

> every time you want to reuse the set of attributes (and you end up
> having duplicated initargs -- how does that work?).
I realized that directly after posting the macro. The respective initargs are
now named :<access-method>-<slot-name>
So, the initarg for the slot STRENGTH accessible via ATTRIBUTE would be
:ATTRIBUTE-STRENGTH

Am Dienstag, 13. Oktober 2015 01:20:20 UTC+2 schrieb informatimago:
> Antsan <thomas.bartscher@gmail.com> writes:
>
> >> (defun get-attribute (actor attribute) (slot-value actor attribute))
> > That wouldn't throw an error on
> > (slot-value actor 'exhaustion)
> > or whichever other slots not representing attributes I happen to add to the
> > class.
>
> Of course it would, since you you don't have slot named exhaustion.
> (you used a gensym).
Oh, right.

> > But moving the whole thing into a macro makes sense.
> >
> > #.`(define-class-with-hidden actor ()
>
> In general, try to avoid #. In this case, you can do with a normal
> macro, so don't do #.
I don't see how, or at least I think changing the macro to accept variables to
be expanded inline would be strange.

> > ((exhaustion ...))
>
> Why stopping using uninterned symbols?
>
> ((#:exhaustion ...)
I don't know. Also thanks for pointing out that I don't need to use GENSYM here.