wiki:Refactoring
close Warning: Can't synchronize with repository "(default)" (/project/cl-openid/svn does not appear to be a Subversion repository.). Look in the Trac log for more information.

Issues mentioned on this page are all resolved, page available as archive only.

Lets collect here things to consider during the planned refactoring.

  • postfix. relying-party.lisp uses URI postfix to identify an OpenID authentication transaction. It was really difficult to understand from the code. The word 'postfix' is uncertain, lets rename the paramether to something like auth-transact-id done
  • id. It was difficult to understand from the code what is meant by the object named 'id'. In this object we store discovered information and other properties (return_to URL, timestamp). Naming it 'id' is confusing, because id (identifier) is expected to an URI like http://someone.livejournal.com/ (or XRI). Lets rename this object (i.e. function parameters etc.) to something like 'auth-transaction'. done; renamed to AUTH-PROCESS
  • handle-indirect-reply. The term used by the spec is 'indirect response'. Lets rename the function to handle-indirect-response, it is better when one thing is always named by only one term. done
  • Thread safety. Working with all global objects must be thread safe. done
  • Usage of package to store unique auth transaction IDs is unnecessary and it is a memory leak (interned symbols will remain in memory). To generate unique keys it is sufficient to use a counter. done
  • It is unclear to me why eval-when is used in the relying-party.lisp when defining the html function and +openid-input-form+. IMHO plain definitions will be OK here.

They won't. It is because (1) on re-evaluation of DEFCONSTANT form, if new value is not EQL old value, implementation behaviour is unspecified and many implementations (including SBCL and CCL) signal an error -- see http://sbcl.sourceforge.net/manual/Defining-Constants.html . (2) the HTML function is used in later definition in the same file, recompiling from scratch fails. Try to compile or load load following file:

(defun foo (x)
  (concatenate 'string "foo" x "foo"))
(defconstant +foo+ (foo "test"))

I agree with rest of remarks, and will take them into account when refactoring.

--maciej

  • (intern (string :cl-openid) :keyword))) at the bottom of cl-openid.asd. We may just say :cl-openid. done
Last modified 14 years ago Last modified on 08/18/08 18:15:40