Beefy Boxes and Bandwidth Generously Provided by pair Networks
We don't bite newbies here... much
 
PerlMonks  

comment on

( [id://3333]=superdoc: print w/replies, xml ) Need Help??
Y'know, until now, I never noticed that "d/l code" anchor right below the "comment on this" anchor. The things you learn every day...

Suggestions subroutine by subroutine...

getmatrix: Pre-defining $input to 1 to prime the while, then checking for valid input at the bottom after you have read it is a little hard to understand. You could accomplish the same thing by making the read from <STDIN> as part of the while, with <while ($input = <STDIN>). That would allow you to eliminate the $input &&... at the end as well.

Going one step further, every use of $input at that point is in locations that default to $_, so you can eliminate it all together. I'd recommend, for clarity sake, also renaming @input to @matrix, since that's what it is.

sub getmatrix { my $what = shift; my @matrix; while (<STDIN>) { chop; if (/^0-9 ]/) die "Not a legal matrix row!\n"; } push @matrix, split / /; } return @matrix; }
checkmatrix: This is short enough that there really isn't much to suggest for improvement. The one thing I would suggest, though is moving the $rowlength assignment out of the loop. First, for clarity, second for efficiency. That and get rid of @row, it's unnecessary.

And I just noticed... ne is for string comparison, not numeric. Since you are dealing with numbers, use !=.

sub checkmatrix { my $rowlength = @$_[0]; for (@_) { $rowlength == @$_ or die "Number of columns was not consistant!\n" +; } }
As a matter of style, I'd have this return some boolean value rather than simply up and dying. It allows the caller to decide what to do with a broken matrix.

printmatrix: If that's how you want to format matrices, good enough for me. Not much to criticize here. (This included for completeness).

getdimen: This is really not clear what you are doing. Obviously, you wanted to get the number of rows and columns, but it seems a round-about way to get them. Especially the for loop with an unguarded last, meaning the loop will execute only once. Not clear at all. I'll do a total rewrite:

sub getdimen { my $rows = @_; # get number of rows my $cols = @$_[0]; # get number of elems in first row return ($rows,$columns); }
getproduct: This is the heart of your routine. The major thing I'll mention here is that you can do something like $product[$i][$j] += $a[$i][$k]*$b[$k][$j]; directly, and perl will DWIM correctly. You don't even need to pre-initialize @product. Using that would make your matrix multiply much simpler to understand. This also eliminates most of your "temp" variables.

The other thing I'd do is make product responsible for validating it's arguments and extracting the info it needs directly, rather than expect it to be called in.

sub getproduct { my ($aref, $bref) = @_; my @a = @$aref; my @b = @$bref; my @product; checkmatrix(@a); checkmatrix(@b); my ($arow, $acol) = getdimen(@a); my ($brow, $bcol) = getdimen(@b); $acol == $brow or die "The matrices can't be multiplied!\n"; for $i (0..$arow) { for $j (0..$bcol) { for $k (0..$acol) { $product[$i][$j] += $a[$i][$k] * $b[$k][$j]; } } } return @product; }
Now, if I got the multiplication wrong, it's easy to see what my mistake was (I haven't done linear algebra in a while, I might have the rows/columns mixed up, for instance).

Perl is very good at having the features that DWIM when you need them to. Trying to go overboard with pushes, temp arrays, etc are not necessarily the way to go. As a general rule, I'd say that if you have to force it to do what you want, there might be an easier way...

Hope this helps...


In reply to RE: Matrix Multiplier by BlaisePascal
in thread Matrix Multiplier by zdog

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post; it's "PerlMonks-approved HTML":



  • Are you posting in the right place? Check out Where do I post X? to know for sure.
  • Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
    <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>
  • Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
  • Want more info? How to link or How to display code and escape characters are good places to start.
Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others scrutinizing the Monastery: (6)
As of 2024-04-20 02:30 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found