comments for beginner ugly code ;-)
[1/9] from: sags::apollo::lv at: 13-Mar-2005 23:23
Hi, Rebolers!
I tried to make a simple web site creation script (attached) that
makes web site from set of templates and simple article files and
chapter structure based on directory structure.
It would be interesting for me to get some opinions of yours
about coding style and security of my approach.
brgds
Janeks
-- Attached file included as plaintext by Ecartis --
-- Desc: Text from file 'mySite.r'
REBOL []
;index page template
index-page: %/c/www/sitex/index.html
;banner page template
banner-page: %/c/www/sitex/banner.html
;menu page template
menu-page: %/c/www/sitex/menu.html
;content template
content-page: %/c/www/sitex/content.html
;chapters directory
chapters-dir: %/c/www/sitex/
;chapter info file
chapter-info: %dirinfo.txt
;extension for article file
article-extension: %.art
;Mesage for empty directories
no-articles-msg: "<br>Under construction"
;script name
script-name: to-string second split-path system/options/script
;cgi functions
do %cgiFuncs.r
html: ""
cgi-vars: []
emit: func [data] [repend html data]
;frame/fields selector
selFrame: func [ cgi-vars ] [
; look for page defs
switch/default select cgi-vars "page" [
"banner" [ makeBanner banner-page ]
"menu" [ makeMenu menu-page]
"content" [ makeContent content-page]
][
emit "Error: no such page type! <br> Probably your index template file with frames definitions
are not correct!"
]
]
makeBanner: func [ templateFile ] [
if error? try [
emit read templateFile
][
emit "Error: could not find template file!"
]
]
makeMenu: func [ templateFile ] [
if error? try [
templ: read templateFile
][
emit "Error: could not find template file!"
quit
]
replace templ "$head" "head content"
replace templ "$title" "Menu"
either select cgi-vars "subChap" [
pathElems: parse select cgi-vars "subChap" "/"
pathStr: rejoin [ {<a href="} script-name {?page=menu">Home</a>} ]
foreach pathEl copy/part pathElems back tail pathElems [
repend pathStr rejoin [ { / <a target="menu" href="} script-name "?page=menu&subChap="
pathEl {">} pathEl "</a>" ]
]
repend pathStr rejoin [ { / } last pathElems ]
replace templ "$path" pathStr
][
replace templ "$path" ""
]
replace templ "$menulist" getMenuLinks chapters-dir
emit templ
emit mold to-string second split-path system/options/script
; emit get-modes chapters-dir 'creation-date
]
makeContent: func [ templateFile ] [
if error? try [
templ: read templateFile
][
emit "Error: could not find template file!"
]
; select cgi-vars "myLink"
; to-file rejoin [ chapters-dir select cgi-vars "myLink"]
; first read/lines to-file join form underDir fileName
if select cgi-vars "myLink" [
artFile: to-file rejoin [ chapters-dir select cgi-vars "myLink"]
artSerie: procArtFile artFile
replace templ "$heading" artSerie/1
replace templ "$content" artSerie/2
emit templ
]
; replace templ "$title" "Menu"
]
procArtFile: func [ artFile ] [
artFile: to-file rejoin [ chapters-dir select cgi-vars "myLink"]
artText: read/lines artFile
artHead: third artText
artText: copy/part next next next artText tail artText
contText: "article"
; processing part of article text
; here could be usable also makeDoc.r
foreach artline artText [
either 0 < length? artline [
repend contText rejoin [ artLine " " ]
][
repend contText <br>
]
]
rez: []
append rez artHead
append rez contText
return rez
]
getMenuLinks: func [ fullDir ] [
result: "<ul>"
either select cgi-vars "subChap" [
aDirectory: to-file rejoin [ fullDir select cgi-vars "subChap" "/" ]
; repend result select cgi-vars "subChap"
][
aDirectory: fullDir
; repend result "Home"
]
numFiles: 0
foreach fileName read aDirectory [
either #"/" = last fileName [
repend result rejoin [ {<li><a target="menu" href="} script-name "?page=menu&subChap="
]
if select cgi-vars "subChap" [ repend result rejoin [ select cgi-vars "subChap" "/" ]
]
repend result replace copy fileName "/" ""
repend result {">}
if error? try [
repend result read to-file join form aDirectory join fileName form chapter-info
][
rejoin [ result replace copy fileName "/" "" ]
]
repend result "</a>"
underDir: to-file join form aDirectory fileName
repend result "<ul>"
foreach fileName2 read underDir [
either #"/" = last fileName2 [
repend result rejoin [ {<li><a target="menu" href="} script-name "?page=menu&subChap="
]
repend result fileName
repend result rejoin [ replace copy fileName2 "/" "" {">} ]
either exists? to-file join form underDir join fileName2 form chapter-info [
repend result read to-file join form underDir join fileName2 form chapter-info
][
repend result replace fileName2 "/" ""
]
repend result "</a>"
][
if article-extension = suffix? fileName2 [
repend result rejoin [ {<li><a target="content" href="} script-name "?page=content&mylink="
fileName ]
if select cgi-vars "subChap" [ repend result rejoin [ select cgi-vars "myLink" "/" ]
]
repend result fileName2
repend result {">}
repend result first read/lines to-file join form underDir fileName2
repend result "</a>"
]
]
]
repend result "</ul>"
][
if article-extension = suffix? fileName [
repend result rejoin [ {<li><a target="content" href="} script-name "?page=content&mylink="
]
if select cgi-vars "subChap" [ repend result rejoin [ select cgi-vars "subChap" "/" ]
]
repend result rejoin [ fileName {">} ]
repend result first read/lines to-file join form aDirectory fileName
repend result "</a>"
numFiles: numFiles + 1
]
]
]
repend result "</ul>"
if numFiles = 0 [
repend result no-articles-msg
]
return result
]
mystr: input-cgi
either mystr [
foreach [var value] decode-cgi mystr [
insert cgi-vars form value
insert cgi-vars form var
]
selFrame cgi-vars
][
html: read index-page
]
print html
[2/9] from: antonr::lexicon::net at: 15-Mar-2005 0:33
Hi Janeks,
It doesn't look so much like beginner code. :)
But I have one thing to point out. Look at this
console session:
f: func [var][rez: [] append rez var rez]
f 2
;== [2]
f 3
;== [2 3]
f 7
;== [2 3 7]
print mold :f
;func [var][rez: [2 3 7] append rez var rez]
You can see this potential problem in procArtFile.
Should do a COPY:
rez: copy []
And while we are looking at procArtFile, here are some
optimizations:
next next next artText ==> skip artText 3 ==> at artText 4
repend contText either 0 < length? artline [rejoin [artLine " "]][<br>]
There are lots of places in the code where you can optimize
in the above way. All those REPEND RESULT are repetitive and it's
usually more convenient to put everything to be repended into a
block and repend that just once, eg:
repend result [
blah
blah
blah
]
In getMenuLinks in notice you are converting to a string (using FORM),
then back to a file (using to-file):
to-file join form underDir join fileName2 form chapter-info
JOIN will keep the type of its first argument, so as underDir is
already a file!, there is no need to convert it backwards and forwards.
This should be just as good:
join underDir [fileName2 chapter-info]
You should also be able to do this:
underDir/:fileName2/:chapter-info
but we found a bug with that with certain filenames (like 1.s3m)
recently so I recommend the first way for stability at the moment.
(pity, as the second way handles "/" so much better.)
Sorry I haven't commented on the overriding aspects of your code
(security etc).
Regards,
Anton.
[3/9] from: gabriele::colellachiara::com at: 14-Mar-2005 14:49
Hi Anton,
On Monday, March 14, 2005, 2:33:20 PM, you wrote:
AR> but we found a bug with that with certain filenames (like 1.s3m)
AR> recently so I recommend the first way for stability at the moment.
AR> (pity, as the second way handles "/" so much better.)
To clarify, that's not a bug. (And, it doesn't apply to the above
case either.)
>> somedir: %/path/to/somedir/
== %/path/to/somedir/
>> f: %1.s3m
== %1.s3m
>> somedir/:f
== %/path/to/somedir/1.s3m
About your ticket, you can write:
>> somedir/%1.s3m
== %/path/to/somedir/1.s3m
but of course not:
>> somedir/1.s3m
** Syntax Error: Invalid decimal -- 1.s3m
** Near: (line 1) somedir/1.s3m
for the same reason that you can't write:
>> [somedir 1.s3m]
** Syntax Error: Invalid decimal -- 1.s3m
** Near: (line 1) [somedir 1.s3m]
and so you would write:
>> join somedir %1.s3m
== %/path/to/somedir/1.s3m
and NOT:
>> join somedir 1.s3m
** Syntax Error: Invalid decimal -- 1.s3m
** Near: (line 1) join somedir 1.s3m
Regards,
Gabriele.
--
Gabriele Santilli <[g--santilli--tiscalinet--it]> -- REBOL Programmer
Amiga Group Italia sez. L'Aquila --- SOON: http://www.rebol.it/
[4/9] from: sags:apollo:lv at: 15-Mar-2005 11:18
Thanks Anton! On 15 Mar 2005 at 0:33, Anton Rolls wrote: >It doesn't look so
much like beginner code. :) I started to code from end of eighties, but
neverdid it as proffesional - no diploma either. Thinking about it. >But I
have one thing to point out. Look at this >console session: >> f: func
[var][rez: [] append rez var rez] > f 2 > ;== [2] >... >You can
seethis potential problem in procArtFile. >Should do a COPY: >> rez:
copy [] Aha its clear - so Rebol fynction values remains outside functions?
> next next next artText  ==>  skip artText 3  ==> 
at artText 4 Actualy this was a bit my lazines >There are lots of places in
the code where you can optimize >in the above way. All those REPEND RESULT
are repetitive and it's >usually more convenient to put everything to be
repended into a block >and repend that just once, eg: >> repend result
[>  blah >  blah >  blah > ] >It is o'k.
Now my code is more viewable. And then I can't use "if" because it returns
none, but of course I use either, that in else case returns "". >JOIN will
keep the type of its first argument, so as underDir is >already a file!,
there is no need to convert it backwards and >forwards. This should be just
as good: >> join underDir [fileName2 chapter-info] >>You should also be
able to do this: >> underDir/:fileName2/:chapter-info Very shortly and
fine. >Sorry I haven't commented on the overriding aspects of your code
[5/9] from: sags::apollo::lv at: 17-Mar-2005 10:15
What if I want to place under repend result [ ]
following statement:
if error? try [
repend result read to-file join aDirectory join fileName form chapter-
info
][
repend result copy/part fileName back tail fileName
]
is it possible only with:
either error? try [
read to-file join aDirectory join fileName form chapter-info
][
copy/part fileName back tail fileName
][
read to-file join aDirectory join fileName form chapter-info
]
?
Janeks
On 15 Mar 2005 at 0:33, Anton Rolls wrote:
....
[6/9] from: antonr:lexicon at: 17-Mar-2005 22:18
Hmm.. If you needed to do that several times within the
repend block, then I would make a nice function to help.
DEFAULT was somebody else's idea, I think it went
something like this:
default: func [
code [block!] fault [block!]
/local err
][
either error? set/any 'err try code fault [err]
]
Examples:
default [join "apples" "pears"]["no good"]
;== "applespears"
default [join "apples" "pears" / 0]["no good"]
;== "no good"
repend result [
default [
read aDirectory/:fileName/:chapter-info
][
join "Couldn't read: " copy/part fileName back tail fileName
]
...
]
Anton.
[7/9] from: sags::apollo::lv at: 18-Mar-2005 8:59
O'k!
I am unshure how in this case the works 'err!
Is the refinement /local err is for generating error messages?
brgds
Janeks
On 17 Mar 2005 at 22:18, Anton Rolls wrote:
[8/9] from: lmecir:mbox:vol:cz at: 18-Mar-2005 9:31
[sags--apollo--lv] napsal(a):
>O'k!
>I am unshure how in this case the works 'err!
>Is the refinement /local err is for generating error messages?
>
>brgds
>Janeks
>
/local is a refinement signaling that the variables following it are
local variables. That means, that 'err is a local variable used to
store
intermediate result.
Below is a more "polished" Default version. The Fault block may use
Error variable "knowing" the error value.
default: func [
{Execute code. If error occurs, execute fault.}
[throw]
code [block!] {Code to execute}
fault [block!] {Error handling code}
] [
either error? set/any 'code try code [
fault: make function! [[throw] error [error!]] fault
fault code
] [get/any 'code]
]
Examples:
>> default [join "apples" "pears"]["no good"]
== "applespears"
>> default [join "apples" "pears" / 0]["no good"]
== "no good"
>> default [join "apples" "pears" / 0][print mold disarm error]
make object! [
code: 312
type: 'script
id: 'cannot-use
arg1: 'divide
arg2: 'string!
arg3: none
near: [join "apples" "pears" / 0]
where: 'default
]
-Ladislav
[9/9] from: antonr::lexicon::net at: 18-Mar-2005 23:43
Ah, thanks Ladislav. I really like your DEFAULT, and I
wish it would be added to Rebol. I even like the name (default),
but I can see it might be seen as a problem (colliding with
other uses of the word), but I think that should not hold it
from being integrated. It could be named "may-fail", "any-fault"
on-fail
or something like that.
Anton.