Ööö... kritizálni lehet?
Minden változód globális. Ez egy rossz szokás, mert ugyan az elején egyszerűsíti a dolgokat, de ha ne adj' $isten elkezd fejlődni a programod, mondjuk többezer soros, tucatnyi modulból álló szörnyeteggé, akkor borzasztóan hátráltatni fog. Azt a változót, amire csak egy függvényben van szükség, a függvényben deklaráld. Tisztább, szárazabb, biztonságosabb érzés.
Ezenkívül mindent a szkript elején deklarálsz. Ez rontja az olvashatóságot: mindig, amikor látsz egy változónevet, akkor töprengened kell, hogy ez mi a szar és hogy került ide. Ha valamire csak pl. egy for cikluson belül van szükség, akkor ott deklaráld.
Aztán mindenütt C-stílusú for ciklusokat használsz:
for ($i=0;$i<$#old;$i++){Ezek helyett jobb (olvashatóbb, gyorsabb) a
for my $i (0..$#old)(A tied még hibás is, mert @old utolsó elemét kihagyja.)
Pár helyen feleslegesen bonyolítasz. Pl. a random_color függvényben nem kell a
if ($rand < 16) {
$hex[$x] = "0" . $hex[$x];
}
,
helyette elég a
sprintf "%02x", ....
Az is a baja annak a függvénynek, hogy egy globális tömböt buzerál, ahelyett, hogy simán visszaadná a generált számot, és a hívóra bízná, hogy mit kezd vele. Én így írnám:
sub random_color {
return join "", map {sprintf "%02x", rand 255} (1..3)';
}
A többit még nem néztem át, de első körben szerintem ennyi is elég...