Common Lisp object setter function style

Question

I am attempting to write a function that takes a list of user-defined objects called nodes to generate connections between them. Each node object has a slot for its unique number ('num') and a slot for a list of numbers that act as the edges between nodes ('edges'). +max-edges+ is an integer that defines how many times an edge pairing will be attempted, and +max-rooms+ is the number of nodes within the node-list getting passed into the function (and is always < 50).

Here are two versions of a function that attempts to solve this problem:

(defun connect-nodes (node-list)
  "Given a NODE-LIST, repeats for +MAX-EDGES+ amount of times
to alter NODE-LIST in-place to connect randomly generated edges to nodes."
  (loop repeat +max-edges+
     do (let ((begin-node (random +max-rooms+))
              (end-node (random +max-rooms+)))
          (when (not (= begin-node end-node))
            (setf (slot-value (nth begin-node node-list) 'edges)
                  (cons end-node
                        (slot-value (nth begin-node node-list) 'edges)))
            (setf (slot-value (nth end-node node-list) 'edges)
                  (cons begin-node
                        (slot-value (nth end-node node-list) 'edges))))))))

(defun connect-nodes% (node-list)
  "Given a NODE-LIST, repeats for +MAX-EDGES+ amount of times
to alter NODE-LIST in-place to connect randomly generated edges to nodes."
  (loop repeat +max-edges+
     do (let ((begin-node (random +max-rooms+))
              (end-node (random +max-rooms+)))
          (when (not (= begin-node end-node))
            (let ((begin-node-lst (slot-value (nth begin-node node-list) 'edges))
                  (end-node-lst (slot-value (nth end-node node-list) 'edges)))
              (setf begin-node-lst (cons end-node begin-node-lst))
              (setf end-node-lst (cons begin-node end-node-lst)))))))

(connect-nodes) works as expected, but the last two lines seem stylistically long and lookup the slot value for the object being setf'd twice, which I imagine could be a performance issue.

(connect-nodes%) attempts to solve the double lookup by binding the location in a lexically-scoped place, but does not actually alter the node-list argument in-place. No changes are made because each location in the let binding (begin-node-lst and end-node-lst) is binding only lexically and going out of scope after both setfs.

So I am asking for clarification on a few points:

• Is my understanding of why the second function fails to alter the argument list correct?
• Is the first function stylistically correct? Is there a better way to write this function that doesn't lookup the slot value twice for setf or is this acceptable for small length lists?

I am running slime + emacs + sbcl if that factors into your answer.

EDIT: Here's what I ended up going with for a list-version of a connect-nodes function thanks to the advice from the answers to my question. I am working on a version that works on vectors, hence this version of connect-nodes is a method on a generic function:

(defmethod connect-nodes ((node-list list))
  "Given a NODE-LIST, repeats for +MAX-EDGES+ amount of times
to alter NODE-LIST in-place to connect randomly generated edges to nodes."
  (loop repeat +max-edges+
     do (let ((begin-node (random +max-rooms+))
              (end-node (random +max-rooms+)))
          (when (not (= begin-node end-node))
            (push end-node (edges (nth begin-node node-list)))
            (push begin-node (edges (nth end-node node-list)))))))

Answer 1

Your understanding about the second function is correct.

You may want to store actual nodes in the edges slot instead of node numbers. Then, instead of binding local variables to the node list inside of the two nodes that you want to connect, though, you can bind them to the nodes themselves, which would also look better than the repeated invocations of nth inside of the setf forms. You could then also directly operate with the nodes when you access edges instead of having to perform an extra lookup.

To improve the style of the first function, I'd suggest two things:

Use push instead of (setf ... (cons thing ...))

slot-value is an accessor, and as such, it can be used as a place. setf is one way to change the value of a place, but Common Lisp defines other operations on places. The pattern that you are using here is implemented in the macro push. By using it, you can simplify your expressions significantly:

(push end-node (slot-value (nth begin-node node-list) 'edges))

Define an accessor for edges instead of using slot-value

slot-value should be used rarely, and as a low-level mechanism, because it is verbose and less flexible than using a named accessor. slot-value also puts the important part of the access, the name of the slot, to the end of the expression, which often makes the code harder to read. In your case, I would name the accessor edges in the class definition:

(edges :initform nil :accessor edges)

That would make your first version more readable:

(push end-node (edges (nth begin-node node-list)))

Answer 2

Instead of:

(setf (slot-value (nth begin-node node-list) 'edges)
      (cons end-node (slot-value (nth begin-node node-list) 'edges)))

You can write:

(push end-node (slot-value (nth begin-node node-list) 'edges))

Why is the following not working as expected?

(let ((begin-node-lst (slot-value (nth begin-node node-list) 'edges))
      (end-node-lst (slot-value (nth end-node node-list) 'edges)))
  (setf begin-node-lst (cons end-node begin-node-lst))
  (setf end-node-lst (cons begin-node end-node-lst)))

You write: attempts to solve the double lookup by binding the location.

That does not work. You can bind locations. You can only bind values. LET binds the values of forms to variables.

In Common Lisp there is the idea of a place. Many side-effect macros work with places: SETF and PUSH are examples. A place is only the source of the accessing code, not a real first-class object

Examples for places:

• foo as a variable
• (aref foo 10) as an array access
• (slot-value object 'foo) as a slot access
• (slot-value (find-object *somewhere* 'foo) 'bar) as a slot access...

Macros like SETF find out at macroexpansion time, based on the source of the accessing form, what form for a setting form to generate. It can't look at things like bindings, where the bindings form is coming from.

In this case one would usually retrieve the object (typically CLOS object or structure) from the data structure, keep a reference to the object and then change the slot value using SLOT-VALUE or WITH-SLOTS. Alternatively use an accessor.

(setf (slot-value person 'name)  "Eva Lu Ator")
(setf (slot-value person 'group) :development)

would be

(with-slots (name group) person
  (setf name  "Eva Lu Ator"
        group :development))

General Advice:

Also note in your function the confusion what a node is. Is it an object of type node or is it a number? If it is a number, I would name the variable node-number.

Avoid NTH and lists. If you need random access, use vectors.

Either use node objects directly (and not numbers for those) or use symbols for them: node-123 and link the node symbol to the node object in some registry. You might want to use numbers only in some cases...

I would write code like this:

(defun connect-nodes (node-vector)
  "Given a NODE-VECTOR, repeats for +MAX-EDGES+ amount of times to connect
nodes via randomly generated edges."
  (loop repeat +max-edges+
        for begin-node-number = (random +max-rooms+) and
            end-node-number   = (random +max-rooms+)
        when (/= begin-node-number end-node-number) do
        (let ((begin-node (aref node-vector begin-node-number))
              (end-node   (aref node-vector begin-node-number)))
          (push end-node   (slot-value begin-node 'edges))
          (push begin-node (slot-value end-node   'edges))))
  node-vector)