Hi all,
I've just written a function to walk the output from PHTML collecting
elements as needed, and was wondering if someone could please glance
over it and tell me if I'm abusing Lisp or am ignorant of a better
idiom? I've used four IFs in a 16 line function, which is the kind of
nested logic that I'll probably not be able to grok in a week.
If anyone is able to help, I've pasted the code here -
http://paste.lisp.org/display/39860
I'm not sure if the pastes on that site expire or not.
Thanks in advance for any advice,
Liam Clarke
> I've just written a function to walk the output from PHTML collecting
> elements as needed, and was wondering if someone could please glance
> over it and tell me if I'm abusing Lisp or am ignorant of a better
> idiom? I've used four IFs in a 16 line function, which is the kind of
> nested logic that I'll probably not be able to grok in a week.
Here's a quick-and-dirty rewrite -- I haven't tested it against the
sample input, but it illustrates a couple of things:
(defun walker (parse-tree tag &optional acc)
(if (null parse-tree)
acc
(destructuring-bind (node &body rest-of-tree) parse-tree
(walker rest-of-tree
tag
(if (consp node)
(if (eql (car node) tag)
(cons (if (eql tag :SCRIPT)
;;To pick up script body due to way
PHTML returns them
;;separate from script tag list
(car rest-of-tree)
node)
acc)
(walker node tag acc))
acc)))))
Notes:
(1) &optional defaults to nil so you'd don't need to explicitly set
acc to '()
(2) (if (not (null parse-tree)) ...) is really the same as (if parse-
tree ...)
But I've written it as (if (null parse-tree) ...) because I like to
see the condition which ends the recursion first -- admittedly, this
is just a personal quirk of mine rather than "idiomatic" lisp, but
it's a bit easier to read later, since you know where the critical
logic is
(3) Since all the branches result in a call to (walker rest-of-tree
tag ...) I've started with that, and placed the conditional logic
*inside* that call, to affect the value of acc in the same way the
original did
(4) (first) instead of (car) -- a style point, but easier to remember
(usually!)
And that's without digging too much into the parsing logic itself;
there are might be some more improvements, either in logic/readability
or efficiency, which could be made.
···········@gmail.com" <··········@gmail.com> writes:
> Hi all,
>
> I've just written a function to walk the output from PHTML collecting
> elements as needed, and was wondering if someone could please glance
> over it and tell me if I'm abusing Lisp or am ignorant of a better
> idiom? I've used four IFs in a 16 line function, which is the kind of
> nested logic that I'll probably not be able to grok in a week.
Well, the code was short enough to put directly in the message, which
would make it more convenient:
(defun walker (parse-tree tag &optional (acc ()))
(if (not (null parse-tree)) ;; [1]
(destructuring-bind (node &body rest-of-tree) parse-tree
(if (consp node) ;; [2]
(walker rest-of-tree
tag
(if (eql (car node) tag) ;; [3]
(cons (if (eql tag :SCRIPT) ;; [4]
;; To pick up script body due to way
;; PHTML returns them separate from
;; script tag list
(car rest-of-tree)
node)
acc)
(walker node tag acc)))
(walker rest-of-tree tag acc)))
acc))
My comments:
[1] I generally don't like double negation-like structures such as
(not (null ...)), so I would either use (if parse-tree ...)
or else reverse the clauses and use
(if (null parse-tree)
acc
...)
[2] The only difference in the bodies of the branches is the
computation of the accumulator argument. So I would want
to get rid of this level and either roll this into a COND
for computing the accumulator, or even better, pull that
computation into a separate variable or separate function:
(defun walker (parse-tree tag &optional (acc ()))
(if (null parse-tree)
acc
(destructuring-bind (node &body rest-of-tree) parse-tree
(walker rest-of-tree
tag
(collect-node-result node tag rest-of-tree acc)))))
(defun collect-node-result (node tag rest-of-tree acc)
(if (and (consp node)
(eql (car node) tag))
;; To pick up script body due to way PHTML returns
;; them separate from script tag list
(if (eql tag :SCRIPT)
(cons (car rest-of-tree) acc)
(cons node acc))
acc))
--
Thomas A. Russ, USC/Information Sciences Institute
Thomas A. Russ wrote:
> ···········@gmail.com" <··········@gmail.com> writes:
>
>
>>Hi all,
>>
>>I've just written a function to walk the output from PHTML collecting
>>elements as needed, and was wondering if someone could please glance
>>over it and tell me if I'm abusing Lisp or am ignorant of a better
>>idiom? I've used four IFs in a 16 line function, which is the kind of
>>nested logic that I'll probably not be able to grok in a week.
>
>
> Well, the code was short enough to put directly in the message, which
> would make it more convenient:
>
> (defun walker (parse-tree tag &optional (acc ()))
> (if (not (null parse-tree)) ;; [1]
> (destructuring-bind (node &body rest-of-tree) parse-tree
> (if (consp node) ;; [2]
> (walker rest-of-tree
> tag
> (if (eql (car node) tag) ;; [3]
> (cons (if (eql tag :SCRIPT) ;; [4]
> ;; To pick up script body due to way
> ;; PHTML returns them separate from
> ;; script tag list
> (car rest-of-tree)
> node)
> acc)
> (walker node tag acc)))
> (walker rest-of-tree tag acc)))
> acc))
>
>
> My comments:
> [1] I generally don't like double negation-like structures such as
> (not (null ...)), so I would either use (if parse-tree ...)
> or else reverse the clauses and use
> (if (null parse-tree)
> acc
> ...)
> [2] The only difference in the bodies of the branches is the
> computation of the accumulator argument. So I would want
> to get rid of this level and either roll this into a COND
> for computing the accumulator, or even better, pull that
> computation into a separate variable or separate function:
>
> (defun walker (parse-tree tag &optional (acc ()))
> (if (null parse-tree)
> acc
> (destructuring-bind (node &body rest-of-tree) parse-tree
> (walker rest-of-tree
> tag
> (collect-node-result node tag rest-of-tree acc)))))
>
> (defun collect-node-result (node tag rest-of-tree acc)
> (if (and (consp node)
> (eql (car node) tag))
> ;; To pick up script body due to way PHTML returns
> ;; them separate from script tag list
> (if (eql tag :SCRIPT)
> (cons (car rest-of-tree) acc)
> (cons node acc))
> acc))
>
Excellent. Agreed on most points, tho surprised we would tolerate &body
where &rest should be. And I think you missed the beauty of flet, which
spares us from creating new functions just to get a one-off job.
I thought the whole destructuring-bind was a bit precious just to avoid
car this and cdr that, and I really do not need something like tree to
be self-documented as verbosely as parse tree. EQ is fine for symbols,
let's learn that and use it (fun thread on this recently). And I think I
found a way to abbreviate the comment. Seems like a small reminder will
suffice and keep the line count down.
Possibly botched in editing:
(defun walker (tree tag &optional (acc ()))
(flet ((collect-node-result (node tag rest acc)
(if (and (consp node)
(eq (car node) tag))
(if (eq tag :SCRIPT) ;; PHTML is retarded
(cons (car rest) acc)
(cons node acc))
acc)))
(if tree ;; I hate negative tests
(walker (rest tree) tag
(collect-node-result (car tree) tag (rest tree) acc))
acc)))
kt
--
http://www.theoryyalgebra.com/
"Algebra is the metaphysics of arithmetic." - John Ray
"As long as algebra is taught in school,
there will be prayer in school." - Cokie Roberts
"Stand firm in your refusal to remain conscious during algebra."
- Fran Lebowitz
"I'm an algebra liar. I figure two good lies make a positive."
- Tim Allen
··········@gmail.com wrote:
> If anyone is able to help, I've pasted the code here -
> http://paste.lisp.org/display/39860
>
> I'm not sure if the pastes on that site expire or not.
It's not very long, so you could post it here.
Do you have some example input list?
--
Frank Buss, ··@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de
On Apr 18, 2:21 am, Frank Buss <····@frank-buss.de> wrote:
> ··········@gmail.com wrote:
> > If anyone is able to help, I've pasted the code here -
> >http://paste.lisp.org/display/39860
>
> > I'm not sure if the pastes on that site expire or not.
>
> It's not very long, so you could post it here.
>
> Do you have some example input list?
>
> --
> Frank Buss, ····@frank-buss.dehttp://www.frank-buss.de,http://www.it4-systems.de
Hi Frank,
I certainly do - I'm not sure what an appropriate format is, but
hopefully it's cut and pastable from that paste website.
http://paste.lisp.org/display/39866
Let me know if you'd prefer it provided in a different format. This is
a sample of the website I'm scraping.
Thanks,
Liam Clarke
··········@gmail.com wrote:
> I've just written a function to walk the output from PHTML collecting
> elements as needed, and was wondering if someone could please glance
> over it and tell me if I'm abusing Lisp or am ignorant of a better
> idiom? I've used four IFs in a 16 line function, which is the kind of
> nested logic that I'll probably not be able to grok in a week.
You have two parts: walking though all elements of the tree and collection
the right ones. To make the logic easier to read, we can split the two
parts. First a function, which calls for every node a function with the
currently visited node and the rest of the tree:
(defun walk-all (list fun)
(when list
(let ((node (first list))
(rest (rest list)))
(when (consp node)
(funcall fun node rest)
(walk-all node fun))
(walk-all rest fun))))
Now we can use this function to collect all nodes we want:
(defun walker (parse-tree tag)
(let (result)
(walk-all parse-tree
(lambda (node rest)
(let ((parsed-tag (car node)))
(when (eq parsed-tag tag)
(if (eq tag :SCRIPT)
(push (car rest) result)
(push node result))))))
result))
A simplified example:
(defparameter *x*
'(:HTML
((:HTML)
((:BODY
:BGCOLOR
"#F5F2ED")
((:TABLE)
(:TR
((:TD :WIDTH "49" :BGCOLOR "#333399")
((:IMG :SRC "foo.jpg")))
((:TD :WIDTH "30" :BGCOLOR "#CC3301")
((:IMG :SRC "bar.jpg")))))))))
CL-USER > (walker *x* :td)
((:TD :WIDTH "30" :BGCOLOR "#CC3301") (:TD :WIDTH "49" :BGCOLOR "#333399"))
Instead of pushing the node to the result list, you could pass another
function to the walker function, which is called with the filtered node.
Then you don't need the intermediate list.
Combining such higher level functions to more complex higher level
functions can be much fun:
http://www.frank-buss.de/lisp/texture.html
(I should update this sometime to the Lispbuilder SDL bindings)
--
Frank Buss, ··@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de
··········@gmail.com wrote:
> Hi all,
>
> I've just written a function to walk the output from PHTML collecting
> elements as needed, and was wondering if someone could please glance
> over it and tell me if I'm abusing Lisp or am ignorant of a better
> idiom? I've used four IFs in a 16 line function, which is the kind of
> nested logic that I'll probably not be able to grok in a week.
>
> If anyone is able to help, I've pasted the code here -
> http://paste.lisp.org/display/39860
>
> I'm not sure if the pastes on that site expire or not.
>
> Thanks in advance for any advice,
>
FWIW
(defun walking-accumulator (tree tag)
(loop for node = (pop tree)
while node
when (and (consp node) (eql (car node) tag))
collect (if (eql tag :script) (pop tree) node)
when (consp node)
append (walking-accumulator node tag)))
CL-USER 14 > (defparameter *x*
'(:HTML
((:HTML)
((:BODY
:BGCOLOR
"#F5F2ED")
((:TABLE)
(:TR
((:TD :WIDTH "49" :BGCOLOR "#333399")
((:IMG :SRC "foo.jpg")))
((:TD :WIDTH "30" :BGCOLOR "#CC3301")
((:IMG :SRC "bar.jpg")))))))))
*X*
CL-USER 15 > (walking-accumulator *x* :td)
((:TD :WIDTH "49" :BGCOLOR "#333399") (:TD :WIDTH "30" :BGCOLOR "#CC3301"))
Wade