Delete a line out of a flat file database

[Mark Constant]

I have pre-made code that deletes a line from a flat file database. I
am trying to pass an argument through a link to give the name of the
line to delete.

Here is my database.txt
image020.jpg | 6/4/2004 | 10:19:59 | | rena

Here is how I use the link
delete.cgi?filedelete=$filename

Now here is what prints out
image020.jpg
File image020.jpg not found!

It is obvious that it is grabbing the argument because it prints out
image020.jpg. The thing I don't understand is why it is not matching
with the image020.jpg in the database.txt.

Here is my code
#!c:/perl/bin/perl

use strict;
use warnings;
use CGI;
use CGI::Carp qw(fatalsToBrowser);
my $q = new CGI;
print $q->header, $q->start_html('Delete File');

my $action = new CGI;
my @temp=split(/=/,$ENV{'QUERY_STRING'});
my $filedelete = $temp[1];
my $file = 'C:/Program Files/Apache
Group/Apache2/htdocs/quickbooks/database.txt';
my $found = undef;
my (@ary);
my $line;

open( OLD, $file ) or die "Couldn't open old file $file: $! \n";
print $temp[1];
foreach $line (@ary) {
chomp($line);
(my $filename, my $filedate, my $filetime, my $notes, my $user) =
split(/\|/,$line);
push( @ary, $line ) unless ( $filedelete eq $filename );
$found = 1 if ( $filedelete eq $filename );
}

close OLD or die "Couldn't close old file $file: $! \n";

unless ($found) {
print $action->h2("File $filedelete not found!");
exit;
}

open( NEW, ">$file" ) or die "Couldn't open new file $file: $!\n";
foreach (@ary) {
print NEW $_, "\n"
}

close NEW or "Couldn't close new file $file: $! \n";;

exit;

print $q->end_html;

 

[Paul Lalli]

Here is my database.txt
image020.jpg | 6/4/2004 | 10:19:59 | | rena

Now here is what prints out
image020.jpg
File image020.jpg not found!

It is obvious that it is grabbing the argument because it prints out
image020.jpg. The thing I don't understand is why it is not matching
with the image020.jpg in the database.txt.

(my $filename, my $filedate, my $filetime, my $notes, my $user) =
split(/\|/,$line);
push( @ary, $line ) unless ( $filedelete eq $filename );
$found = 1 if ( $filedelete eq $filename );

You're splitting on the | only. But look at your database lines. The
elements are separated by " | ", not by "|". So the contents of $filename
are 'images020.jpg ', which is not string equal to 'image20.jpg'

I would suggest changing your split line to
split /\s+\|\s+/, $line;

(untested on my part, btw).

Paul Lalli

 

[Ben Morrow]

Quoth (e-mail address removed) (Mark Constant):

I have pre-made code that deletes a line from a flat file database. I
am trying to pass an argument through a link to give the name of the
line to delete.

Here is my database.txt
image020.jpg | 6/4/2004 | 10:19:59 | | rena

Here is how I use the link
delete.cgi?filedelete=$filename

Now here is what prints out
image020.jpg
File image020.jpg not found!

It is obvious that it is grabbing the argument because it prints out
image020.jpg. The thing I don't understand is why it is not matching
with the image020.jpg in the database.txt.

Here is my code
#!c:/perl/bin/perl

use strict;
use warnings;
use CGI;
use CGI::Carp qw(fatalsToBrowser);
my $q = new CGI;
print $q->header, $q->start_html('Delete File');

my $action = new CGI;
my @temp=split(/=/,$ENV{'QUERY_STRING'});

DON'T EVER DO THIS AGAIN. Use CGI.

my $filedelete = $temp[1];

my $filedelete = $action->param('filedelete');

my $file = 'C:/Program Files/Apache
Group/Apache2/htdocs/quickbooks/database.txt';
my $found = undef;

my $found;

my (@ary);

my @ary;

my $line;

open( OLD, $file ) or die "Couldn't open old file $file: $! \n";

Don't put "\n" on the end of die messages.
Use lexical filehandles.
Specify the mode to open the file in, for safety.

open my $OLD, '<', $file
or die "can't open $file: $!";

print $temp[1];
foreach $line (@ary) {

for my $line (@ary) {

Create variables in the smallest possible scope.

chomp($line);
(my $filename, my $filedate, my $filetime, my $notes, my $user) =

my ($filename, $filedate, $filetime, $notes, $user) =

But anyway, why do you need more than

my ($filename) =

?

split(/\|/,$line);

You are splitting on /\|/. This will leave whitespace all round your
filename. Try

.... = split /\s* \| \s*/x, $line;

push( @ary, $line ) unless ( $filedelete eq $filename );
$found = 1 if ( $filedelete eq $filename );

None of those parens are necessary.

}

close OLD or die "Couldn't close old file $file: $! \n";

unless ($found) {
print $action->h2("File $filedelete not found!");
exit;

No. This will not correctly end the HTML.

}

open( NEW, ">$file" ) or die "Couldn't open new file $file: $!\n";
foreach (@ary) {
print NEW $_, "\n"
}

close NEW or "Couldn't close new file $file: $! \n";;

exit;

Nor will this.

if ($found) {
open my $NEW, '>', $file or die "couldn't create $file: $!";
print $NEW "$_\n" for @ary;
}
else {
print $action->h2("File $filedelete not found!");
}

print $q->end_html;

You *must* use locking in a situation like this, or you will end up with
a corrupted database. Something like:

use Fcntl qw/:flock/;

{
open my $OLD, '<', ...;
flock $OLD, LOCK_SH or die "can't acquire read lock: $!";
# read stuff
}
# $OLD closes here, lock is released

if ($found) {
open my $NEW, '>', ...;
flock $NEW, LOCK_EX or die "can't acquire write lock: $!";
# write stuff
}
# ditto for $NEW

Alternatively, you could open the file '+<' and then lock it LOCK_EX,
and truncate it rather than closing/reopening.

Ben

 

[Brian McCauley]

Subject: Delete a line out of a flat file database

See FAQ: "How do I [ ... ]delete a line in a file [...]?"

I have pre-made code that deletes a line from a flat file database.

What do you mean by "pre-made"? You mean someone else gave you this
code. Give it back to them.

Here is my code
#!c:/perl/bin/perl

use strict;
use warnings;
use CGI;
use CGI::Carp qw(fatalsToBrowser);

Good start.

my $q = new CGI;
print $q->header, $q->start_html('Delete File');

my $action = new CGI;

What? Two CGI objects? Why?

my @temp=split(/=/,$ENV{'QUERY_STRING'});

If you are using CGI why access $ENV{'QUERY_STRING'} directly?

my $filedelete = $temp[1];

Ever heard of a list slice? There's no need for @temp.

my $file = 'C:/Program Files/Apache
Group/Apache2/htdocs/quickbooks/database.txt';
my $found = undef;

No need to explicitly initialise to undef.

my (@ary);
my $line;

You are suffering from premature declaration. Variables should
usually be declared where they are first used.

open( OLD, $file ) or die "Couldn't open old file $file: $! \n";
print $temp[1];
foreach $line (@ary) {

Here is where you should have declared $line.

@ary is always empty here so this loop will never do anything.

I think you meant to say:

chomp($line);
(my $filename, my $filedate, my $filetime, my $notes, my $user) =
split(/\|/,$line);

my can take a list of aguments so the above is more simply written:

my ($filename, $filedate, $filetime, $notes, $user) =
split(/\|/,$line);

push( @ary, $line ) unless ( $filedelete eq $filename );
$found = 1 if ( $filedelete eq $filename );

Having and if and an unless with the same constion makes me think
you'd be better off with a if-else.

}

close OLD or die "Couldn't close old file $file: $! \n";

unless ($found) {
print $action->h2("File $filedelete not found!");
exit;

You forgot the end_html.

}

open( NEW, ">$file" ) or die "Couldn't open new file $file: $!\n";
foreach (@ary) {
print NEW $_, "\n"
}

You have not considered that the server may be handling two requests
at the same time. You need some kind of file locking.

close NEW or "Couldn't close new file $file: $! \n";;

You forgot the die;

exit;

This is spurious.

print $q->end_html;

--
\\ ( )
. _\\__[oo
.__/ \\ /\@
. l___\\
# ll l\\
###LL LL\\

 

[Kevin Collins]

Quoth (e-mail address removed) (Mark Constant):

I have pre-made code that deletes a line from a flat file database. I
am trying to pass an argument through a link to give the name of the
line to delete.

Here is my database.txt
image020.jpg | 6/4/2004 | 10:19:59 | | rena

Here is how I use the link
delete.cgi?filedelete=$filename

Now here is what prints out
image020.jpg
File image020.jpg not found!

It is obvious that it is grabbing the argument because it prints out
image020.jpg. The thing I don't understand is why it is not matching
with the image020.jpg in the database.txt.

Here is my code
#!c:/perl/bin/perl

use strict;
use warnings;
use CGI;
use CGI::Carp qw(fatalsToBrowser);
my $q = new CGI;
print $q->header, $q->start_html('Delete File');

my $action = new CGI;
my @temp=split(/=/,$ENV{'QUERY_STRING'});

DON'T EVER DO THIS AGAIN. Use CGI.

my $filedelete = $temp[1];

my $filedelete = $action->param('filedelete');

my $file = 'C:/Program Files/Apache
Group/Apache2/htdocs/quickbooks/database.txt';
my $found = undef;

my $found;

my (@ary);

my @ary;

my $line;

open( OLD, $file ) or die "Couldn't open old file $file: $! \n";

Don't put "\n" on the end of die messages.

Why not? It isn't needed and I never do, but there isn't any reason not to.
Better would be to explain why it not needed than just "don't do this". In
fact, from my 5.8.0's 'perldoc -f die':

Equivalent examples:

die "Can't cd to spool: $!\n" unless chdir '/usr/spool/news';
chdir '/usr/spool/news' or die "Can't cd to spool: $!\n"

Use lexical filehandles.
Specify the mode to open the file in, for safety.

In this case, I don't think it is necessary. The file name is known and
"normal". For dealing with unkowns, I would agree. According to the faq for
open:

Use 3-argument form to open a file with arbitrary weird
characters in it,

open(FOO, '<', $file);

open my $OLD, '<', $file
or die "can't open $file: $!";

print $temp[1];
foreach $line (@ary) {

for my $line (@ary) {

Create variables in the smallest possible scope.

chomp($line);
(my $filename, my $filedate, my $filetime, my $notes, my $user) =

my ($filename, $filedate, $filetime, $notes, $user) =

But anyway, why do you need more than

my ($filename) =

?

split(/\|/,$line);

You are splitting on /\|/. This will leave whitespace all round your
filename. Try

... = split /\s* \| \s*/x, $line;

push( @ary, $line ) unless ( $filedelete eq $filename );
$found = 1 if ( $filedelete eq $filename );

None of those parens are necessary.

But they do add clarity and help make precedence more obvious.

 

[Paul Lalli]

Why not? It isn't needed and I never do, but there isn't any reason not to.
Better would be to explain why it not needed than just "don't do this". In
fact, from my 5.8.0's 'perldoc -f die':

Equivalent examples:

die "Can't cd to spool: $!\n" unless chdir '/usr/spool/news';
chdir '/usr/spool/news' or die "Can't cd to spool: $!\n"

Because putting "\n" suppresses useful information. Without a "\n", the
die function will output not only its argument string, but also the file
and line number on which the die occurred. Adding the "\n" in there makes
you lose this information.

But they do add clarity

That's debatable.

and help make precedence more obvious.

"Learn the language".


Paul Lalli

 

[Jim Gibson]

I have pre-made code that deletes a line from a flat file database. I
am trying to pass an argument through a link to give the name of the
line to delete.

Here is my database.txt
image020.jpg | 6/4/2004 | 10:19:59 | | rena

Here is how I use the link
delete.cgi?filedelete=$filename

Now here is what prints out
image020.jpg
File image020.jpg not found!

It is obvious that it is grabbing the argument because it prints out
image020.jpg. The thing I don't understand is why it is not matching
with the image020.jpg in the database.txt.

Here is my code
#!c:/perl/bin/perl

[variable initialzation snipped]

open( OLD, $file ) or die "Couldn't open old file $file: $! \n";
print $temp[1];

foreach $line (@ary) {
chomp($line);
(my $filename, my $filedate, my $filetime, my $notes, my $user) =
split(/\|/,$line);
push( @ary, $line ) unless ( $filedelete eq $filename );
$found = 1 if ( $filedelete eq $filename );
}

close OLD or die "Couldn't close old file $file: $! \n";

[rest of program snipped]

 

[Kevin Collins]

Because putting "\n" suppresses useful information. Without a "\n", the
die function will output not only its argument string, but also the file
and line number on which the die occurred. Adding the "\n" in there makes
you lose this information.



That's debatable.

If you use parens, there can be no ambiguity...

"Learn the language".

Well, then why bother with indenting, white-space, comments, etc? If you "know
the language", why use any of those constructs - they are mostly not required?

Kevin

 

[Uri Guttman]

KC> If you use parens, there can be no ambiguity...

KC> Well, then why bother with indenting, white-space, comments, etc?
KC> If you "know the language", why use any of those constructs - they
KC> are mostly not required?

then you might as well put parens around everything and become lisp.

parens are best used when needed and they can hide (or make noisy) stuff
when they are not. i still do push( @foo, $bar ) for style reasons and
not from any ambiguity thing. the code (and reader) should be expected
to know basic perl precedence rules. baby code is ok when you start out
but you should graduate to cleaner code where you assume the reader has
enough skill to follow basic perl.

uri

 

[Joe Smith]

Because putting "\n" suppresses useful information. Without a "\n", the
die function will output not only its argument string, but also the file
and line number on which the die occurred. Adding the "\n" in there makes
you lose this information.

That additional information is completely useless to the end user.
1) Leave out the trailing \n for messages that tell the programmer
there is a bug in the program that needs to be fixed.
2) Put in the \n for messages that tell the user that he/she entered
something wrong and should try again.

-Joe

 

[Joe Smith]

That additional information is completely useless to the end user.
1) Leave out the trailing \n for messages that tell the programmer
there is a bug in the program that needs to be fixed.
2) Put in the \n for messages that tell the user that he/she entered
something wrong and should try again.

I should have also stated my position on "\n" in die() and warn() is:
Do include \n when you're working with tainted values.
-Joe

 

[Eric Bohlman]

You *must* use locking in a situation like this, or you will end up with
a corrupted database. Something like:

use Fcntl qw/:flock/;

{
open my $OLD, '<', ...;
flock $OLD, LOCK_SH or die "can't acquire read lock: $!";
# read stuff
}
# $OLD closes here, lock is released

At which point someone else who runs the script can now read the file, and
it will be identical to what you have...

if ($found) {
open my $NEW, '>', ...;
flock $NEW, LOCK_EX or die "can't acquire write lock: $!";
# write stuff
}

But the two array contents won't be identical at this point (unless both
users deleted the same item), so one of them will win and the other will
lose.

# ditto for $NEW

Alternatively, you could open the file '+<' and then lock it LOCK_EX,
and truncate it rather than closing/reopening.

Which solves the problem. In a "read and update" situation you can't
afford to release the lock in between.

 

[Anno Siegel]

That additional information is completely useless to the end user.
1) Leave out the trailing \n for messages that tell the programmer
there is a bug in the program that needs to be fixed.
2) Put in the \n for messages that tell the user that he/she entered
something wrong and should try again.

That takes care of errors by the end user and errors by the programmer.

A module also has an intermediate user, the author of a module or script
that uses the given module. Errors on this level should normally be
treated by croak() from Carp.pm, which (tries to) report an error from
the caller's perspective. croak() is never usefully called with a
trailing line feed, since the modified error location is the only
difference to die().

Together with the choice whether to die or just to warn this can make
the decision of how to treat a particular situation non-trivial.

Anno

 

[Ben Morrow]

Which solves the problem. In a "read and update" situation you can't
afford to release the lock in between.

Gah! Sorry, yes, you're right of course. Clearly not thinkig straight...

Ben