what do you think about my code

Hi,

I wrote a program for a programming competition and it would be great
if I could hear your comments on it. The idea and the code can be
found from here:

http://kamentomov.blogspot.com/2008/...mpetition.html

--
Kamen

In article <ur6gkh6wf.fsf@cybuild.com>, Kamen T <kamen@cybuild.com>
wrote:

> Hi,
>
> I wrote a program for a programming competition and it would be great
> if I could hear your comments on it. The idea and the code can be
> found from here:
>
> http://kamentomov.blogspot.com/2008/...mpetition.html

Somehow the code lacks documentation.
Checkout the various ways to add retrievable documentation
to Lisp code. Example:

(defun foo (bar)
"foo gets ... does ... returns ..." )

What are the types for the slots of the defstructs?
You have a 'node' in coordinates, but it is an integer?

Top-level closures make the code harder to debug.
I usually would avoid them. I would put all
interesting variables on the function parameter list
and make it individually testable.

There are undefined functions.

You have a wide screen. Good. ;-)

(defstruct coordinates x y node)
If you don't have any options.

Instead of (setq lst (nconc lst
you may want to use PUSH and reverse the result
in the VALUES form.

Generally the code is quite readable, but it lacks
any documentation and any hints what the variables
and slots should hold.

Kamen T <kamen@cybuild.com> writes:

> Hi,
>
> I wrote a program for a programming competition and it would be great
> if I could hear your comments on it. The idea and the code can be
> found from here:
>
> http://kamentomov.blogspot.com/2008/...mpetition.html
>
> --
> Kamen

I do not have a comment on the code, haven't looked at it yet - but I
had a look at your blog and I thought it would be better if you do not
mix English and Bulgarian on the same page. I would write an English and
a Bulgarian version of the page instead of mixing the two languages on
the same page. I think that would make it easier to read for people who
do not understand one of the two languages.

--
Dimitre Liotev
(format t "~{~a~}" (reverse '("et" "n" "in." "a" "zn" "@" "l" "d")))

Hi Rainer,

On Mon, Jan 14 2008, Rainer Joswig wrote:

> In article <ur6gkh6wf.fsf@cybuild.com>, Kamen T <kamen@cybuild.com>
> wrote:
>
>> Hi,
>>
>> I wrote a program for a programming competition and it would be
>> great if I could hear your comments on it. The idea and the code
>> can be found from here:
>>
>> http://kamentomov.blogspot.com/2008/...mpetition.html
>
> Somehow the code lacks documentation.
> Checkout the various ways to add retrievable documentation to Lisp
> code. Example:
>
> (defun foo (bar)
> "foo gets ... does ... returns ..." )
>
> What are the types for the slots of the defstructs?
> You have a 'node' in coordinates, but it is an integer?
>

The program was written under (time) pressure so you'd have to excuse
the lack of documentation.

To illustrate the time pressure better - I consider the type
declarations utterly important and I got a timeout before adding them.

> Top-level closures make the code harder to debug.
> I usually would avoid them. I would put all
> interesting variables on the function parameter list
> and make it individually testable.
>

I agree with your remark again, but it is a comparatiely small closure
and two of the four variables in question act as constants. The other
two, well, I'd consider fixing that.

> There are undefined functions.
>

All functions are defined except those defined in cl-ppcre. My package
definition is missing and that should not be as it is.

> You have a wide screen. Good. ;-)
>

:-)

> (defstruct coordinates x y node)
> If you don't have any options.
>

In fact I do have options. I didn't have time to add them.

> Instead of (setq lst (nconc lst
> you may want to use PUSH and reverse the result
> in the VALUES form.
>

Thanks, that's a good idea. That somehow slipped under my fingers.

> Generally the code is quite readable, but it lacks
> any documentation and any hints what the variables
> and slots should hold.

Thank you very much for reviewing the code and for your remarks!

--
Kamen

On Mon, Jan 14 2008, Dimitre Liotev wrote:

> Kamen T writes:
>
>> Hi,
>>
>> I wrote a program for a programming competition and it would be great
>> if I could hear your comments on it. The idea and the code can be
>> found from here:
>>
>> http://kamentomov.blogspot.com/2008/...mpetition.html
>>
>> --
>> Kamen
>
> I do not have a comment on the code, haven't looked at it yet - but
> I had a look at your blog and I thought it would be better if you do
> not mix English and Bulgarian on the same page. I would write an
> English and a Bulgarian version of the page instead of mixing the
> two languages on the same page. I think that would make it easier to
> read for people who do not understand one of the two languages.

Hi Dimitre,

Thanks for the advise. The real problem is that blogspot.com doesn't
seem to support multiple languages. So one needs to find getarounds
the lack of that feature. I don't like having to mix the two languages
either.

--
Kamen

On Jan 14, 7:29 am, Kamen T <ka...@cybuild.com> wrote:
> Hi,
>
> I wrote a program for a programming competition and it would be great
> if I could hear your comments on it. The idea and the code can be
> found from here:
>
> http://kamentomov.blogspot.com/2008/...mpetition.html
>
> --
> Kamen

Hi Kamen,

Rainer Joswig has pointed out already that your code is not
documented. I would strongly support that statement. The problem is
not even with documentation strings per se (though having them would
be good) - if you carefully select names for your functions/slots/
variables you can _most often_ guess what they're for without even
reading docstrings.

My biggest problem with your code is that your functions are too large
- literally, and there are too few of them. Your load-data function
consists of 53 LOC, with seven loops: loop inside a loop inside a
loop, loop inside a loop inside a loop, and finally another loop (or
is that loop a part of some higher level loop as well)? That's
completely unreadable, and should be very hard to maintain / debug.
Are you _absolutely_ sure that you cannot split it into smaller
subroutines with more understandable functionalities?

Good rule of thumb would be "only one function for a function". My own
experience shows me that having small functions - usually <= 10 LOC -
and writing documentation strings for each (and thus carefully
thinking about their purpose in life) significantly *improves* my
productivity.

That's just my humble opinion - YMMV.

Best Regards,
Victor.

PS. I strongly prefer iterate (http://common-lisp.net/project/
iterate/) to loop - it's more readable, more powerful and much easier
to extend, but that's a question of personal choice obviously.

PPS. öÅÌÁÀ ÕÓÐÅÈÏ× × ÐÒÏÇÒÁÍÍÉÒÏ×ÁÎÉÉ (that's Russian, but judging by
how I can understand most things written in your blog in Bulgarian,
you should get it).

In article
<3aae2cc3-7ad5-428b-8d36-b670e54dbee5@v46g2000hsv.googlegroups.com>,
vtail <victor.kryukov@gmail.com> wrote:

> On Jan 14, 7:29 am, Kamen T <ka...@cybuild.com> wrote:
> > Hi,
> >
> > I wrote a program for a programming competition and it would be great
> > if I could hear your comments on it. The idea and the code can be
> > found from here:
> >
> > http://kamentomov.blogspot.com/2008/...mpetition.html
> >
> > --
> > Kamen
>
> Hi Kamen,
>
> Rainer Joswig has pointed out already that your code is not
> documented. I would strongly support that statement. The problem is
> not even with documentation strings per se (though having them would
> be good) - if you carefully select names for your functions/slots/
> variables you can _most often_ guess what they're for without even
> reading docstrings.

There is always the option to declare the types of slots:

(defclass tree ()
((root-node :type (or nil node))))

(defclass node ()
((sub-nodes :type list :documentation "the sub-nodes bla bla"))
(:documentation "the node class bla bla"))

? (documentation 'node 'type)
"the node class bla bla"

(defstruct struct-node
(sub-nodes nil :type list))

> My biggest problem with your code is that your functions are too large
> - literally, and there are too few of them. Your load-data function
> consists of 53 LOC, with seven loops: loop inside a loop inside a
> loop, loop inside a loop inside a loop, and finally another loop (or
> is that loop a part of some higher level loop as well)? That's
> completely unreadable, and should be very hard to maintain / debug.
> Are you _absolutely_ sure that you cannot split it into smaller
> subroutines with more understandable functionalities?

I think one needs to find a balance here. Sometimes with many
small functions you get a real flood of functions. This may also
create cognitive overload (many symbols to remember).
If you have some time to carefully architect a function, you
can introduce some sub-functions and use those.
Functional abstraction is one tool for creating maintainable
code.

I have no problem reading a longer function (one or two pages),
if it is well written (structured, formatted, balanced, descriptive
names, ...).

I don't know what others are doing, but years ago we printed
the code and had those pages bound to a 'book' even.
So the code also contained page markers in front of large
functions. Then we had markers on the printed pages and
hand-written annotations. I haven't done that since many years,
especially since screens got bigger. Though I think it would
be nice to have an editor interface that would look a bit
like that: a browseable book. A system would be the book,
subsystems were chapters, files were sub-chapters,
an index, a content overview, markers into the text. A little
bit like literate programming, but without the overhead.

Are there any services which can create nice books (sort of)
from source code?

(as a side note: the book 'Building Problem Solvers'
http://www.qrg.northwestern.edu/BPS/readme.html
had a supplement book that contained just the
Common Lisp code)

What are others doing? Are you printing code and using that
as a reference?

>
> Good rule of thumb would be "only one function for a function". My own
> experience shows me that having small functions - usually <= 10 LOC -
> and writing documentation strings for each (and thus carefully
> thinking about their purpose in life) significantly *improves* my
> productivity.
>
> That's just my humble opinion - YMMV.
>
> Best Regards,
> Victor.
>
> PS. I strongly prefer iterate (http://common-lisp.net/project/
> iterate/) to loop - it's more readable, more powerful and much easier
> to extend, but that's a question of personal choice obviously.

Sometimes it is better to use an ugly standard, than to introduce
a new tool. If you are ready to spend some time with the ITERATE
macro and you need better loop syntax/functionality (even
your own iteration extension) it can be worth using it.

>
> PPS. öÅÌÁÀ ÕÓÐÅÈÏ× × ÐÒÏÇÒÁÍÍÉÒÏ×ÁÎÉÉ (that's Russian, but judging by
> how I can understand most things written in your blog in Bulgarian,
> you should get it).

>
> I don't know what others are doing, but years ago we printed
> the code and had those pages bound to a 'book' even.
> So the code also contained page markers in front of large
> functions. Then we had markers on the printed pages and
> hand-written annotations. I haven't done that since many years,
> especially since screens got bigger. Though I think it would
> be nice to have an editor interface that would look a bit
> like that: a browseable book. A system would be the book,
> subsystems were chapters, files were sub-chapters,
> an index, a content overview, markers into the text. A little
> bit like literate programming, but without the overhead.
>
> Are there any services which can create nice books (sort of)
> from source code?
>
> (as a side note: the book 'Building Problem Solvers'
> http://www.qrg.northwestern.edu/BPS/readme.html
> had a supplement book that contained just the
> Common Lisp code)
>
> What are others doing? Are you printing code and using that
> as a reference?
>

Well Donald Knuth was the first (to my knowlege) to introduce the term
literate programming
http://www-cs-faculty.stanford.edu/~knuth/lp.html

Beyond that Mathematica Notebooks can be used to make combination of
documentation and program.

--------------
John Thingstad

On Mon, Jan 14 2008, vtail wrote:

> On Jan 14, 7:29 am, Kamen T <ka...@cybuild.com> wrote:
>> Hi,
>>
>> I wrote a program for a programming competition and it would be great
>> if I could hear your comments on it. The idea and the code can be
>> found from here:
>>
>> http://kamentomov.blogspot.com/2008/...mpetition.html
>>
>> --
>> Kamen
>
> Hi Kamen,
>
Hi Victor,

> Rainer Joswig has pointed out already that your code is not
> documented. I would strongly support that statement. The problem is
> not even with documentation strings per se (though having them would
> be good) - if you carefully select names for your functions/slots/
> variables you can _most often_ guess what they're for without even
> reading docstrings.
>
Agreed.

> My biggest problem with your code is that your functions are too
> large - literally, and there are too few of them. Your load-data
> function consists of 53 LOC, with seven loops: loop inside a loop
> inside a loop, loop inside a loop inside a loop, and finally another
> loop (or is that loop a part of some higher level loop as well)?
> That's completely unreadable, and should be very hard to maintain /
> debug. Are you _absolutely_ sure that you cannot split it into
> smaller subroutines with more understandable functionalities?
>
Thanks for this remark. Generally, I agree with you - the load-data
function is too big and complex and it's hard to be maintained. The
thing is it's not supposed to be maintained. Once we pass over the
competition's deadline - it's over - the code is useless. That's
because of its highly specific nature.

The real question is - would the code have been easier to write if it
consisted of short and more readable functions? Considering the fact
that I could have used "trace" to debug it, the answer is YES.

And yet CL provides me with the advantage to cleanly define and use
variables within a very small scope. So although a function is long it
is still readable because it is build from independent blocks of code.

Anyway, I currently see one portion that I should have taken out into
a separate function and that's only at first look.

> Good rule of thumb would be "only one function for a function". My
> own experience shows me that having small functions - usually <= 10
> LOC - and writing documentation strings for each (and thus carefully
> thinking about their purpose in life) significantly *improves* my
> productivity.
>
> That's just my humble opinion - YMMV.
>

Thank you very much for your remarks!

> Best Regards,
> Victor.
>
> PS. I strongly prefer iterate (http://common-lisp.net/project/
> iterate/) to loop - it's more readable, more powerful and much
> easier to extend, but that's a question of personal choice
> obviously.
>
In my case it's a question of not having enough time to learn it. If
Peter Siebel had written about iterate in PCL (instead about loop), I
would have probably used it instead of loop.

> PPS. Æåëàþ óñïåõîâ â ïðîãðàììèðîâàíèè (that's Russian, but judging by
> how I can understand most things written in your blog in Bulgarian,
> you should get it).

Áîëüøîå ñïàñèáî äëÿ òâîé ìåéë è ïîæåëàíèÿ. ß ïîíèìàþ òåáå íå òîëüêî
ïîòîìó÷òî íàøè ÿçûêè ïîõîæå îäèí äðóãî, íî ïîòîìó÷òî ÿ ó÷èëàñü íà
Ðóñêèé ÿçûê â øêîëå, íî åòî áûëî äàâíî òîìó íàçàä :-)

Best regards,

--
Kamen

On Jan 14, 7:29 am, Kamen T <ka...@cybuild.com> wrote:
> Hi,
>
> I wrote a program for a programming competition and it would be great
> if I could hear your comments on it. The idea and the code can be
> found from here:
>
> http://kamentomov.blogspot.com/2008/...mpetition.html
>
> --
> Kamen

I've started looking into the problem, and it sounds interesting. Can
you please comment what's the purpose of the message length / maximum
message size? Does it put constraint on how students can exchange
messages?

Also, you seem to be impressed with CL-PPCRE, which is a very good
library indeed. However, sometimes you can achieve the same with less
effort. E.g. instead of

(defun read-two-numbers (file)
(multiple-value-bind (sss arr)
(scan-to-strings
'(:sequence
(:register (:greedy-repetition 1 nil :digit-class))
(:greedy-repetition 0 nil :whitespace-char-class)
(:register (:greedy-repetition 1 nil :digit-class)))
(read-line file nil))
(declare (ignore sss))
(values (parse-integer (aref arr 0))
(parse-integer (aref arr 1)))))

you could just say:

(defun read-two-numbers (stream)
"Read two numbers from the input stream and return them as values"
(values (read stream) (read stream)))

(defun test-read-two-numbers ()
(with-input-from-string (s "12 13")
(read-two-numbers s)))

BASSCOM> (test-read-two-numbers)
12
13

Even if for some reasons you forgot (or didn't know) about 'read', you
may notice that (:register (:greedy-repetition 1 nil :digit-class))
etc. is the frequent pattern in your program. Usually something that
is repeated over and over again is a good sign for refactoring.

Also, (macroexpand `,(* 2 (expt 10 (* 7 2)))) looks very...
intriguing. What were you trying to say? (* 2 (expt 10 14)) provides
the same answer. Also, if this constant is something meaningful for
your algorithm, by no means define a symbolic constant for it.

More comments to follow once I understand the problem better.

Victor.