Re: File modification
by liverpole (Monsignor) on Jul 07, 2006 at 12:24 UTC
|
Hi madtoperl,
A number of suggestions, actually...
- You should use "strict" and "warnings".
- You should use the 3-arg form of open.
- You should check for failure conditions, such as open failing, or @ARGV not being set.
- If you use the module FileHandle, your filehandles don't have to be typeglobs.
- It is redundant to do ($_ =~ /.../); instead just do (/.../).
- If you use regex captures, you don't have to split afterwards.
- You're not using $changeflag; get rid of it.
- Finally, you're writing every line to the file, so why not just do a regex substitution and, if it fails, write the line anyway?
- It's considered good form (though not technically necessary in this case) to close your files when you're done with them.
Here's my rewrite:
#!/usr/bin/perl -w
use strict;
use warnings;
use FileHandle;
(my $argact = shift) or die "Syntax: $0 <fileid>\n";
my $input = new FileHandle;
my $newfile = new FileHandle;
open($input, "<", "one.txt") or die "Failed to read 'one.txt' ($!)\n
+";
open($newfile, ">", "output") or die "Failed to write 'output' ($!)\n
+";
while(<$input>){
s/^Authentication\s+number\s+is\s+(\d+)/Authentication number is $
+argact/;
print $newfile "$_";
}
close $input;
close $newfile;
Isn't that a lot cleaner?
s''(q.S:$/9=(T1';s;(..)(..);$..=substr+crypt($1,$2),2,3;eg;print$..$/
| [reply] [Watch: Dir/Any] [d/l] [select] |
|
liverpole:
++ Excellent suggestions! One trivial note, though: Your blank lines appear to be a long string of blanks, as there are many unexpected line continuation characters on the screen here. (I just did a view source: The code lines don't appear to have trailing blanks, but all the blank lines do. I don't know if it's an artifact of how you pasted the code in, but I thought I'd mention it.)
--roboticus
| [reply] [Watch: Dir/Any] |
|
Thanks, roboticus, for your kind comments, and the feedback about the "long lines". I've fixed them.
Yes you're right, it is an artifact of cut-and-paste. I've seen it before, and usually just fix it by hand, though it can be tedious if it's a long program. I don't know if it's the browser I'm using (Firefox), or if there's some easier way to fix it, but I'd be glad to hear of anyone else who's found a simpler solution.
s''(q.S:$/9=(T1';s;(..)(..);$..=substr+crypt($1,$2),2,3;eg;print$..$/
| [reply] [Watch: Dir/Any] |
|
| [reply] [Watch: Dir/Any] [d/l] |
Re: File modification
by davorg (Chancellor) on Jul 07, 2006 at 12:15 UTC
|
$argact = $ARGV[0] or die "No number given\n";
open INPUT, 'one.txt' or die $!;
open NEWFILE, '>', 'output' or die $!;
while (<INPUT>){
if(/^Authentication\s+number\s+is/) {
my ($old_num) = /(\d+)/;
if ($argact != $oldnum) {
$changeflag = 1;
s/\d+/$argact/;
}
}
print NEWFILE;
}
Note: I've included $changeflag only because it was in the original code. I've no idea what it's there for. Maybe its value is checked later on in the program.
Update: Typo fixed.
--
< http://dave.org.uk>
"The first rule of Perl club is you do not talk about
Perl club." -- Chip Salzenberg
| [reply] [Watch: Dir/Any] [d/l] |
|
Update: too slow ...
Minor nit ...
s/\d+/$argact;
is missing a closing /.
| [reply] [Watch: Dir/Any] [d/l] [select] |
|
Hi davorg
I am running your code and getting the following compilation error,
[ristee:croco]perl -c filemod.pl
Substitution replacement not terminated at filemod.pl line 11.
thanks
| [reply] [Watch: Dir/Any] [d/l] |
|
| [reply] [Watch: Dir/Any] |
Re: File modification
by graff (Chancellor) on Jul 07, 2006 at 13:01 UTC
|
I think it's very odd that no one has thought to check that the command-line arg is actually a number. Also, if the idea is to replace the existing "one.txt" file, no one seems to have thought about renaming the output file back to "one.txt".
#!/usr/bin/perl
use strict;
( @ARGV == 1 and $ARGV[0] =~ /^\d+$/ )
or die "Usage: $0 N\n where N is a new number for one.txt\n";
open( I, "one.txt" ) or die "$0: reading one.txt: $!";
open( O, "one.txt.new" ) or die "$0: writing one.txt.new: $!";
while(<I>) {
s/(Authentication number is) \d+/$1 $ARGV[0]/;
print O;
}
close I;
close O;
rename "one.txt.new", "one.txt";
Note that the substitution will have no effect on lines that do not match the regex -- it only changes the digit(s) at the end of the targeted line. | [reply] [Watch: Dir/Any] [d/l] |
Re: File modification
by jdporter (Paladin) on Jul 07, 2006 at 13:01 UTC
|
#!perl -i.bak
my $newnum = shift;
@ARGV = qw( one.txt );
undef $/; # sluurp
$_ = <>;
s/(Authentication number) \d+/$1 $newnum/;
print
The critical piece in all that is the -i.bak option passed to perl. If you can't do it on the shebang line, do it the exec (cmd) line of perl.
We're building the house of the future together.
| [reply] [Watch: Dir/Any] [d/l] [select] |
Re: File modification
by TedPride (Priest) on Jul 07, 2006 at 15:34 UTC
|
You don't need a 3-argument open when the open doesn't have any user input in it. And obfuscating the code doens't really help either, even if the result is shorter.
use strict;
use warnings;
my ($argact, $handle, $num);
($argact) = $ARGV[0] =~ /(\d+)/;
exit if !$argact;
open $handle, 'one.txt';
$_ = join '', <$handle>;
close $handle;
($num) = m/(\d+)$/;
if ($num != $argact) {
s/(\d+)$/$argact/;
open $handle, '>one.txt';
print $handle $_;
close $handle;
}
| [reply] [Watch: Dir/Any] [d/l] |
|
It's true you don't need the 3-argument open, but I believe it's considered good programming practice, if for no other reason than getting users in the habit of using safer code.
Your example has other issues, though. While it's true you're exiting if the argument isn't numeric, it might be confusing to the user not to get a syntax message. And if no argument is given, instead of a more friendly syntax message one gets:
Use of uninitialized value in pattern match (m//) at ./mytest line 8.
Additionally, because of the '$' in the regex, there's the problem of input which doesn't contain digits at the very end, which will fail even if there's an extra newline in the input:
Use of uninitialized value in pattern match (m//) at ./mytest line 15.
s''(q.S:$/9=(T1';s;(..)(..);$..=substr+crypt($1,$2),2,3;eg;print$..$/
| [reply] [Watch: Dir/Any] [d/l] [select] |