in reply to mp3 collection renamer

Time for code review ;)
########################################################### ### not gonna complain about strict, for whatever reason use MP3::Info; die "Must use parameters and seperator\n (SEPERATOR ARTIST TITLE YEAR +ALBUM GENRE COMMENT)" unless @ARGV; $sep=shift; @tokens=@ARGV; while(<*.mp3>) # while is wrong as you move files around, # should be for (grep /\.mp3$/i, <*>) # to insure getting files like "bla.MP3" ### indent your code! { my $tag=get_mp3tag($_) or (push(@errors,"No tag for $_\n") and nex +t); $nn=''; ### now you're using <space><sep><space> as a separator ### I wanted <sep> only. for $i (@tokens){$nn.=$tag->{$i}." $sep ";} my $z=rindex($nn,$sep); my $len=length $sep; substr($nn,$z,$len+1,''); chop $nn; $nn.='.mp3'; ### the last 6 lines should've been something like: ### $nn = join($sep, map $tag->{$_}, @tokens) . ".mp3"; ### and this is not my personal preferrence, but a more ### readable, straight to the point, honest to god statement print "Renamed $_ to $nn \n" if rename $_,$nn; ### where is my error check? } print "\n\n"; print @errors;
And one more final thing which is more important than all of the above blabbering: do "perl name.pl '-' ARTIST" and wonder why all your artists suddenly have one song each.
Or worse: Watch all your mp3s reduced to a singleton ".mp3" as you forget to pass args other than the sep!!.

Hope this helps!!