Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My program calculates the determinant of a matrix of any degree. If you really can't understand my comments or indentifiers please let me know.

//;;MakeShift make element number 'ind' the head of list:
(defun MakeShift (ind L buf)
    (if (= ind 1) (cons (car L) (append buf (cdr L)))
       (makeReplace (- ind 1) (cdr L) (append buf (list (car L))))
    )
)

//;;Shift call MakeShift:
(defun Shift (ind L) (MakeShift ind L nil))

//;;makeTransp make transposition of two elements list:
(defun makeTransp (L) (cons (cadr L) (cons (car L) nil)))

//;;PushForEach put element elem into a heads of all lists of L:
(defun PushForEach (elem L)
    (if (null L) nil (cons (cons elem (car L)) (PushForEach elem (cdr L)))
    )
)

//;;MakeTranspositions create a list of all transpositions
//;;using first transposition like '(1 2 3 ...).
//;;transpNum is transpositions  number and numOfElem
//;;is amount of elements in transposition:
(defun MakeTranspositions (transp transpNum numOfElem)
    (cond ((> transpNum numOfElem) nil)
        ((= numOfElem 2) (cons transp (cons (makeTransp transp) nil)))
        (T (append (PushForEach (car transp) (MakeTranspositions (cdr transp) 1 (- numOfElem 1)))
            (MakeTranspositions (Shift (+ transpNum 1) transp) (+ transpNum 1) numOfElem)))
    )
)

//;;MakeFirstTransp make a first transpositiion like '(1 2 3 ... )
//;;which has number of element equal matrix degree:
(defun MakeFirstTransp (matrixDegree transp)
    (if (= matrixDegree 0) transp
        (MakeFirstTransp (- matrixDegree 1) (cons matrixDegree transp))
    )
)

//;;Transpositions make all transpositions of matrix using MakeTranspositions:
(defun Transpositions (matrixDegree)
    (MakeTranspositions (MakeFirstTransp matrixDegree nil) 1 matrixDegree)
)

//;;GetCol return elemnt number col in row (row belong to matrix):
(defun GetCol (col rowVector)
    (if (= col 1) (car rowVector)
        (GetCol (- col 1) (cdr rowVector))
    )
)

//;;GetElem return element a[row][col] which belong to matrix:
(defun GetElem (row col matrix)
    (if (= row 1) (GetCol col (car matrix))
        (GetElem (- row 1) col (cdr matrix))
    )
)

//;;CheckFirst check first element in transposition (cons first transp) for even:
(defun CheckFirst (first transp)
    (cond ((null transp) 1)
        ((< first (car transp)) (CheckFirst first (cdr transp)))
        (T (* -1 (CheckFirst first (cdr transp))))
    )
)

//;;Sign return sign of transposition (1 or -1):
(defun Sign (transp)
    (if (null (cdr transp)) 1
        (* (CheckFirst (car transp) (cdr transp)) (Sign (cdr transp)))
    )
)

//;;Product return product of elements of matrix by transposition transp:
(defun Product (matrix transp) (GetProduct matrix 1 transp))

//;;GetProduct are called by Product:
(defun GetProduct (matrix ind transp)
    (if (null transp) 1
        (* (GetElem ind (car transp) matrix)
            (GetProduct matrix (+ ind 1) (cdr transp))
        )
     )
)

//;;GetSumm return summ of all products by transpositions whith their signs:
(defun GetSumm (matrix transps)
    (if (null transps) 0
        (+ (* (Sign (car transps)) (Product matrix (car transps)))
            (GetSumm matrix (cdr transps))
        )
    )
)

//;;Determinant call GetSumm:
(defun Determinant (matrix matrixDegree)
    (GetSumm matrix (Transpositions matrixDegree))
)

//;;So, programm work fast.
share|improve this question
1  
You may want to document the data structures used and provide an example how the code is supposed to be used. –  Rainer Joswig Feb 9 '13 at 11:50
add comment

1 Answer

up vote 2 down vote accepted

The first thing is to learn proper indentation.

//;;PushForEach put element elem into a heads of all lists of L:
(defun PushForEach (elem L)
    (if (null L) nil (cons (cons elem (car L)) (PushForEach elem (cdr L)))
    )
)

What's wrong with the code layout?

  • Common Lisp has built-in documentation features. A documentation string can be placed inside the function and can be retrieved with the DOCUMENTATIONfunction.
  • trailing parentheses are a big DON'T DO THAT. It just wastes space without adding much information. Your editor will count parentheses or will show the corresponding parentheses.
  • put constructs, when they are too long on a line, over several lines.
  • don't use CamelCase, use hyphens

Let's do the editing step by step:

(defun PushForEach (elem L)
  "put element elem into a heads of all lists of L"
    (if (null L) nil (cons (cons elem (car L)) (PushForEach elem (cdr L)))
    )
)

(defun PushForEach (elem L)
  "put element elem into a heads of all lists of L"
    (if (null L) nil (cons (cons elem (car L)) (PushForEach elem (cdr L)))))

Now we get the version which is best readable:

(defun push-for-each (elem L)
  "put element elem into a heads of all lists of L"
  (if (null L)
      nil
      (cons (cons elem (car L))
            (push-for-each elem (cdr L)))))

Above function has a documentation string and useful code layout.

Next: can we write it better? You can bet that basic recursion in Lisp has been provided as a higher-order function. Applying a function over all items in a list is called mapping. The basic mapping function is MAPCAR.

(defun push-for-each (elem L)
  "put element elem into a heads of all lists of L"
  (mapcar #'(lambda (item)
              (cons elem item))
          L))
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.