Mailing List Archive: 49091 messages
  • Home
  • Script library
  • AltME Archive
  • Mailing list
  • Articles Index
  • Site search
 

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: >>&#160;f: func [var][rez: [] append rez var rez] >&#160;f 2 >&#160;;== [2] >... >You can seethis potential problem in procArtFile. >Should do a COPY: >>&#160;rez: copy [] Aha its clear - so Rebol fynction values remains outside functions?
>&#160;next next next artText&#160; ==>&#160; skip artText 3&#160; ==>&#160;
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: >>&#160;repend result [>&#160;&#160;blah >&#160;&#160;blah >&#160;&#160;blah >&#160;] >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: >>&#160;join underDir [fileName2 chapter-info] >>You should also be able to do this: >>&#160;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.