Re: Program problem

[Stephen Berman]

On Wed, 29 Nov 2023 00:16:46 +0000 (UTC) Lewis Creary via Users list for the 
GNU Emacs text editor <help-gnu-emacs@gnu.org> wrote:

> (It has been suggested to me by someone on the above mailing list thatthe
> following content would be appropriate for this list.)
> My purpose here is to discuss a bug I've discovered in an emacs lispfunction,
> fill-rows, that I've written (shown below).  The main ideaof this function is
> to help solvers of Sudoku puzzles who want to keeptrack of the numbers they've
> already entered into a puzzle.  This willhelp them to backtrack when they
> change some of those numbers.  It'simportant to understand the documentation
> string of this function(included in the function definition) before reading
> the code.

Your code appeared badly formatted, so I've reformatted it:

(defun fill-rows (row-nums change-strings pzl-form)
  "This fn takes as arguments a list of row indices, a list of an
equal number, s, of 9-digit strings, and a puzzle form (i.e, an
sexp of the form:

\((n n n n n n n n n)
  (n n n n n n n n n)
  (n n n n n n n n n)
  (n n n n n n n n n)
  (n n n n n n n n n)
  (n n n n n n n n n)
  (n n n n n n n n n)
  (n n n n n n n n n)
  (n n n n n n n n n)),

where n is a number between 0 and 9).  The fn returns as value
the result of filling in row m of the argument puzzle form with
numbers specified by the mth 9-digit string."
  (let* ((row-index 0)         
         (row-num-index 0)
         (row-str (nth row-index change-strings))         
         (row-num (nth row-num-index row-nums))
         (change-nums nil)         
         (str-pos 0))
    ;; (comment) cycle through the change-strings
    (while (and (< row-index (length row-nums))
                (< str-pos 9))
        ;; cycle through row-str, contributing to change-list
      (while (< str-pos 9)
        (setq change-nums
              (append change-nums          
                      (list (string-to-number              
                             (substring row-str str-pos (1+ str-pos)))))
              row-str (nth row-index change-strings))
        (setcar (car (nthcdr row-num pzl-form))
                (nth str-pos change-nums))
        (cl-incf str-pos))
      (cl-incf row-index)
      (cl-incf row-num-index))
    pzl-form))

There are a number of problems here.  One is that you increment str-pos
in the inner while-loop but don't reset it after leaving that loop, so
that when you start the second run through the outer loop, the condition
(< str-pos 9) is false and the function returns pzl-form after only one
run.  And you keep resetting row-str in the inner loop to the same
element of change-strings but don't change it at the end, so you never
get to the other strings in change-strings.  Another problem is you are
changing the same element of pzl-form each time through the outer loop,
because rownum doesn't change.  Also, in the setcar sexp, the first
argument is changing just the first element of the list because you take
its car, and the second argument changes it each time to the next
element in change-nums.

> I'll very much appreciate receiving any proposed bugfixes or other 
> suggestions you may have.

Here is a variant of your code that returns the result you want; rather
the an outer while-loop it uses dolist to loop directly over the
row-nums.  I've commented out unneeded parts of your original code:

(defun fill-rows (row-nums change-strings pzl-form)
  ""
  (let ((row-index 0)         
        ;; (row-num-index 0)
        ;; (row-str (nth row-index change-strings))         
        ;; (row-num (nth row-num-index row-nums))
        (row-str "")
        (change-nums nil)         
        (str-pos 0))
    ;; (comment) cycle through the change-strings
    (dolist (i row-nums)
      (setq row-str (nth row-index change-strings))
        ;; cycle through row-str, contributing to change-list
      (while (< str-pos 9)
        (setq change-nums
              (append change-nums          
                      (list (string-to-number              
                             (substring row-str str-pos (1+ str-pos))))))
        (cl-incf str-pos))
      (setcar (nthcdr i pzl-form) change-nums)
      (setq str-pos 0
            change-nums nil)
      (cl-incf row-index)
      ;; (cl-incf row-num-index)
      )
    pzl-form))

As food for thought and for you to experiment with, here is an
alternative implementation that uses some higher-level elisp functions
to avoid lower-level functions like setcar and nthcdr, and hence shorten
the code.  Also, it restructures the data into a form that seems to me
better suited to what you want, given your description.  The doc string
tries to explain that:

(defun fill-rows (dim rows)
  "Fill a square matrix of dimensions DIM with values from ROWS.
DIM is an integer specifying the number of rows and columns in
the matrix.  ROWS is an alist with elements (ROW . ELEMENTS),
where ROW specifies the (1-based) row number of the matrix and
ELEMENTS is a string whose characters are numerals specifying the
contents of the cells of ROW (in which each numeral in ELEMENTS
has been converted to the corresponding integer).  Each cell of
the rows not specified by ROWS is filled with 0.  Since the
result should be a square matrix, the lengths of ROWS and
ELEMENTS must equal DIM and each ROW must be an integer between 1
and DIM (inclusive)."
  (let (matrix)
    (dotimes (i dim)
      (let* ((rpair (assq (1+ i) rows))
             (row (if rpair
                       (mapcar #'string-to-number
                               (mapcar #'string
                                       (string-to-list (cdr rpair))))
                     (make-list dim 0))))
        (setq matrix (append matrix (list row)))))
    matrix))

Here's the invocation with your test data:

(fill-rows 9 '((1 . "123456789") (3 . "987654321") (5 . "123498765")))

I hope that was helpful.

Steve Berman