in reply to 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

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]

      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]

      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]

In Section Meditations