11
\$\begingroup\$

Description

Print all student names from a CSV file.

CSV Format:

  • CSV file does not contain string delimited by double quotes.
  • None of the cells has "," character or "\"" character.
  • There are exactly two columns.
  • No empty cells.

Code

csvread.pl

#!/usr/bin/perl

use strict;
use warnings;

my $filename = "students.csv";
open (STUDENTS,$filename) ||  die "Cannot Open '$filename'";

# get column names
my $line = <STUDENTS>;
chomp($line); 
my @column_names = split(",",$line);

print "Printing Student ${column_names[1]}s...\n";

# get all names
while($line = <STUDENTS>){
    chomp($line);
    (my $id,my $name) = split(",",$line);
    print "$name\n";
}

#--------------------------------------------------------
# documentation
#


=head1 NAME

Print Student Names

=head1 SYNOPSIS

    chmod a+x ./csvread.pl
    ./csvread.pl

=cut

students.csv

ID,Name
1,Ashani
2,Sandeepani
3,Nimesha
4,Kaveesha
5,Abimani
6,Himadu
7,Vishmi
8,Shela
9,Vishvani
10,Claudia
11,Dinuwanthi
12,Anthonella
13,Sachini
14,Hiruni
15,Nimesha
16,Kavindi
17,Roshini
18,Dilini
19,Devinka
20,Noela
21,Nithmi
22,Azekah
23,Ishara
24,Andriya
25,Rishini
26,Shashini
27,Oshadhi
28,Sandali
29,Jehani
30,Thamasha
31,Shalini
32,Milka
33,Gihani
34,Sakuni
35,Piumi
36,Stefania
37,Kinkini
38,Shehana
39,Nimanshi
40,Emalsha
41,Shohani
42,Madusha
43,Nisaja
44,Kiyara
45,Dinithi
46,Sadunika
47,Sewmini
48,Rachel

Review

Conventions/Generic

\$\endgroup\$
7
\$\begingroup\$

It looks quite good. Perldoc, use strict, use warnings are all good practices. The formatting is good, and the code is readable.

I don't recommend hard-coding the name of the data file in your program. That turns it from a general-purpose program into throwaway code. If you use the default filehandle <>, then Perl will do the "right thing": if a filename is specified as a command-line parameter, it will read from that file; if not, it will read from STDIN. As a bonus, it saves you the trouble of having to call open(). (You neglected to explicitly close(STUDENTS), by the way.)

The CSV-parsing code is repeated for the header row and for the body. It should be made into a subroutine.

(my $id, my $name) could be written more idiomatically as my ($id, $name).

I find it annoying to use string interpolation like print "$something\n" just because you need a newline. I suggest taking advantage of the newish say feature.

The facts that you want the second column, and that the columns are the $id and $name, are both hard-coded. Perhaps you should add an assertion.

Suggested implementation

use strict;
use warnings;
use feature qw(say);

sub columns {
    my $line = shift;
    return unless defined $line;
    chomp($line);
    split ',', $line;
}

my @column_names = columns(scalar <>);

die unless $column_names[1] eq 'Name';
say "Printing Student ${column_names[1]}s...";
while (my ($id, $name) = columns(scalar <>)) {
    say $name;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Explicit return for split makes sense; no need for ${} interpolation syntax, and while (my $line = <>) is not only more idiomatic but it would simplify columns() as well. \$\endgroup\$ – mpapec Oct 21 '14 at 11:32
4
\$\begingroup\$

@Brythan is very right about using a proper variable my $STUDENTS instead of the bare-word STUDENTS. Just to add some examples of reading from that:

my $line = <$STUDENTS>;    
while ($line = <$STUDENTS>) {

I recommend to adopt good formatting practices from other languages, for example:

  • put a space after commas in parameter lists
  • put a space between ){
  • put a space between if( and while(
  • .... and so on

For example instead of this:

my @column_names = split(",",$line);
while($line = <STUDENTS>){

Write like this:

my @column_names = split(",", $line);
while ($line = <STUDENTS>) {

Instead of this:

my $line = <STUDENTS>;
chomp($line); 

This is simpler and a very common practice in Perl:

chomp(my $line = <STUDENTS>);

This could have been simpler:

(my $id,my $name) = split(",",$line);

Like this:

my ($id, $name) = split(",", $line);
\$\endgroup\$
3
\$\begingroup\$

I'd write the open this way:

open my $STUDENTS, "<", $filename or die "Cannot open '$filename':  '$OS_ERROR'";

Using the my variable makes the file handle locally scoped. The bare word (typeglob) is globally scoped and can potentially conflict with modules.

Use or rather than || because its precedence is lower than a ,.

Show the error with $OS_ERROR, which is another name for $!.

chomp $line;

To differentiate built-in functions from subroutines, I write them without parentheses. I may have gotten this from Damian Conway's Perl Best Practices.

my (undef, $name) = split ',', $line;

Since you never use $id, there's no reason to have it at all. Use undef to silently drop it.

I'm more used to the my (list...) format. Note that if there were a third entry, you could save a my.

If you are not using variable interpolation, you might as well use a single-quoted string. There's a slight performance benefit, and it helps differentiate between strings that contain variables and those that don't.

\$\endgroup\$
  • \$\begingroup\$ use English => $OS_ERROR \$\endgroup\$ – mpapec Oct 21 '14 at 11:18

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.