Beefy Boxes and Bandwidth Generously Provided by pair Networks
There's more than one way to do things
 
PerlMonks  

File handles and loops

by dr.jekyllandme (Sexton)
on Jul 31, 2012 at 06:45 UTC ( [id://984576]=perlquestion: print w/replies, xml ) Need Help??

dr.jekyllandme has asked for the wisdom of the Perl Monks concerning the following question:

Hello, I am trying to write a program that will allow a user to edit a file using editor. Once the user quits the editor, I am going to parse the file to make sure that the expected format of this file has been followed. If not, my program will print out an error message for the line number and reopen the file to allow the user to correct their mistakes. This will go on until they have correctly conformed to the expected format. For example, my rule is that all lines should begin with a lowercase letter. My code so far does this, but I know that I am messing up on dealing with file handles. I am using a loop to parse each line of my file to make sure the string is valid. If it is not, I close my current file handle and allow another edit to occur. Below is my code so far: 
my $string = join( "\n", qw(howdy partner goodbye Friend saynora adios
+ Amigo) );
open(MY_FILE, '>my_file.txt');
print MY_FILE $string;
close(MY_FILE);
&edit_file('my_file.txt');

sub edit_file {
    my $my_file = shift;
    my $editor = $ENV{'editor'};
    system('vim', $my_file);
    my $validate = &validate_file($my_file);
    print "File is ok\n" if $validate;
}

sub validate_file {
    my $my_file = shift;
    open( MY_FILE, $my_file ) or die "Unable to open file\n";
    while( <MY_FILE> ) { 
        chomp( $_ );
        unless( $_ =~ /^[a-z]/ ) { 
            close( MY_FILE );
            print "Error at line $.\n";
            print "$_ does not begin with a lower case letter\n";
            print "Hit return to continue: ";
            <STDIN>;
            &edit_file( $my_file );
        }   
    }   
    close( MY_FILE );
    return 1;
}

[download]
I am not sure if my approach is correct. If anyone can give me pointers how to improve my code, it would be great. Thank you.

Replies are listed 'Best First'.
Re: File handles and loops
by Athanasius (Archbishop) on Jul 31, 2012 at 07:42 UTC

    Hello dr.jekyllandme,

    Neighbour has dealt with the main problem — the inappropriate recursion — in your code. However...

    If anyone can give me pointers how to improve my code, it would be great. Thank you.

    Well, since you asked...  ;-)

    • Always  use strict;

    • Always  use warnings;

    • Prefer lexical filehandles (with lowercase names):  my $fh

    • Use the 3-argument form of open: 
      open(my $fh, '>', 'my_file.txt');

      [download]
    • Either use autodie; at the head of your script, or else add 
      or die "...";

      [download]
      after every open and close.

    • Do not use the & sigil when calling functions: 
      edit_file('my_file.txt');

      [download]
    • If using the default variable $_, you often don’t need to name it explicitly: 
      open(my $fh, '<', $myfile) or die "Unable to open file\n";

while (<$fh>)
{
    chomp;             # <-- here and
    unless (/^[a-z]/)  # <-- here
    {
        ...

      [download]

    HTH,

    Athanasius <°(((>< contra mundum

[reply]
[d/l]
[select]

      In addition, this

          my $editor = $ENV{'editor'};
    system('vim', $my_file);

      [download]

      should be like this:

          my $editor = $ENV{'EDITOR'} || 'vim';
    system($editor, $my_file);

      [download]
[reply]
[d/l]
[select]

        Also consider Proc::InvokeEditor. It doesn't work as nicely on Windows, as it doesn't have notepad.exe as the fallback editor there, but if you have vi(m) somewhere, it'll get used.

[reply]
[d/l]
Re: File handles and loops
by Neighbour (Friar) on Jul 31, 2012 at 07:11 UTC
    In your validate_file sub, you open the file and while looping through the lines, (if something's wrong), close the file and allow it to edit. But what happens after the editing is done? The file is still closed, the while loop will stop (since it can't read from the closed filehandle) it will attempt to close the filehandle again, and return 1. This is not what you intended :)
    How about something like this (untested)? 
    sub validate_file {
    my $file_ok = 0;
    while (not $file_ok) {
        $file_ok = 1; # Let's assume the file has no error
        open( MY_FILE, $my_file ) or die "Unable to open file\n";
        while( <MY_FILE> ) { 
            chomp( $_ );
            unless ( $_ =~ /^[a-z]/ ) {
                close( MY_FILE );
                print "Error at line $.\n";
                print "$_ does not begin with a lower case letter\n";
                print "Hit return to continue: ";
                <STDIN>;
                &edit_file( $my_file );
                $file_ok = 0;
            }
        }
        close( MY_FILE );
    }
    return 1;
}

    [download]
    The initial $file_ok-value seems a bit off, but this is to make sure the while loop triggers at least once :)
    This method means that validate_file is called once, and will only return when the file is ok. I'm not sure if that's what you intended. You could move the while (not $file_ok)-loop out of validate_file-sub and keep calling the sub while the file is not ok.
[reply]
[d/l]
[select]
Re: File handles and loops
by anazawa (Scribe) on Jul 31, 2012 at 08:24 UTC
    Your problem interests me :) AnyEvent will help you manage loops: 
    use strict;
use warnings;
use AnyEvent;

my $file = 'my_file.txt';
my $cv = AnyEvent->condvar;

print "start editing $file ... Ľn";

# main loop
my $reader = AnyEvent->io(
    fh => \*STDIN,
    poll => 'r',
    cb => sub {
        system( 'vim', $file );
        if ( is_valid( $file ) ) {
            print "File is okĽn";
            $cv->send; # exits "main loop"
        }
        else {
            print "Hit return to continueĽn";
        }
    },
);

# enters "main loop" till $cv gets ->send
$cv->recv;

sub is_valid {
    # validate my_file.txt here
}

    [download]
[reply]
[d/l]
Re: File handles and loops
by tobyink (Canon) on Jul 31, 2012 at 09:56 UTC

    I think you mingle the validation code and the user interaction code too much. Here's a somewhat cleaner version...

    use 5.010;
use strict;
use warnings;

{
    package Local::File;
    
    use Any::Moose;
    use overload q[""] => \&contents;
    
    has filename => (
        is       => 'ro',
        isa      => 'Str',
        required => 1,
    );
    
    sub open
    {
        my $self = shift;
        my $mode = shift // '<';
        open my $fh, $mode, $self->filename
            or confess sprintf(
                "could not open '%s' in '%s' mode: $!",
                $self->filename,
                $mode,
            );
        return $fh;
    }
    
    sub init
    {
        my $fh = shift->open('>');
        say $fh $_ for qw(
            howdy partner goodbye Friend saynora adios Amigo
        );
    }
    
    sub edit
    {
        system($ENV{EDITOR}, shift->filename);
    }
    
    sub errors
    {
        my $fh = shift->open;
        my @errors;
        while (<$fh>)
        {
            next if /^[a-z]/;
            chomp;
            push @errors, qq{Line does not begin with lower-case lette
+r, syntax error on line $. ("$_")};
        }
        return @errors;
    }

    sub contents
    {
        my $fh = shift->open;
        local $/ = undef;
        return <$fh>;
    }
}


my $file = Local::File::->new(filename => '/home/tai/tmp/my-file.txt')
+;
$file->init;
while (my @errors = $file->errors)
{
    say for @errors;
    say "please press ENTER to edit file";
    <STDIN>;
    $file->edit;
}

say "Valid file!";
say $file;

    [download]
    perl -E'sub Monkey::do{say$_,for@_,do{($monkey=[caller(0)]->[3])=~s{::}{ }and$monkey}}"Monkey say"->Monkey::do'
[reply]
[d/l]
      Hi tobyink, Thank you for replying. You, Neighbour, and the other Monks really helped me out. But I wanted to ask you how should I close the filehandle $fh after it has been opened? I've written my code somewhat similar to you and Neighbours but I am not sure if it is correct. Here is a snippet of the problem: 
      sub my_close {
    my( $fh ) = @_;                                                   
+                                               
    close($fh) or die "$!\n";
}                                                                     
+                                               
sub my_open {                                                         
+                                               
    my( $file, $mode ) = @_;                                          
+                                               
    open my $fh, $mode, $file or die "$!\n";                          
+                                               
    return $fh;
}
sub errors {                                                          
+                                               
    my( $file, $mode ) = @_;
    my $fh = my_open($file, $mode);
    my @errors = ();
    while( <$fh> ) {
        chomp;                                                        
+                                               
        next if /^[a-z]/;
        push( @errors, qq(Line $. '$_' does not begin with lowercase l
+etter.) );                                     
    }
    my_close($fh); # Safe to close file like this???                  
+                                                                     
+           
    return @errors;
}

      [download]
      Thank you.
[reply]
[d/l]

        You rarely need to close lexical file handles. They automatically close once the variable goes out of scope. That is, if the my $fh is defined within a function, then once that function finishes executing, the file handle disappears.

        perl -E'sub Monkey::do{say$_,for@_,do{($monkey=[caller(0)]->[3])=~s{::}{ }and$monkey}}"Monkey say"->Monkey::do'
[reply]
[d/l]
Re: File handles and loops
by Neighbour (Friar) on Jul 31, 2012 at 09:23 UTC
    Also, you could change your verification code to be somewhat more like this: 
    #!/usr/bin/perl

use strict;
use warnings;
use v5.10;

sub edit_file {
    my $my_file = shift;
    my $editor = $ENV{'EDITOR'} || 'vim';
    system($editor, $my_file);
}

sub validate_file {
    my ($my_file) = @_;
    my $error;

    open(MY_FILE, '<', $my_file) or die "Unable to open file [$my_file
+]: $!";
    while(<MY_FILE>) {
        chomp;
        given ($_) {
            print("Checking [$_]\n");
            if (/^[^[:lower:]]/) { # Line does not start with a lowerc
+ase letter
                $error = "$_ does not begin with a lower case letter";
            }
            if (/somethingorother/) {
                $error = "$_ has somethingorother";
            }
            default {
                # Line is ok
            }
        }
        if ($error) {
            $error = "At line [$.]: $error";
            close( MY_FILE );
            return $error; # Stop parsing the file after the first err
+or
        }
    }
    close(MY_FILE);
    return undef;
}

my $string = join( "\n", qw(howdy partner goodbye Friend saynora adios
+ Amigo) );
open(MY_FILE, '>', 'my_file.txt') or die "Can not open my_file.txt: $!
+";
print MY_FILE $string;
close(MY_FILE);
edit_file('my_file.txt');

my $error = 'Nothing went wrong';
while ($error) {
    $error = validate_file('my_file.txt');
    if ($error) {
        print "Error validating file: $error\n";
        print "Hit return to continue: ";
        <STDIN>;
        edit_file('my_file.txt');
    }
}
print "File is ok\n";

    [download]
    This uses a given-when construct to allow you to easily add more checks. It also incorporates some of the suggestions mentioned by other monks.
    The validation-loop has been moved out of validate_file and the returnvalue of validate_file has been used in a more descriptive matter.
    Note that this relies on the truth and falsehood of the empty string (undef is false, empty string is false, nonempty string is true).
[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?

www.com | www.net | www.org
Node Status?
node history
Node Type: perlquestion [id://984576]
Approved by Ratazong
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? • Last hour • Other CB clients
Other Users?
Others studying the Monastery: (5)
As of 2024-04-19 07:59 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found