Quoth (e-mail address removed):
My bad about the post. I have now read the guidelines.
....but seem to be unwilling to follow them.
Where is
use strict;
use warnings;
?
open (FILE, "+<test.txt") || die ("test.txt wont open");
Use two-arg open.
Use lexical filehandles.
Say why the open failed.
The first rule of programming is: never type anything twice. Put the
filename in a variable.
my $database = 'test.txt';
open(my $FILE, '+<', $database) || die("'$database' won't open: $!");
If you use or instead of || those parens are optional:
open my $FILE, '+<', $database
or die "can't open '$database': $!";
but the code's OK like that if you find it easier to understand.
Don't *ever* use numeric constants for flock, seek, &c. Ask Perl to get
them right for you.
Check that you got the lock.
use Fcntl qw/:flock/; # this goes at the top
flock $FILE, LOCK_EX
or die "can't lock '$database': $!";
You need to declare variables under 'use strict'.
my @lines = said:
These would need declaring as well, except you very rarely need loop
counters in Perl. Perl knows how long your arrays are.
You do, however, need
my @splitdata;
foreach $element (@lines) {
^^ my
($element1, $element2) = split(/\|/, $element);
Variables with similar names are a red flag. They mean you should be
using data structures: in this case, an array.
my
@elements = split(/\|/, $element);
@splitdata[$j] = $element1;
Don't use @ for accessing single array elements. It appears to work, but
it doesn't mean quite what you think it does. See perldoc -q @array.
$splitdata[$j] = $element1;
@splitdata[$k] = $element2;
This is pointless. $j and $k will always be equal, so you set the same
element twice. Every other element will be blank. I presume you meant to
set successive elements?
Use Perl's array operators, and then you don't need the temporaries
either:
push @splitdata, split /\|/, $element;
It's generally clearer to use the assignment operators where you can:
$j += 2;
, except, of course, that you don't need this at all.
$k=$k + 2;
}
foreach $p (param()) {
What is param? You didn't include it in your script. I'm going to guess
you meant to put
use CGI qw/:standard/;
at the top: when the posting guidelines say 'a short, complete script'
they mean you need to post the *whole* of your script. You also need to
provide example input: in this case, a query string.
Note that the order of parameters is not guaranteed to be the same as
the order of fields in the form. You should give your paramaters names
and access them by name.
$formvalue = param($p);
@formvalue[$i] = $formvalue;
if ($formvalue eq "") {
@final[$i] = @splitdata[$i];
}
else {
@final[$i] = $formvalue;
}
This can be written more simply as
$final[$i] = $formvalue || $splitdata[$i];
I don't really understand what you are trying to achieve here. A
somewhat cleaner solution might be
my @params = params;
my %final;
$final{$_} = shift @splitdata for @params;
$final{$_} = param($_) for grep param($_), @params;
but I really think that a better answer would be to put the form field
names into your database. Do you have to use that file format? If you
could change to something like GDBM, then you could reduce the whole
script down to
use warnings;
use strict;
use GDBM_File;
use CGI qw/:standard/;
tie my %data, GDBM_File => './data', GDBM_WRCREAT;
$data{$_} = param($_) for grep param($_), params;
as GDBM handles locking for you. This will consider a parameter value of
'0' to be equivalent to '', so you may prefer
$data{$_} = param($_) for grep { length param($_) }, params;
It also assumes that your fields all have unique names; again, this may
not be the case, but you didn't provide us with any input data.
$x=0;
$y=0;
$z=0;
foreach (@final) {
$firstpart = @final[$x];
$secondpart = @final[$y];
Again, $x and $y are the same, so you're getting the same element twice.
Also, the loop loops once for every element in @final, but you're adding
2 to $x each time, so before long you'll be off the end of the array.
$writevalue = "$firstvalue|$secondpart";
What is $firstvalue? Is this your real script, or is that the bug you
were having trouble with? I can't see how it would produce the effect
you describe, though, unless this is not your whole script.
@finalwrite[$z] = $writevalue;
$x = $x + 2;
$y = $y + 2;
$z++;
}
seek FILE, 0, 0;
Don't use magic numbers, use constants.
use Fcntl qw/:seek/;
seek $FILE, 0, SEEK_SET;
truncate FILE, 0;
print FILE @finalwrite;
Since you haven't set $,, this will print the array elements all on one
line, separated by spaces. I don't think that's what you meant.
Ben
