Closed Thread Icon

Topic awaiting preservation: Perl/CGI - code review (Page 1 of 1) Pages that link to <a href="https://ozoneasylum.com/backlink?for=24473" title="Pages that link to Topic awaiting preservation: Perl/CGI - code review (Page 1 of 1)" rel="nofollow" >Topic awaiting preservation: Perl/CGI - code review <span class="small">(Page 1 of 1)</span>\

 
silence
Maniac (V) Inmate

From: Melbourne, Australia
Insane since: Jan 2001

posted posted 12-22-2004 17:33

I'm writing a tutorial for Guru's on Perl/CGI and before I got into the actual tutorial writing, I was wondering if I could get a review of the code.

The purpose is simple:

  1. Display news items stored in flat file database (sort of like a blog)
  2. Sort news items by date and display a paged view
  3. Add news items



The basic algorithm for displaying news is:

  • read news items from database and store them in an associative array using the timestamp as the key and an array reference to the array holding the information
  • print them out in a paged view depending on $newsitemsperpage value in config file, including navigation



The basic algorithm for modifying news is:

  • check if there are any parameters passed into the script

    • if there are parameters passed in, assume that this is a new item to be added
    • read news items from database and store them in an associative array using the timestamp as the key and an array reference to the array holding the information
    • create a new timestamp for the news item, making sure that this isn't a duplicate (if so, create a new timestamp)
    • add news item to database



    • if there are no paramaters passed into the script, display form to add news item



The database is a flat text file in the following format:

code:
1099666755
test 14
test test test test test test
test test test testtest test test test
test test test test
test test test testtest test test
test test test test
test test test testtest test test test
test test test testtest test test test
test test test test
---- NEWSBLOCK ----



The first line of each block is the time stamp. This is UTC. The second line is the subject and all additional lines are content. Each block is separated by "---- NEWSBLOCK ----" on the last line.

There are 3 documents:

  • displaynews.pl - displays news items
  • modifynews.pl - add news item (modify functionality may be added later
  • config.pl - global variables and some subroutines



What I'm looking for is c+c regarding the way things are coded, other simpler/better ways of doing certain things, algorithmic improvements, etc.

Here are the pages:
http://members.optusnet.com.au/aanesi/ozone/displaynews.pl
http://members.optusnet.com.au/aanesi/ozone/modifynews.pl
http://members.optusnet.com.au/aanesi/ozone/config.pl

These versions don't work since my host doesn't provide server side scripting, but I can post a working version if that will help.



(Edited by silence on 12-22-2004 17:34)

(Edited by silence on 12-22-2004 17:37)

hyperbole
Paranoid (IV) Inmate

From: Madison, Indiana, USA
Insane since: Aug 2000

posted posted 12-22-2004 23:21

Hi silence,

The first thing that strikes me about the system you have created is that you are reading all the news items into the perl program before processing them. (I mainly looked at the modify script.)

This is going to cause a couple of problems. First as the number of news items gets large, you will be using a lot of cpu and memory resources to process the entire file each time you want to add a new news item. Wouldn't it be better to read the file one line at a time and only store the lines you are interested in?

Second, while you have the entire news file in memory and are processing it, there is nothing to stop another instance of the modify script from adding a different news item with the same time stamp to the file. I would suggest that you open the database file at the start of the script and use flock to lock the file. Then unlock the file after you have completed processing the new new item.

Here is a pretty good article about using flock to ensure that only one process can access a resource at a time.




.

-- not necessarily stoned... just beautiful.


(Edited by hyperbole on 12-22-2004 23:24)

silence
Maniac (V) Inmate

From: Melbourne, Australia
Insane since: Jan 2001

posted posted 12-23-2004 00:10

Hey, Hyperbole, thanks for the review.

quote:
The first thing that strikes me about the system you have created is that you are reading all the news items into the perl program before processing them.



You're right. Now that I look at it, I can parse the database file and only store the timestamps since that's the only part I need. Then I can just append the new news item. Cheers.

quote:
I would suggest that you open the database file at the start of the script and use flock to lock the file.



I had debated using flock before starting, but I didn't want to get into it this time around. I will include it in the tutorial for completeness since it is an important issue, but I had to cut out that, a lot of other stuff, to keep it relatively simple.

I'm still kicking myself for using array references. Now I have to explain that to people.

Thanks again.

bitdamaged
Maniac (V) Mad Scientist

From: 100101010011 <-- right about here
Insane since: Mar 2000

posted posted 12-23-2004 03:04

Let's see.

First I am in the habit of always using the "strict" pragma. It's just a bit safer.

on display

if ($line =~ /\d\d\d\d\d\d\d\d\d\d/)

is much easeir to do like so

if ($line =~ /$\d{10}) $ makes it start of line then \d ten times



.:[ Never resist a perfect moment ]:.

(Edited by bitdamaged on 12-23-2004 03:05)

hyperbole
Paranoid (IV) Inmate

From: Madison, Indiana, USA
Insane since: Aug 2000

posted posted 12-23-2004 20:23

bit,

Good sugesstion about use strict. I missed that even though I always use it myself.

I'm sure you mean /^\d{10}/.

^ means start of line
$ means end of line



.

-- not necessarily stoned... just beautiful.

« BackwardsOnwards »

Show Forum Drop Down Menu