Re: Lady Aleena's first working module

You're using one of my pet-peeve style errors:

sub random_alignment {
  my $type = shift;
  if ($type eq 'parts') {
    return $parts[rand @parts];
  }
  elsif ($type eq 'good_vs_evil') {
    return $good_vs_evil[rand @good_vs_evil];
  }
  elsif ($type eq 'lawful_vs_chaotic') {
    return $lawful_vs_chaotic[rand @lawful_vs_chaotic];
  }
[download]

After a return there can never be an else. the sub returns, so this much better reads like this:

sub random_alignment
{
  my $type = shift;
  if ($type eq 'parts') {
    return $parts[rand @parts];
    }
  if ($type eq 'good_vs_evil') {
    return $good_vs_evil[rand @good_vs_evil];
    }
  if ($type eq 'lawful_vs_chaotic') {
    return $lawful_vs_chaotic[rand @lawful_vs_chaotic];
    }
[download]

And personally, I'd go even further:

sub random_alignment
{
  my $type = shift;
  $type eq "parts"             and return $parts[rand @parts];
  $type eq "good_vs_evil"      and return $good_vs_evil[rand @good_vs_
+evil];
  $type eq "lawful_vs_chaotic" and return $lawful_vs_chaotic[rand @law
+ful_vs_chaotic];
[download]

Clean, short code. Beautiful, right?

Enjoy, Have FUN! H.Merijn

Comment on Re: Lady Aleena's first working module Select or Download Code

Replies are listed 'Best First'.
Re^2: Lady Aleena's first working module by Herkum (Parson) on Oct 07, 2009 at 14:34 UTC
Now you are doing one of my pet peeves, using to much syntax which is not clarifying the code at all. This is better, `my $type = shift; return $type eq 'parts' ? $parts[ rand @parts] : $type eq 'good_vs_evil' ? $good_vs_evil[ rand @good_v +s_eval] : $type eq 'lawful_vs_chaotic' ? $lawful_vs_chaotic[ rand @lawful +_vs_chaotic] ? q{}` [download]	[reply] [d/l]
Re^3: Lady Aleena's first working module by Tux (Canon) on Oct 07, 2009 at 18:46 UTC
That kind of code will only work of there is just one return, and "" (or the hideous q{} from PBP) is the only possible alternative. My example just simplifies the pre-emptive returns, and lives in the assumption that the default (after all the returns were done) still needs some real coding. Personally, I like brevity, and both my code and yours are terse. I do not however think that your code is any clearer than mine. Both are clearer than the code of the OP though. Enjoy, Have FUN! H.Merijn	[reply]
Re^2: Lady Aleena's first working module by Lady_Aleena (Priest) on Oct 14, 2009 at 21:55 UTC
Tux, With what Herkum suggested in Re: Lady Aleena's first working module, using a hash instead of a lot of arrays, I was able to cut down on the redundant code a bit. You can see my changes in Re^2: Lady Aleena's first working module. I am a bit confused as to why you would put the closing bracket of the `if` on the next level down from it. I am of the mind that the closing bracket should be on the same level as where it was opened. I am also not a big fan of condensing the `if`, but that is because I don't want it to get lost. That is the same reason I concatenate over interpolate. *Have a nice day!* Lady Aleena	[reply] [d/l] [select]
Re^3: Lady Aleena's first working module by Tux (Canon) on Oct 15, 2009 at 06:20 UTC
In my humble but very honest opinion, indenting the closing brace to that level is the only logical thing to do. I have tried to explain that (and some other style issues) in here. I know many won't agree, but you have to admit I at least gave it a lot of thought and I'm not one of those people that just blindly follow what their favourite editor thinks to be the best style today. Enjoy, Have FUN! H.Merijn	[reply]


No such thing as a small change
	PerlMonks