There are a few things I see in here that I would suggest could be different.
I don't like that the logic for determining whether a cell is a sink is on both Rainfall and Cell. In fact, both classes have the method called is_sink...
My preference would be to move the logic on to the Cell, which already knows how to calculate it's neighbours.... and then the Rainfall class can just call:
push @sinks, $cell
if $cell->is_sink($self);
This indicates a larger problem though, that a fair amount of code is repeated (calculating neighbours, etc.). My suggestion is that you should have an initial pass of the field after creating each cell. This inialization pass should get each cell to compute, and store their neighbour list, as well as compute whether the cell is a sink. Putting it in the constructor of the Rainfall class seems like the right idea, but, since the initalization can also record the sinks, it seems quite a lot for a constructor. I am on the fence. The up-side is that it will make the execution faster.... Consider the RainFall constructor:
sub new {
my ($class, %attrs) = @_;
# initialize all elements of the matrix to cells O(n)
for my $i ( 0 .. @{ $attrs{field} } - 1) {
for my $j ( 0 .. @{ $attrs{field}->[$i] } - 1 ) {
$attrs{field}->[$i]->[$j] =
Cell->new( x => $i,
y => $j,
elevation => $attrs{field}->[$i]->[$j],
);
}
}
my @sinks;
for my $i ( 0 .. @{ $attrs{field} } - 1) {
for my $j ( 0 .. @{ $attrs{field}->[$i] } - 1 ) {
my $cell = $attrs{field}->[$i]->[$j];
$cell.initialize($self);
push @sinks if $cell->is_sink;
}
}
$attrs{sinks} = @sinks;
bless \%attrs, $class;
}
The initialize method on the Cell will calculate, and store, the array of neighbours. This will substantially reduce the number of times they need to be calculated.
If you have the one-off initialization then Cell->is_sink can take no parameters again.
Apart from the restructuring of the is_sink, and the persistence of the neighbours array, I have a nit-pick about some of your loop-conditions.... you often have code like:
for my $row ( 0 .. $self->rows - 1 ) {
..
You should rather be using the size-of operator rather than the scalar one:
for my $row ( 0 .. $#{$self->rows} ) {
Similarly with things like:
for my $i ( 0 .. @{ $attrs{field} } - 1) {
Should be:
for my $i ( 0 .. $#{ $attrs{field} }) {
one last nit-pick, why do the subtraction when >= works fine too:
next NEIGHBORS
if $xmod > $rows - 1 || $ymod > $cols - 1 || $xmod < 0 || $ymod < 0;
This could easily be:
next NEIGHBORS
if $xmod >= $rows || $ymod >= $cols || $xmod < 0 || $ymod < 0;
Although, again, it is unclear that $rows and $columns are arrays here, and I would prefer:
next NEIGHBORS
if $xmod > $#{$rows} || $ymod > $#{$cols} || $xmod < 0 || $ymod < 0;