Topic awaiting preservation: Perl/CGI - code review (Page 1 of 1)

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:

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)

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)

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.

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)

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.