Problem with passing array to subroutine

[Ben Bullock]

Can anyone explain what I'm doing wrong in the following? The output is
something like this:

First try:
1 X 1: A
1 X 2: B
1 X 3: C
2 X 1: D
2 X 2: E
2 X 3: F
3 X 1: G
3 X 2: H
3 X 3: I
Try again:
1 X 1: A
1 X 2: B
1 X 3: C
2 X 1: D
2 X 2: E
2 X 3: F
3 X 1: G
3 X 2: H
3 X 3: I
Now what happens:
Total size 3 X 3
1 X 1: Not defined
1 X 2: Not defined
1 X 3: Not defined
2 X 1: Not defined
2 X 2: Not defined
2 X 3: Not defined
3 X 1: Not defined
3 X 2: Not defined
3 X 3: Not defined

<<<<<<<<<<<<<<<<<<<<<<<<<

I don't know why I get the "Not defined" in the last part. I think it's an
error in calling the subroutine but I'm not sure what I should do to get
the correct syntax.

The following is the shortest I could make my sample program. This is a
complete running example.

============================

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

sub read_table
{
my $input_text = $_[0]; # assumes the table text is in the first argument
$input_text =~ m/\{\|(.*?)\|\}/s;
my @rows;
my $table_text = $1;
my @row_text = split (/\|\-/s, $table_text);
my $row_number = 0;
my $n_columns = 0;
my $column_number;
my %cell;
foreach my $rt (@row_text) {
# print "rt='$rt'\n";
$column_number = 0;
$row_number++;
chomp $rt;
while ($rt =~ s/\|(.*)//) {
$column_number++;
$cell{"text"} = $1;
# print "text = '$1' rt = '$rt'";
$rows[$row_number][$column_number] = { %cell };
}
if ($column_number > $n_columns) {
$n_columns = $column_number;
}
}
$rows[0][0] = [$row_number, $n_columns];
for (my $x = 1; $x <= $row_number; $x++) {
for (my $y = 1; $y <= $n_columns; $y++) {
print "$x X $y: ";
my $cell_ref = $rows[$x][$y];
if ($cell_ref) {
my %cell = %$cell_ref;
print $cell{"text"};
} else {
print "Not defined";
}
print "\n";
}
}
return @rows;
}

sub print_table
{
my @rows = $_[0];
my $dim = $rows[0][0];
if (! $dim) {
print "No dimensions in table.\n";
return;
}
my @dimensions = @$dim;
my $row_number = $dimensions[0];
my $n_columns = $dimensions[1];
print "Total size $row_number X $ n_columns\n";
for (my $x = 1; $x <= $row_number; $x++) {
for (my $y = 1; $y <= $n_columns; $y++) {
print "$x X $y: ";
my $cell_ref = $rows[$x][$y];
if ($cell_ref) {
my %cell = %$cell_ref;
print $cell{"text"};
} else {
print "Not defined";
}
print "\n";
}
}
}

my $text = <<ENDTEXT;
{|
|A
|B
|C
|-
|D
|E
|F
|-
|G
|H
|I
|}
ENDTEXT

print "First try:\n";
my @table = read_table ($text);
print "Try again:\n";
for (my $x = 1; $x <= 3; $x++) {
for (my $y = 1; $y <= 3; $y++) {
print "$x X $y: ";
my $cell_ref = $table[$x][$y];
if ($cell_ref) {
my %cell = %$cell_ref;
print $cell{"text"};
} else {
print "Not defined";
}
print "\n";
}
}
print "Now what happens:\n";
print_table (@table);

 

[xhoster]

Can anyone explain what I'm doing wrong in the following?

Thanks for posting a working program. But with a script this large (and
ugly), you should have provided a little more exposition about what purpose
each part served.

Anyway,

....

sub print_table
{
my @rows = $_[0];

since you pass in the whole array, you need to capture the whole
array, not just the first element of it, in @rows:

my @rows = @_;
....

} ....

print_table (@table);

Xho

 

[David Squire]

Can anyone explain what I'm doing wrong in the following?
[snip]

I don't know why I get the "Not defined" in the last part. I think it's an
error in calling the subroutine but I'm not sure what I should do to get
the correct syntax.

Correct. You are passing a list of values to print_table, which is then
trying to assign the first element of that list to an array. Focussing
just on the quickest fix I could come up with for your code:

[snip]

sub print_table
{
my @rows = $_[0];

Change to:

sub print_table
{
my $table_ref = shift;
my @rows = @{$table_ref};

[snip]

print_table (@table);

Change to:

print_table (\@table);

Moral: it's always a good idea to pass *references* to large and complex
data structures.

DS

 

[xhoster]

Change to:

sub print_table
{
my $table_ref = shift;
my @rows = @{$table_ref};

[snip]

print_table (@table);

Change to:

print_table (\@table);

Moral: it's always a good idea to pass *references* to large and complex
data structures.

Since you are just copying the whole thing anyway with the
my @rows = @{$table_ref}, what is the point of passing the reference
rather than passing the array itself (and using my @rows=@_)?

Xho

 

[Brian Wakem]

Can anyone explain what I'm doing wrong in the following? The output is
something like this:

First try:
1 X 1: A
1 X 2: B
1 X 3: C
2 X 1: D
2 X 2: E
2 X 3: F
3 X 1: G
3 X 2: H
3 X 3: I
Try again:
1 X 1: A
1 X 2: B
1 X 3: C
2 X 1: D
2 X 2: E
2 X 3: F
3 X 1: G
3 X 2: H
3 X 3: I
Now what happens:
Total size 3 X 3
1 X 1: Not defined
1 X 2: Not defined
1 X 3: Not defined
2 X 1: Not defined
2 X 2: Not defined
2 X 3: Not defined
3 X 1: Not defined
3 X 2: Not defined
3 X 3: Not defined

<<<<<<<<<<<<<<<<<<<<<<<<<

I don't know why I get the "Not defined" in the last part. I think it's an
error in calling the subroutine but I'm not sure what I should do to get
the correct syntax.

The following is the shortest I could make my sample program. This is a
complete running example.

============================

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

sub read_table
{
my $input_text = $_[0]; # assumes the table text is in the first
argument $input_text =~ m/\{\|(.*?)\|\}/s;
my @rows;
my $table_text = $1;
my @row_text = split (/\|\-/s, $table_text);
my $row_number = 0;
my $n_columns = 0;
my $column_number;
my %cell;
foreach my $rt (@row_text) {
# print "rt='$rt'\n";
$column_number = 0;
$row_number++;
chomp $rt;
while ($rt =~ s/\|(.*)//) {
$column_number++;
$cell{"text"} = $1;
# print "text = '$1' rt = '$rt'";
$rows[$row_number][$column_number] = { %cell };
}
if ($column_number > $n_columns) {
$n_columns = $column_number;
}
}
$rows[0][0] = [$row_number, $n_columns];
for (my $x = 1; $x <= $row_number; $x++) {
for (my $y = 1; $y <= $n_columns; $y++) {
print "$x X $y: ";
my $cell_ref = $rows[$x][$y];
if ($cell_ref) {
my %cell = %$cell_ref;
print $cell{"text"};
} else {
print "Not defined";
}
print "\n";
}
}
return @rows;
}

sub print_table
{
my @rows = $_[0];

my @rows = @_;

 

[David Squire]

Can anyone explain what I'm doing wrong in the following?

[snip]

sub print_table
{
my @rows = $_[0];

Change to:

sub print_table
{
my $table_ref = shift;
my @rows = @{$table_ref};

[snip]

print_table (@table);

Change to:

print_table (\@table);

Moral: it's always a good idea to pass *references* to large and complex
data structures.

.... and of course it would then be sensible to keep using $table_ref,
dereferencing it when needed, instead of copying the whole thing to
@rows, particularly if the table is large, e.g.

my $dim = $rows[0][0];

would become:

my $dim = $$table_ref[0][0];

or perhaps more clearly:

my $dim = $table_ref->[0][0];

DS

 

[David Squire]

Change to:

sub print_table
{
my $table_ref = shift;
my @rows = @{$table_ref};

[snip]

print_table (@table);

Change to:

print_table (\@table);

Moral: it's always a good idea to pass *references* to large and complex
data structures.

Since you are just copying the whole thing anyway with the
my @rows = @{$table_ref}, what is the point of passing the reference
rather than passing the array itself (and using my @rows=@_)?

True... as I address in my own follow-up to my own post (written
while you were writing this). I am in the habit of passing references in
that situation, and it was a way of doing that, and then making the OP's
code work with minimal changes.

DS

 

[Tad McClellan]

#!/usr/bin/perl -w

use warnings; # is much better than the -w switch, because it is scoped

$input_text =~ m/\{\|(.*?)\|\}/s;
my @rows;
my $table_text = $1;

You should never use the dollar-digit variables unless you have
first ensured that the match *succeeded*, else they will contain
stale values from some previous match that did succeed.

die "'$input_text' did not match" unless $input_text =~ m/\{\|(.*?)\|\}/s;
my $table_text = $1;

my @row_text = split (/\|\-/s, $table_text);

The m//s modifier changes the meaning of dot, it is useless
when there is no dot in your regex.

Hyphen is not special in regexes, so there is no need to escape it.

while ($rt =~ s/\|(.*)//) {

Q: How many times will that loop iterate?

A: One.

It is a loop that does not loop!

So then, why is it in a loop?


if ($rt =~ s/\|(.*)//) {

 

[Ben Bullock]

use warnings; # is much better than the -w switch, because it is scoped

Thanks for that tip.

You should never use the dollar-digit variables unless you have
first ensured that the match *succeeded*, else they will contain
stale values from some previous match that did succeed.

In fact, in the original program, I did that; this is a quick running
example to demonstrate only one problem I had. I deliberately removed lots
of testing of input and various conditionals in order to make the program
short enough to be postable.

die "'$input_text' did not match" unless $input_text =~
m/\{\|(.*?)\|\}/s;
my $table_text = $1;




The m//s modifier changes the meaning of dot, it is useless
when there is no dot in your regex.

In this case, again, I simplified the regexp to a "minimal" version and
forgot to remove the s.

Hyphen is not special in regexes, so there is no need to escape it.

Thanks for that tip.

Q: How many times will that loop iterate?

A: One.

A: Three? Or else why does it print three results?

It is a loop that does not loop!

No, it is a loop which does indeed loop. Try replacing the above with

$rt =~ s/\|(.*)//;

in my original code (also removing the end } of the "while") and you'll see
that the output results are different.

Thanks for the other code critiques too. But I feel like I have wasted your
time somewhat; the above example program is a stripped-down-for-posting
version of a much longer program, with all kinds of extra conditionals and
odd bits, which is why there is some possibly redundant or odd-looking code
in there. I should have said that more clearly to save your time.

Anyway, as a meta-query, would it be better to post the whole code of my
program or to post this kind of minimal script? The posting guidelines
(which everyone seems so keen on) currently suggest posting the minimal
item.

 

[Ben Bullock]

Ben Bullock wrote:
Change to:

sub print_table
{
my $table_ref = shift;
my @rows = @{$table_ref};

[snip]

print_table (@table);

Change to:

print_table (\@table);

Moral: it's always a good idea to pass *references* to large and complex
data structures.

Thank you. This is exactly what I wanted to know. I was trying to pass the
reference but I couldn't get the syntax right.

 

[Ben Bullock]

Thanks for posting a working program. But with a script this large (and
ugly), you should have provided a little more exposition about what
purpose
each part served.

And I thought my humble script would be so obvious to the Perl gurus...

Anyway,

sub print_table
{
my @rows = $_[0];

since you pass in the whole array, you need to capture the whole
array, not just the first element of it, in @rows:

my @rows = @_;

Thanks very much. I was trying to pass in a reference, but I couldn't get
the syntax right.

 

[Tad McClellan]

A: Three? Or else why does it print three results?

Doh! I failed my own test!

Sorry.

Anyway, as a meta-query, would it be better to post the whole code of my
program

No.


or to post this kind of minimal script?

It is better to post a minimal script.