Beefy Boxes and Bandwidth Generously Provided by pair Networks
Keep It Simple, Stupid
 
PerlMonks  

File modification

by madtoperl (Friar)
on Jul 07, 2006 at 12:02 UTC ( #559768=perlquestion: print w/replies, xml ) Need Help??

madtoperl has asked for the wisdom of the Perl Monks concerning the following question:

Hi Monks,

I am having a file named one.txt,the contents are like,
This file is having data It is used for authentication Authentication number is 9
When I am running my script I get the new authentication no by @ARGV.If my new authenticaton is not equal to the authentication no found in the one.txt file,the new authentication number should be replaced inthe one.txt file.For this simple work I am using lot of lines.It looks bad for me.Let me know some other professional code to reduce my lines monks.The code is given below,
perl auth.pl 9 $argact = $ARGV[0]; open input,"one.txt"; open newfile,">output"; while(<input>){ if($_ =~ /^Authentication\s+number\s+is/){ @array = split(" ",$_); $fileid = (split(" ",$_))[3]; if($fileid ne $argact){ $changeflag = 1; pop(@array); push(@array,$argact); $newline = join(" ",@array); print newfile "$newline\n"; }else{ print newfile "$_"; } }else{ print newfile "$_"; } }
Thanks and Regards,
madtoperl.

Replies are listed 'Best First'.
Re: File modification
by liverpole (Monsignor) on Jul 07, 2006 at 12:24 UTC
    Hi madtoperl,

    A number of suggestions, actually...

    1. You should use "strict" and "warnings".
    2. You should use the 3-arg form of open.
    3. You should check for failure conditions, such as open failing, or @ARGV not being set.
    4. If you use the module FileHandle, your filehandles don't have to be typeglobs.
    5. It is redundant to do ($_ =~ /.../); instead just do (/.../).
    6. If you use regex captures, you don't have to split afterwards.
    7. You're not using $changeflag; get rid of it.
    8. 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?
    9. 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$..$/
      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

        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$..$/
        ++liverpole,

        just to be pedantic... ;-)

        • check the return value of close FH (think NFS; file system full?)

        --shmem

        oops, this leaf wrong twig :-P

        _($_=" "x(1<<5)."?\n".q/)Oo.  G\        /
                                      /\_/(q    /
        ----------------------------  \__(m.====.(_("always off the crowd"))."
        ");sub _{s./.($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e.e && print}
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

      Update: too slow ...

      Minor nit ...

      s/\d+/$argact;
      is missing a closing /.

      -- Hofmator

      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

        Yes, there was a typo in my code. Pretty simple to fix tho' I would have thought.

        --
        <http://dave.org.uk>

        "The first rule of Perl club is you do not talk about Perl club."
        -- Chip Salzenberg

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.
Re: File modification
by jdporter (Canon) 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.
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; }
      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$..$/

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://559768]
Approved by Hue-Bond
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others about the Monastery: (6)
As of 2021-12-06 12:25 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?
    R or B?



    Results (32 votes). Check out past polls.

    Notices?