3
\$\begingroup\$

I have written this Perl code, and there is still more to add, however I was hoping that I could get some opinions on whether it could be written better.

Specifically, there is a central if-elsif structure. Should I make this a subroutine or not? What's best practice?

Also, look at the 2nd block of code, and let me know if it's worth it to change this to a subroutine (either put entire if-else into sub, or just the inner portion).

Generally, I'm just wondering if there are some better tricks to get the same job done. Perhaps using a array of hashes instead of array of arrays? For easier read. Would this prevent eq matches from appearing?

First, the declarations:

#!/usr/bin/perl

use strict;
use warnings FATAL => "all";
require 'verbTenseChanger.pl';

## -JUST FOR TESTING- ##
my $chapter_section = "chpt"."31_4";
my $search_key = "move";
my $category_id = "all";

# --- Files --- #
open(my $parse_corpus, '<', "parsed${chapter_section}.txt") or die $!;

# --- Different forms of the Searchword --- #
my @temp_changeverbforms = map changeVerbForm( $search_key, 0, $_ ), 1..4;
my @verbforms;
push (@verbforms, $search_key);# Watch extra for loop
foreach my $temp_changeverbforms (@temp_changeverbforms) {
    push (@verbforms, $temp_changeverbforms) unless ($temp_changeverbforms eq "");
}

# --- Variables for required info from parser --- #
my @entirechapter = <$parse_corpus>;
my $chapternumber;
my $sentencenumber;
my $sentence;
my $grammar_relation;
my $argument1;
my $argument2;
my @all_matches;

my $entirechapter = join ('',@entirechapter); ##Flatten file (make one big string)

#To get each sent. and info in one string:
my @sentblocks = split (/Parsing\s/,$entirechapter);##Remove "Parsing" which is on the line of the chptnumber
$chapternumber = $sentblocks[1]; ## file: chpt... will always be at [1]

# --- Retrieve necessary info from text --- #

#Loop through all the sentences, and for each check every form of searchverb
foreach my $sentblock (@sentblocks) {
    foreach my $verbform (@verbforms) { 
        ##"next" skips to next iteration of loop, substitute for an over-arching if statement
        next unless ($sentblock =~ /\b$verbform\b/i); ##Ensure the sentence contains the searchkey
        next unless ($sentblock =~ /\(VB\w*\s+\b$verbform\b\)\s+/i); ##Ensure searchkey is a verb
        #Sent.number and Sentence:
        next unless ($sentblock =~ /\[(sent. \d+) len. \d+\]: \[(.+)\]/); ##Remember, talking about the whole block here
        $sentencenumber = $1;
        $sentence = $2;
        $sentence =~ s/, / /g;

Here is the if-elsif that may or may not be converted to a subroutine:

        #Dependencies (relations and arguments):
        if ($category_id eq "all") {
            my @lines = split ("\n",$sentblock); ##Split by a newline
            foreach my $line (@lines) {
                my @matches;
                next unless ($line =~ /\b$verbform\b/i); ##Ensure dependency contains searchword
                            ##NEXT LINE IS DIFFERENCE:
                next unless ($line =~ /subj\w*\(|obj\w*\(|prep\w*\(|xcomp\w*\(|agent\w*\(|purpcl\w*\(|conj_and\w*\(/); ##Ensure dependency only contains desired grammar relations
                next unless ($line =~ /(\w+)\((\w+)\-\d+\,\s(\w+)\-\d+\)/); ##Ensure dependency is a dependency AND get info from it
                $grammar_relation = $1;
                $argument1 = $2;
                $argument2 = $3;

                next if ($argument1 eq $argument2); ##Ensure 1st and 2nd argument aren't the same
                next if ($grammar_relation =~ /xcomp/i and $argument2 !~ /\b$verbform\b/i); ##Ensure for xcomp the searchword is the 2nd argument
                next if ($argument1 =~ /^\S$/ or $argument2 =~ /^\S$/); ##Exclude if argument is only 1 character

                push(@matches, $chapternumber, $sentencenumber, $sentence, $grammar_relation, $argument1, $argument2); ##All here, so either all get pushed or none (no holes in array)
                push @all_matches, \@matches;
            }
        }
        elsif ($category_id eq "subj") {
            my @lines = split ("\n",$sentblock); ##Split by a newline
            foreach my $line (@lines) {
                my @matches;
                next unless ($line =~ /\b$verbform\b/i); ##Ensure dependency contains searchword
                next unless ($line =~ /subj\w*\(|agent\w*\(/); ##Ensure dependency only contains desired grammar relations
                next unless ($line =~ /(\w+)\((\w+)\-\d+\,\s(\w+)\-\d+\)/); ##Ensure dependency is a dependency AND get info from it
                $grammar_relation = $1;
                $argument1 = $2;
                $argument2 = $3;

                next if ($argument1 eq $argument2); ##Ensure 1st and 2nd argument aren't the same
                next if ($argument1 =~ /^\S$/ or $argument2 =~ /^\S$/); ##Exclude if argument is only 1 character

                push(@matches, $chapternumber, $sentencenumber, $sentence, $grammar_relation, $argument1, $argument2);
                push @all_matches, \@matches;
            }
        }
        elsif ($category_id eq "xcomp") {
            my @lines = split ("\n",$sentblock); ##Split by a newline
            foreach my $line (@lines) {
                my @matches;
                next unless ($line =~ /\b$verbform\b/i); ##Ensure dependency contains searchword
                next unless ($line =~ /xcomp\w*\(|conj_and\w*\(|prep_by\w*\(|purpcl\w*\(/); ##Ensure dependency only contains desired grammar relations
                next unless ($line =~ /(\w+)\((\w+)\-\d+\,\s(\w+)\-\d+\)/); ##Ensure dependency is a dependency AND get info from it
                $grammar_relation = $1;
                $argument1 = $2;
                $argument2 = $3;

                next if ($argument1 eq $argument2); ##Ensure 1st and 2nd argument aren't the same
                next if ($grammar_relation =~ /xcomp/i and $argument2 !~ /\b$verbform\b/i); ##Ensure for xcomp the searchword is the 2nd argument
                next if ($argument1 =~ /^\S$/ or $argument2 =~ /^\S$/); ##Exclude if argument is only 1 character

                push(@matches, $chapternumber, $sentencenumber, $sentence, $grammar_relation, $argument1, $argument2);
                push @all_matches, \@matches;
            }
        }
        elsif ($category_id eq "obj" or $category_id eq "prep") {
            my @lines = split ("\n",$sentblock); ##Split by a newline
            foreach my $line (@lines) {
                my @matches;
                next unless ($line =~ /\b$verbform\b/i); ##Ensure dependency contains searchword
                next unless ($line =~ /$category_id\w*\(/); ##Ensure dependency only contains desired grammar relations
                next unless ($line =~ /(\w+)\((\w+)\-\d+\,\s(\w+)\-\d+\)/); ##Ensure dependency is a dependency AND get info from it
                $grammar_relation = $1;
                $argument1 = $2;
                $argument2 = $3;

                next if ($argument1 eq $argument2); ##Ensure 1st and 2nd argument aren't the same
                next if ($argument1 =~ /^\S$/ or $argument2 =~ /^\S$/); ##Exclude if argument is only 1 character

                push(@matches, $chapternumber, $sentencenumber, $sentence, $grammar_relation, $argument1, $argument2);
                push @all_matches, \@matches;
            }
        }
    }
}

#To make the if elsif into subroutine: Name:get_all_matches  Pass In: ($sentblock, $verbform, $chapternumber, $sentencenumber, $sentence) Return:  @allmatches (if needed, return reference)

And then to print:

# --- Loop through all Matches --- #

for my $arrayref (@all_matches) { 
    #for my $item (@$arrayref) { 
        print @$arrayref, "\n\n\n";
    #}
}
#Sort by what?? 1.grammar_relation 2.arguments 3.alphabetical
#But how to print heading only once? use hash?
# How to sort by frequency (ie. sort by the number sentences with the same grammar_relation AND arg1 AND arg2)

You can ignore the comments of the last one, or give me some bonus pointers from experience. Later, I have to sort the results. Would the sorting be done before that last step?

Here's the format of the input if you're really inclined on understanding the code:

Parsing file: chpt31_4.txt

Parsing [sent. 15 len. 9]: [These, animals, move, slowly, or, not, at, all, .]

(ROOT
(S
(NP (DT These) (NNS animals))
(VP (VBP move)
(ADVP (RB slowly)
(CC or)
(RB not))
(ADVP (IN at) (DT all)))
(. .)))

det(animals-2, These-1)
nsubj(move-3, animals-2)
advmod(move-3, slowly-4)
advmod(move-3, not-6)
conj_or(slowly-4, not-6)
advmod(move-3, at-7)
pobj(at-7, all-8)

\$\endgroup\$
2
\$\begingroup\$

You, sir, need a Strategy pattern -- at least to resolve your if->elseif problem. I find a Gang of Four style reference like http://www.blackwasp.co.uk/Strategy.aspx to be a little easier on the brain for initial consumption of a new pattern than the perl deep-dive, but YMMV.

\$\endgroup\$
2
  • \$\begingroup\$ This is really interesting stuff, it does seem like an algorithm would be helpful. However... I just started learning Perl, so haven't gotten to objects yet, so it may take a while. Thanks! \$\endgroup\$
    – Jon
    Jun 12 '11 at 12:57
  • \$\begingroup\$ Just to clarify, a 'Design Pattern' is how you organize an algorithm, they don't generally solve issues except those of clarity / code quality. Martin Fowler and Kent Beck have some excellent books on code quality, notably in the areas of Design Patterns and Refactoring. Glad to help :) \$\endgroup\$
    – gazarsgo
    Jun 12 '11 at 13:13
2
\$\begingroup\$

Fewer temp variables make code much more readable:

# --- Loop through all Matches --- #
print @{$_}, "\n\n\n" for @all_matches;

The output of map is an array. Learn to chain array-outputting functions into other array-outputting functions to further reduce temp var clutter:

# --- Different forms of the Searchword --- #
my @verbforms = (
                   $search_key,
                   (
                      grep { $_ ne '' }
                      map { changeVerbForm( $search_key, 0, $_ ) } (1..4);
                   )
                );

See the Schwartzian Transform for the most commonly used idiom of this idea.

Use || and && to reduce number of statements:

next if $arg1 eq $arg2 || $arg1 =~ /^\S$/ || $arg2 =~ /^\S$/;

I sense ternary operators may also service you well, but I'm out of time!

\$\endgroup\$
1
  • \$\begingroup\$ Does: my @verbforms = ( $search_key, map { changeVerbForm( $search_key, 0, $_ ) || (); } 1..4 ); do the same thing? It seems to work the same on the outside... Would having all the next statements in one line increase or decrease efficiency?? And wow, the ternary operator seems really powerful. Again is it less efficient than a standard if-else condition?? Thanks a lot for your time. \$\endgroup\$
    – Jon
    Jun 15 '11 at 21:01

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.