lwicks has asked for the wisdom of the Perl Monks concerning the following question:

Hi All,

As part of my "get better with Perl" campaign, I have started running through cyber-dojo.com exercises when time permits. Tonight I took a run at the 100 doors exercise.

Doing it was good and educational "deliberate practise" for me. But I know form the office that code review is a great way to learn. So I turn to you oh PerlMonks community, please if you have some time and enthusiasm, please review my code (below) and tell me how you would improve it.

I am hoping to create code that is not "clever" rather "maintainable". So please review it in that light if you could (i.e. I'm not wanting to learn obfuscation).

Thanks!

doors.t 

use strict;
use warnings 'all';
use Test::More qw( no_plan );
require "doors.perl";
 
 # Initialise array of doors.
 my @doors = init_doors();
 is($doors[1], 'Closed', 'Door one should be closed');
 is($doors[100], 'Closed', 'Door 100 should be closed');
 
 toggle_door(\$doors[1]);
 is($doors[1], 'Open', 'Door one should be closed');
 toggle_door(\$doors[1]);
 is($doors[1], 'Closed', 'Door one should be open');
 
 
 my @answer = loop_through_doors();
 
 # Doors open are: 1, 4, 9, 16, 25, 36, 49, 64, 81, and 100
 is($answer[1], 'Open');
 is($answer[4], 'Open');
 is($answer[9], 'Open');
 is($answer[16], 'Open');
 is($answer[25], 'Open');
 is($answer[36], 'Open');
 is($answer[49], 'Open');
 is($answer[64], 'Open');
 is($answer[81], 'Open');
 is($answer[100], 'Open');
 
 # Check some of the other doors are closed.
 is($answer[2], 'Closed');
 is($answer[3], 'Closed');
 is($answer[50], 'Closed');
 is($answer[99], 'Closed');

[download]
doors.perl: 
use strict;
use warnings;
 
 sub init_doors
 {
   my @doors;
   for my $i (1 .. 100)
   {
     $doors[$i] = 'Closed';
   }
   return(@doors); 
 }
 
 sub toggle_door
 {
   my $door_ref = shift;
   if( $$door_ref eq 'Closed' )
   {
     $$door_ref = 'Open';
   } else {
     $$door_ref = 'Closed';
   }
   return $$door_ref; 
 }
 
 sub loop_through_doors
 {
  my @doors = init_doors();
   for my $pass (1 .. 100)
   {
     for my $visit (1 .. 100)
     {
       if (($visit % $pass) == 0)
       {
          toggle_door(\$doors[$visit]);
       }
     }
   }
 
   return(@doors); 
 }
 
 1;

[download]
You may also be able to look at it on Cyber-Dojo via the following link, I am not sure how long it remains live for however: Cyber-Dojo Link

Replies are listed 'Best First'.
Re: Please review my code: 100 Doors.
by choroba (Cardinal) on Jul 02, 2013 at 20:28 UTC
    Maybe a bit of DRY: 
    is($answer[$_ ** 2], 'Open') for 1 .. 10;

    [download]
    لսႽ† ᥲᥒ⚪⟊Ⴙᘓᖇ Ꮅᘓᖇ⎱ Ⴙᥲ𝇋ƙᘓᖇ
[reply]
[d/l]
Re: Please review my code: 100 Doors.
by AnomalousMonk (Archbishop) on Jul 02, 2013 at 23:17 UTC 
    sub toggle_door
  {
    my $door_ref = shift;
    if( $$door_ref eq 'Closed' )
    {
      $$door_ref = 'Open';
    } else {
      $$door_ref = 'Closed';
    }
    return $$door_ref; 
  }

    [download]

    My own preference is to avoid  if ... else ... or  if ... elsif ... else ... vipers' nests if possible — and if you add another state or states (e.g., 'HalfClosed'), you'll have to add one or more  elsif clauses. "But this application will never, ever require another state", you say. Famous last words.

    The following is my preferred approach to something like this. Most of the heavy lifting is actually data validation. (The state keyword requires Perl version 5.10+.) The following is tested per your test plan: 

    sub toggle_door {

  my ($door_ref) = @_;

  state $transition = { qw(Open Closed  Closed Open) };

  $$door_ref // die 'undefined door state';
  exists $transition->{$$door_ref}
    or die qq{unknown door state '$$door_ref'};

  return $$door_ref = $transition->{$$door_ref};

  }

    [download]

    At some sacrifice of self-documentation,  $_[0] can be used in place of  $$door_ref throughout the  toggle_door() function, in which case the initial
        my ($door_ref) = @_;
    statement is not needed, and, important note, the function is called with no reference taken, e.g., toggle_door($doors[$i]);(also tested, but I'm not sure I would actually use this): 

    sub toggle_door {

  state $transition = { qw(Open Closed  Closed Open) };

  $_[0] // die 'undefined door state';
  exists $transition->{$_[0]}
    or die qq{unknown door state '$_[0]'};

  return $_[0] = $transition->{$_[0]};

  }

    [download]
[reply]
[d/l]
[select]
      Thx, that is a great suggestion. Having worked mainly in 5.8 the newer functions are often not at the front of my mind.

      In terms of using $_[0] I agree on the "self-documentation" point so did have it with the extra step for readability later in mind.

      thanks again!

[reply]
Re: Please review my code: 100 Doors.
by Loops (Curate) on Jul 02, 2013 at 20:38 UTC

    init_doors could be simplified a bit. Even further except you are ignoring element 0 and starting your array use at 1 in other code

    sub init_doors()
{
        (undef, ('Closed') x 100);
}

    [download]
[reply]
[d/l]

Back to Seekers of Perl Wisdom