6
\$\begingroup\$

The below code removes empty line from test.txt. I posted this answer on StackOverflow but someone commented that this code needs review and suggested me to post here. Please post your comments!

use strict;
use warnings;

my $file = "test.txt";
 open (FH, "+< $file");
my @lines = ();
my $i = 0;
while (<FH>)
{
    if ( m/^\s*$/ )
    {
        print "Found an empty line\n";
    }
    else
    {
        $lines[$i] = $_;
        $i++;
    }
}
close FH;
open (FH, "+> $file");
print FH @lines;
close FH;
\$\endgroup\$
7
\$\begingroup\$

Don't indent the open line differently to the surrounding lines.

Always check the result of open, or use autodie.

Why do you use the + modes to open the file? Just use plain < and > if you don't need to read and write, respectively.

Don't use bareword filehandles, use variables:

open my $fh, '<', $filename or die $!;

There's no need to use the variable $i, you can just push to the array.

If the input file is very large, your code could run out of memory. You don't need to store all the lines in an array, you can print them to a different file and rename it to the original name at the end.

\$\endgroup\$
5
\$\begingroup\$

Your program is a decent attempt to remove empty lines, and for sure, it works, but, how could it be better? (The StackOverflow folk are right, a review could help).

First up, the use strict; and use warnings; are good things. It's important to let the system help you as much as possible when writing code.

But, there are a few things which concern me. First up is the Unix philosophy of not polluting input files, and using pipes and redirection where possible. In your case, removing lines "in place" - saving over the input file - is a "bad thing". Instead, you should use the concept of reading from standard input, and outputting to standard output. This allows for there to be a more natural use-case for people ("path of least surprise"). It also makes standard Ctrl-C type actions safer (a Ctrl-C in the middle of the write will leave you with nothing - no input, and broken output).

Obviously, having the file name as a constant in the program is a bad thing too. It should be a program argument, or just taken from stdin.

As choroba mentions, there's also the concern for running out of memory in large files as you buffer the whole input before writing it out again.

Finally, perl makes STDIN handling easy. With the <> file-handle, you can read STDIN or the input files if there are any....

Perl makes liberal use of short-circuit gates when looping. next is used often.

Putting this all together, I would have something like:

#!/usr/bin/perl -w
use strict;
use warnings;

while (<>) {
    next if m/^\s+$/;
    print;
}

Note that there are no reasons to have file handles.... and the lines are kept in the default variable $_ at all times.

You would use it like:

cat file.txt | stripempty > file_mod.txt

or you can put file.txt on the argument line:

stripempty file.txt > file_mod.txt

Of course, on a unix machine you can also do:

cat file.txt | sed -re '/^\s*$/d'

Or, sed even supports editing in-place with:

sed -i -re '/^\s*$/d' file.txt

Note that the -i in-place process does not simply overwrite the input file. It creates a new file to put the output in, and then, when it's complete, it renames the new file over the input file (which is a safe, atomic operation which can't fail part way through).

\$\endgroup\$
  • \$\begingroup\$ I don't think using $_ is a good habit to get into, but for a tiny script like this it does make things cleaner. \$\endgroup\$ – chicks Nov 11 '17 at 2:03
  • \$\begingroup\$ @chicks: That isn't a common view amongst experienced Perl programmers, and I don't think it belongs in a code review. \$\endgroup\$ – Borodin Nov 18 '17 at 13:06
  • \$\begingroup\$ My view is in perlmonks.org/?node_id=989115 and the Perl Best Practices book. Your idea of the "common" view seems rather selective and limited. \$\endgroup\$ – chicks Nov 18 '17 at 13:12
  • \$\begingroup\$ perl -n -i -e 'print if /\S/' file.txt \$\endgroup\$ – Brad Gilbert Nov 18 '17 at 21:29
4
\$\begingroup\$

For this simple text-processing task, a lot of simplifications and improvements are possible.

  • It would be nice to be able to specify the filename as a command-line argument rather than hard-coding "test.txt".
  • You should process the file one line at a time, rather than storing the entire file contents in memory. This is possible, since the output will never be longer than the input, and thus will never overwrite lines of the file before you have had a chance to read them.
  • The $i counter is unnecessary; you can simply use the push function to append an element to an array.

Furthermore, you can take advantage of special Perl processing modes for operating on each line in a file:

  • -i: Treat command-line arguments as filenames; each file is to be edited in place.
  • -p: Put an implicit loop around your program, such that for each line it will print $_ for you automatically. (You could also use -n instead.)

Here's one simple solution:

#!/usr/bin/perl -pi

if (/^\s*$/) {
    print STDERR "Found an empty line\n";
    undef $_;
}
\$\endgroup\$
  • \$\begingroup\$ -i doesn't affect how command-line arguments are treated; it simply implements in-place editing if the contents of @ARGV are treated as input files and read using <>. It also doesn't work without a parameter on Windows. \$\endgroup\$ – Borodin Nov 18 '17 at 13:09
  • \$\begingroup\$ @Borodin That is what the original code did: edit test.txt in place. So, run my program with test.txt as a command-line parameter. \$\endgroup\$ – 200_success Nov 18 '17 at 13:12
  • \$\begingroup\$ That's fine, except that, as I said, your code won't work on Windows. I'm simply disputing your description of what -i does. \$\endgroup\$ – Borodin Nov 18 '17 at 13:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.