current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -1 down vote favorite
1

I am looking for a Perl (5.8.8) script for CSV parsing that would follow CVS standards.

(See Wikipedia or RFC-4180 for details)

Sure, code should be fast enough, but more important is that it should use no libraries at all.

This is what I have for now :

#!/usr/bin/perl

use strict;
use warnings;

sub csv {
  no warnings 'uninitialized';
  my ($x, @r) = (pop, ()); my $s = $x ne '';
  $x =~ s/\G(?:(?:\s*"((?:[^"]|"")*)"\s*)|([^",\n]*))(,|\n|$)/{
    push @r, $1.$2 if $1||$2||$s; $s = $3; ''}/eg;
  $r[$_] =~ s/"./"/g for 0..@r-1;
  $x? undef : @r;
}

@test = csv( '"one",two,,"", "three,,four", five ," si""x",,7, "eight",' .
          ' 9 ten,, ' . "\n" . 'a,b,,c,,"d' . "\n" . 'e,f",g,' );

(!defined $test[0])? die : print "|$_|\n" for @test;

Same code, but with comments :

#!/usr/bin/perl

use strict;
use warnings;

sub csv {

  no warnings 'uninitialized';
  # we can use uninitialized string variable as an empty without warning

  my ($x, @r) = (pop, ()); my $s = $x ne '';
  # function argument (input string) goes to $x
  # result array @r = ()
  # variable $s indicates if we (still) have something to parse

  $x =~ s/\G(?:(?:\s*"((?:[^"]|"")*)"\s*)|([^",\n]*))(,|\n|$)/{
  # match double-quoted element or non-quoted element
  # double-quoted element can be surrounded with spaces \s* that are ignored
  #   and such element is any combination of characters with no odd sequence 
  #   of double-quote character ([^"]|"")*
  # non-quoted element is any combination of characters others than double-quote
  #   character, comma or new-line character ([^",\n]*)
  # element is followed by comma or new-line character (for non-quoted elements) 

    push @r, $1.$2 if $1||$2||$s;
    # if match found, push it to @r result array
    $s = $3; 
    # do we (still) have something to parse?
    ''
    # replace match with empty string, so at the end we can check if all is done

  }/eg;
  # /e = execute { ... } for each match, /g = repeatedly

  $r[$_] =~ s/"./"/g for 0..@r-1;
  # replace double double-quotes with double-quote only

  $x? undef : @r;
  # if $x is not empty, then CSV syntax error occurred and function returns undef
  # otherwise function returns array with all matches
}

@test = csv( '"one",two,,"", "three,,four", five ," si""x",,7, "eight",' .
          ' 9 ten,, ' . "\n" . 'a,b,,c,,"d' . "\n" . 'e,f",g,' );
# simple test

(!defined $test[0])? die : print "|$_|\n" for @test;
# die if csv returns an error, otherwise print elements surrounded with pipe char |

The code gets the following output:

|one|
|two|
||
||
|three,,four|
| five |
| si"x|
||
|7|
|eight|
| 9 ten|
||
| |
|a|
|b|
||
|c|
||
|d
e,f|
|g|
||

All improvements will be appreciated.

share|improve this question
1  
1) Wikipedia is NOT an authoritative source. Don't use it when quoting standards. –  Loki Astari Mar 28 '12 at 6:06
1  
Maybe use /x modifier in your regular expression so it can be understood by more humans. Don't use l (el) as a variable because it looks like a 1 (one). –  Apprentice Queue Mar 28 '12 at 6:08
4  
2) That is completely unreadable. The point of a review is to try and make the code more maintainable. As it stands this code is completely pointless. In a year when you come back you will not understand what it does let a lone how to fix it. Though Perl has a reputation of being a write once language it need not be, you can write readable maintainable and efficient perl. –  Loki Astari Mar 28 '12 at 6:09
1  
Loki's point 2 almost deserves to be an answer here. Someone asking to review this code should get the "this is unreadable, start over" advise. All I can add is "I hope you have a bunch of unittests that cover every imaginable case, else maintaining this script will be hell." –  Konerak Mar 28 '12 at 13:33
1  
What's the point in adding the explanation in the question. The explanation should be with the code. An explanation not in the code is not going to help your maintainer next year (nor yourself). –  Loki Astari Mar 28 '12 at 14:34

1 Answer 1

up vote 6 down vote accepted

General Overview

  • It is unreadable
    • This is OK as an exercise for your reg ex muscles.
    • BUT this is not maintainable code. As such it is would never get past a code review at any company or get placed in production.
    • You may get away with it for a one off script that you throw away.

Code Comments

Perl has a reputation as being unreadable. Fortunately it does not need to be (unless you are entering the Perl obfuscated contest). So best not to write Perl that is unreadable (as you will not understand it next year)

  • Your code is written in a way that makes modification after release nearly imposable. A bug fix or update will have to build the reg-exp from the ground up to understand how it works.

  • Variable names should be meaningful.

    • There is no reason to use @_ for example as your own variable.

  • Always have the following in your code (unless there is a very good reason not too)

    use strict;
use warnings;

  • It is always a good idea to run your code through lint

    perl -MO=Lint foo.pl

Algorithm

Regular expressions are not well suited for parsing complex structures. Though CSV may look simple on the surface; it is unfortunately inherently complexity with nested line breaks and quotes.

A better idea would be to define a real parser using the grammar defined in RFC-4180 as a starting point.

share|improve this answer
6  
@stackoverflow: Your assertions that a readable code can not be light or high performance are wrong or just silly. Also regular expressions (which are general purpose) are not as efficient as a well built parser (which will be specific to the job). My suggestions re-write using a parser then time to see how much slower you code is. –  Loki Astari Mar 28 '12 at 15:39
    
Good starting point to learn CSV parsing (grammar, readers, streams...) is csvreader.com/java_csv.php and ostermiller.org/utils/CSV.html –  Michael Mar 28 '12 at 17:16

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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