"Dmitry Jouravlev" <·········@aussiemail.com.au> writes:
> Hi there,
>
> I'm learning lisp, and i have written my first program - it calculates
> frequencies of words and letters in a text file.
>
> I would appreciate if anyone wanted to comment on anything that can be
> improved in it.
>
> ;---------------------------------------------------------------
> (defclass frequency-counter ()
> ((counter :initform (make-hash-table :test 'equal))))
>
> ;---------------------------------------------------------------
> (defmethod add-counter ((c frequency-counter) word)
> (with-slots (counter) c
> (incf (gethash (string-downcase word) counter 0))))
You could use (make-hash-table :test (function equalp)) [and below use
(function string-lessp) instead of (function string<)] and forget
about string-downcase.
> ;---------------------------------------------------------------
> (defmacro sort-in-place (var &rest params)
> `(setq ,var (sort ,var ,@params)))
>
> ;---------------------------------------------------------------
> (defmethod print-frequency-counter ((c frequency-counter) freq-sort)
> (with-slots (counter) c
> (let ((freq-list ())
> (sort-function (if freq-sort #'> #'string<))
> (sort-key (if freq-sort #'second #'first)))
> (maphash
> #'(lambda (k v)
> (push (list k v) freq-list))
> counter)
> (sort-in-place freq-list sort-function :key sort-key)
> (dolist (pair freq-list)
> (format t "~&~A: ~A" (first pair) (second pair))))))
In general, there are a lot of words. I'd store and sort them in a
vector rather than in a list and expect better performance.
> ;---------------------------------------------------------------
> (defun print-frequencies (words letters freq-sort)
> (format t "~&-----~&Words~&-----")
> (print-frequency-counter words freq-sort)
> (format t "~&-----~&Letters~&-----")
> (print-frequency-counter letters freq-sort))
This is the only function I find a little problematic.
It's processing two (more or less) independant data.
I'd rather write:
(defun report-frequency (title data freq-sort)
(format t "~&-----~&~A~&-----" title)
(print-frequency-counter data freq-sort))
and bellow call it twice:
(report-frequency "Words" words freq-sort)
(report-frequency "Letters" letters freq-sort)
> ;---------------------------------------------------------------
> (defun read-word (infile)
> (loop for c = (read-char infile nil)
> until (or (not (characterp c))
> (not (alphanumericp c)))
> when (alphanumericp c) collect c))
>
> ;---------------------------------------------------------------
> (defun skip-whitespace (infile)
> (loop for c = (read-char infile nil)
> until (or (not (characterp c))
> (alphanumericp c))
> finally (when (characterp c)
> (unread-char c infile))))
>
> ;---------------------------------------------------------------
> (defun read-words (infile)
> (skip-whitespace infile)
> (loop for word = (read-word infile)
> until (null word)
> do (skip-whitespace infile)
> collect word))
>
> ;---------------------------------------------------------------
> (defun charlist-to-string (word)
> (coerce word 'string))
>
> ;---------------------------------------------------------------
> (defun count-word-frequency (infile words letters)
> (mapcar
> #'(lambda (word)
> (add-counter words
> (charlist-to-string word))
> (mapcar
> #'(lambda (letter)
> (add-counter letters letter))
> word))
> (read-words infile)))
(function (lambda ...)) is a pleonasm. lambda is a macro that already
expands to (function (lambda ...)).
> ;---------------------------------------------------------------
> (defun run-word-frequency (filename &optional (freq-sort))
> (let ((words (make-instance 'frequency-counter))
> (letters (make-instance 'frequency-counter)))
> (with-open-file (infile filename)
> (count-word-frequency infile words letters))
> (print-frequencies words letters freq-sort)))
>
>
> Thanks very much,
> Dmitry Jouravlev
>
--
"You question the worthiness of my code? I should kill you where you
stand!"
Hi there,
I'm learning lisp, and i have written my first program - it calculates
frequencies of words and letters in a text file.
I would appreciate if anyone wanted to comment on anything that can be
improved in it.
Okay, a few remarks...
First, comments are kind of a cool concept. ;-)
(defun foo (bar)
"Foo is a very cool function."
bar)
Same for methods and macros. Now for a class:
(defclass foo ()
((bar :documentation "fantastic, we have a slot documentation!"))
(:documentation "here we document this class"))
Also when you post some code, it might be useful to
also provide a short example one can try. Or atleast some
example how it works for you.
#|
; to check out foo do:
(foo 'bar)
|#
The comment lines to divide the files are kind of unusual.
;---------------------------------------------------------------
(defclass frequency-counter ()
((counter :initform (make-hash-table :test 'equal))))
ADD-COUNTER as a function name does not really describe what the function does.
;---------------------------------------------------------------
(defmethod add-counter ((c frequency-counter) word)
(with-slots (counter) c
(incf (gethash (string-downcase word) counter 0))))
I don't think this macro is worth it. See below.
;---------------------------------------------------------------
(defmacro sort-in-place (var &rest params)
`(setq ,var (sort ,var ,@params)))
Instead of the DOLIST in the next function you can do something like this:
(loop for (k v) in (sort freq-list sort-function :key sort-key)
do (format t "~&~A: ~A" k v))
;---------------------------------------------------------------
(defmethod print-frequency-counter ((c frequency-counter) freq-sort)
(with-slots (counter) c
(let ((freq-list ())
(sort-function (if freq-sort #'> #'string<))
(sort-key (if freq-sort #'second #'first)))
(maphash
#'(lambda (k v)
(push (list k v) freq-list))
counter)
(sort-in-place freq-list sort-function :key sort-key)
(dolist (pair freq-list)
(format t "~&~A: ~A" (first pair) (second pair))))))
"Dmitry Jouravlev" <·········@aussiemail.com.au> writes:
> Hi there,
>
> I'm learning lisp, and i have written my first program - it calculates
> frequencies of words and letters in a text file.
Excellent! Welcome!
> I would appreciate if anyone wanted to comment on anything that can be
> improved in it.
Just a couple of points:
> ;---------------------------------------------------------------
> (defclass frequency-counter ()
> ((counter :initform (make-hash-table :test 'equal))))
I really don't think that the
;------------------....
lines add much value. C-M-f lets you skip top level forms
quite easily, and M-x set-selective-display 2 will give you
a nice top level overview of all the defuns in your file.
> (defun read-word (infile)
> (loop for c = (read-char infile nil)
> until (or (not (characterp c))
> (not (alphanumericp c)))
> when (alphanumericp c) collect c))
Typically, in a function such as this, I would call the
argument `stream', instead of `infile'. This code would
work on any stream, be it file, socket, or user input.
You might consider seeing what the typical argument names used
for functions defined in the Hyperspec are; they are often good
choices to think about when defining your own functions, especially
those which act on predefined, standardized objects (e.g. streams.)
> (defun read-words (infile)
> (skip-whitespace infile)
> (loop for word = (read-word infile)
> until (null word)
> do (skip-whitespace infile)
> collect word))
This was the function which bugged me the most: you slurp the
entire file in memory in a huge long list of words before doing
anything with it. Think about how you could rewrite your code
to weave the act of computing the frequency statistics with reading
words, so you could process an arbitrarily large file in
(near) constant space.
> Thanks very much,
You're welcome, and that was a very, VERY good first attempt.
--
It would be difficult to construe Larry Wall, in article
this as a feature. <·····················@netlabs.com>
Alain Picard wrote:
> "Dmitry Jouravlev" <·········@aussiemail.com.au> writes:
>
>
>>Hi there,
>>
>>I'm learning lisp, and i have written my first program - it calculates
>>frequencies of words and letters in a text file.
>
>
> Excellent! Welcome!
>
>
>>I would appreciate if anyone wanted to comment on anything that can be
>>improved in it.
>
>
> Just a couple of points:
>
>
>>;---------------------------------------------------------------
>>(defclass frequency-counter ()
>> ((counter :initform (make-hash-table :test 'equal))))
>
>
> I really don't think that the
> ;------------------....
> lines add much value. C-M-f lets you skip top level forms
> quite easily, and M-x set-selective-display 2 will give you
> a nice top level overview of all the defuns in your file.
This is the second person to mistakenly excoriate your use of -------
Ignore them. I got great great readability out of:
; --- frequency-counter -------------------------------
(defun frequency-counter (.....)
Why? When scrolling, my eyes just needed to pick out stuff surrounded by
the hyphens. Piece of cake. Not that this makes logical sense, but it
sure seemed to work in re the psychology of perception.
--
Kenny
Why Lisp? http://wiki.alu.org/RtL_Highlight_Film
"I've wrestled with reality for 35 years, Doctor, and I'm happy to state
I finally won out over it."
Elwood P. Dowd, "Harvey", 1950
Kenny Tilton <·······@nyc.rr.com> writes:
> This is the second person to mistakenly excoriate your use of -------
>
> Ignore them. I got great great readability out of:
>
> ; --- frequency-counter -------------------------------
> (defun frequency-counter (.....)
>
> Why? When scrolling, my eyes just needed to pick out stuff surrounded
> by the hyphens. Piece of cake. Not that this makes logical sense, but
> it sure seemed to work in re the psychology of perception.
It's right. But I wouldn't put one bar per function. Just a couple
of bars per sections. For example:
;;----------------------------------------------------------------------
;; The TREE class
;;----------------------------------------------------------------------
;; This class modelize a tree, a kind of vegetation with branches and
;; leaves.
;; blah blah blah
;;
(defclass tree (plant)
...)
(defmethod grow ((self tree) ...)
...)
...
;;----------------------------------------------------------------------
;; Metabolism generic functions
;;----------------------------------------------------------------------
(defmethod eat ((animal woodivore) (plant tree))
...)
(defmethod cut ((animal woodcutter) (plant tree))
...)
...
--
"Remember, Information is not knowledge; Knowledge is not Wisdom;
Wisdom is not truth; Truth is not beauty; Beauty is not love;
Love is not music; Music is the best." -- Frank Zappa
Kenny Tilton <·······@nyc.rr.com> writes:
>
> This is the second person to mistakenly excoriate your use of -------
>
> Ignore them. I got great great readability out of:
>
> ; --- frequency-counter -------------------------------
> (defun frequency-counter (.....)
What can I say.... you're wrong? :-)
Seriously, typically, one is looking at code via some sort
of advanced editor, with colorizing support.
Something like
> ; --- frequency-counter -------------------------------
> (defun frequency-counter (.....)
is redundant, because I can SEE that it defines frequency-counter.
It's right there in the defun! The standard commenting hints, e.g.
to put 3 `;;;' at the beginning of a "header" comment, take care
of all you need:
;;; Stats Support
(defun frequency-counter
....
Means you see the entire line "Stats Support" in a different color,
so the noise induce by the ----------------- is totally spurious and
annoying.
But as this is all based on aesthetics and religion, I'm not
expecting to change your mind...
--ap
Hi,
Thanks to everyone who's replied. I've learned a lot from your
suggestions. I will post my 'improved' version sometime soon.
I have a few questions though:
Pascal,
> (function (lambda ...)) is a pleonasm. lambda is a macro
> that already expands to (function (lambda ...)).
Sorry, I don't really understand what you are suggesting...
Following Alain's suggestion i'm going to change count-word-frequency
to something along the lines of
loop for word = (read-word)
do (add-counter word)
so both read-words (with its long list of words) and mapcar/lambda
inside this function are going to disappear.
And i've renamed 'add-counter' to 'count-element'. Thanks Rainer.
Wade,
> (defun read-word (infile)
> (when (peek-char t infile nil)
> (loop for c = (read-char infile nil)
> while (and c (alphanumericp c)) collect c)))
This is very nice, but it has a problem: it stops when it encounters
non-alphanumeric words, while the desired result is to skip them:
CG-USER(3): (with-input-from-string (in " * hello")
(read-word in))
NIL
I would like it to return
(#\h #\e #\l #\l #\o)
This is of course because 'peek-char t' only skips 'whitespace'. I hit
the exact same problem originally, and had to change it to read-char /
unread-char. I entertained the thought of temporarily (inside this
function) changing the readtable to count non-alphanumeric characers as
whitespace, but thought that would be too intrusive and an overkill.
Alain,
I am using Allegro Common Lisp and not Emacs (blasphemy?) so i cannot
use the suggested shortcuts. But given what they can do, I suppose i
should learn it. Also i think ACL has some 'cooperate with emacs' mode.
Is it good / better that the built-in editor?
Also, instead of this:
(defclass frequency-counter ()
((counter :initform (make-hash-table))))
i originally wanted to do
(defclass frequency-counter (hash-table)
())
but could not inherit from 'built-in-class'. Is that limitation on
purpose? If i could do that, it would save me on the duplicated
'with-slots'.
Thanks & regards,
Dmitry Jouravlev
Dmitry Jouravlev wrote:
> Wade,
>
>>(defun read-word (infile)
>> (when (peek-char t infile nil)
>> (loop for c = (read-char infile nil)
>> while (and c (alphanumericp c)) collect c)))
>
>
> This is very nice, but it has a problem: it stops when it encounters
> non-alphanumeric words, while the desired result is to skip them:
> CG-USER(3): (with-input-from-string (in " * hello")
> (read-word in))
> NIL
> I would like it to return
> (#\h #\e #\l #\l #\o)
>
>
(defun flush-whitespace (stream)
(loop for c = (read-char stream nil)
while (and c (not (alphanumericp c)))
finally (progn
(when c (unread-char c stream))
(return c))))
(defun read-word (stream)
(when (flush-whitespace stream)
(loop for c = (read-char stream nil)
while (and c (alphanumericp c)) collect c)))
(defun read-words (stream)
(loop for word = (read-word stream)
while word collect word))
CL-USER 1 > (with-input-from-string (in " * hello")
(read-word in))
(#\h #\e #\l #\l #\o)
CL-USER 2 >
Wade
Wade Humeniuk <··················@telus.net> writes:
> (defun flush-whitespace (stream)
> (loop for c = (read-char stream nil)
> while (and c (not (alphanumericp c)))
> finally (progn
> (when c (unread-char c stream))
> (return c))))
You may also want to try (peek-char t stream).
"Dmitry Jouravlev" <·········@aussiemail.com.au> writes:
> Alain,
> I am using Allegro Common Lisp and not Emacs (blasphemy?) so i cannot
> use the suggested shortcuts. But given what they can do, I suppose i
> should learn it. Also i think ACL has some 'cooperate with emacs' mode.
When I last had a play with ACL, it didn't have its own editor,
but had an emacs package called ELI to drive ACL from emacs. I
remember thinking that it was excellent.
I'd be quite surprised if their own editor didn't also have very
sophisticated lisp editing facilities; certainly moving by sexp
and hiding are pretty basic.
> Is it good / better that the built-in editor?
I've not seen their editor. Perhaps, for lisp editing, it's as
nice as emacs; certainly, it's possible that it's better integrated
into the lisp environment. But emacs will be with you forever,
and let you far more than just editing lisp code.
I'm not going to make a recommendation on this one --- I'll let
some of the folks who know both emacs and ACL's editor well do that.
Good luck and happy lisping!
Hi,
Well, here is my 'improved' version. Thanks very much for everyone's
suggestions. I have tried to implement most of them.
One thing that surprised me was that the memory taken by the new
'count-word-frequency' was not that much different from the old one:
CG-USER(146): (time (with-open-file (stream "lost in space.txt")
(count-word-frequency stream *word-hash-table*
*letter-hash-table*)))
; cpu time (non-gc) 34,760 msec user, 172 msec system
; cpu time (gc) 10,147 msec user, 0 msec system
; cpu time (total) 44,907 msec user, 172 msec system
; real time 50,875 msec
; space allocation:
; 32,325,749 cons cells, 241,656,936 other bytes, 0 static bytes
NIL
CG-USER(149): (time (with-open-file (stream "lost in space.txt")
(count-word-frequency-old stream
*old-word-hash-table* *old-letter-hash-table*)))
; cpu time (non-gc) 32,856 msec user, 31 msec system
; cpu time (gc) 10,331 msec user, 0 msec system
; cpu time (total) 43,187 msec user, 31 msec system
; real time 46,109 msec
; space allocation:
; 32,715,354 cons cells, 243,592,632 other bytes, 0 static bytes
((1 1 1 1) (1 1) (2 1 1 1 1) (1 1) (2 1 2 1 3) (1 2 2 1 3 1 4 2) (2 1 5
2 2 2 1) (1) (3 6 2 3 2) (1 1 1 1) ...)
Regards,
Dmitry Jouravlev
;-------------------------------------------------------------
; Calculate frequency of words and letters
;-------------------------------------------------------------
;
; To run, call (run-word-frequency "filename.txt" freq-sort)
;
; if freq-sort is t, will sort the result by frequency,
; otherwise alphabetically.
;
;-------------------------------------------------------------
;-------------------------------------------------------------
(defclass frequency-counter ()
((counter :initform (make-hash-table :test 'equalp))))
;-------------------------------------------------------------
(defmethod count-element ((c frequency-counter) element)
(with-slots (counter) c
(incf (gethash element counter 0)))) ; if not exist, set to 0, and
always increment
;-------------------------------------------------------------
(defmethod frequency-counter-to-sequence ((c frequency-counter))
(with-slots (counter) c
(let ((freq-list (make-array 8 :fill-pointer 0 :adjustable t)))
(maphash
(lambda (k v)
(vector-push-extend (list k v) freq-list))
counter)
freq-list)))
;-------------------------------------------------------------
(defmethod print-frequency-counter ((c frequency-counter) freq-sort
title)
(let ((freq-list (frequency-counter-to-sequence c))
(sort-function (if freq-sort #'> #'string-lessp))
(sort-key (if freq-sort #'second #'first)))
(format t "~&-----~&~A~&-----" title)
(loop for (k v) across (sort freq-list sort-function :key sort-key)
do (format t "~&~A: ~A" k v))))
;-------------------------------------------------------------
(defun skip-whitespace (stream)
(loop for c = (read-char stream nil)
until (or (not (characterp c))
(alphanumericp c))
finally (when (characterp c)
(unread-char c stream))))
;-------------------------------------------------------------
(defun read-word (stream)
(skip-whitespace stream)
(loop for c = (read-char stream nil)
until (or (not (characterp c))
(not (alphanumericp c)))
when (alphanumericp c) collect c))
;-------------------------------------------------------------
(defun charlist-to-string (word)
(coerce word 'string))
;-------------------------------------------------------------
(defun count-word-frequency (stream words letters)
(loop for word = (read-word stream)
while word
do (progn
(count-element words
(charlist-to-string word))
(mapcar
(lambda (letter)
(count-element letters letter))
word)
(skip-whitespace stream))))
;-------------------------------------------------------------
(defun run-word-frequency (filename &optional (freq-sort))
(let ((words (make-instance 'frequency-counter))
(letters (make-instance 'frequency-counter)))
(with-open-file (stream filename)
(count-word-frequency stream words letters))
(print-frequency-counter words freq-sort "Words")
(print-frequency-counter letters freq-sort "Letters")))
Dmitry Jouravlev wrote:
> Hi,
>
> Well, here is my 'improved' version. Thanks very much for everyone's
> suggestions.
There are a couple of more observations regarding comment style.
In general (and for the benefit of Emacs and friends) top level comments
should always start with four or three semicolumns, four for major
sections and three for the rest
;;;;-------------------------------------------------------------
;;;; Calculate frequency of words and letters
;;;;-------------------------------------------------------------
;;;;
;;;; To run, call (run-word-frequency "filename.txt" freq-sort)
;;;;
;;;; if freq-sort is t, will sort the result by frequency,
;;;; otherwise alphabetically.
;;;;
;;;;-------------------------------------------------------------
;;;-------------------------------------------------------------
(defclass frequency-counter ()
((counter :initform (make-hash-table :test 'equalp))))
just nitpicking....
Cheers
--
Marco