http://qs1969.pair.com?node_id=154237

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

Good day, sisters and brothers. I'm in need of some quality feedback on a new module I'm working on. It hasn't been relased on CPAN yet, and is still what I would consider early alpha code. I'd like any constructive input you would care to provide, including a name for the module. For the moment I'm calling it Crypt::Playwright, though I'm not necessarily wed to that name. The module is mildly funny and moderately useful. It does actually perform a somewhat useful function, so I wasn't sure it was appropriate to place it under the Acme:: family of modules.

At any rate, the code (which is approx 350 lines) appears in my follow-up to this post. Kindly post reponses to this, the parent node, rather than replying to the code post, which follows.

Thanks much.
___________________
Kurt

Replies are listed 'Best First'.
(Ovid) Re: Requesting feedback
by Ovid (Cardinal) on Mar 25, 2002 at 22:18 UTC

    sifukurt wrote (in the POD) "It does offer a degree of security".

    Actually, I would tend to disagree as it's a simple substitution cipher. Anyone trying to read the output is going to realize that it's terribly random and, if they want to know what it contains, will quickly decipher it. Steganography is an information hiding technique that works best when combined with true encryption. Make your module inherit from another, have the user supply a secret (or work out a public key system?), encrypt the data and then run your module over it. If someone realizes that the "play" is hiding something, they'll still have the encryption hurdle to climb. Alternately, give the user the choice of encryption system, keylength, and maybe the choice of symmetric or public keys. Until then, this should be in the Acme:: namespace.

    Fun module, though :)

    Side notes: You require Exporter but don't use it.

    Too many globals will make it tough to maintain.

    Abstract out "magic" variables. For example:

    if ( $name =~ /^\[/ && ! $val ) {

    It's not immediately clear that stage directions indicate a newline. You have to search for this snippet to understand:

    sub StageDirections { return "[ " . RandomElement( \@Names ) . " " . RandomElement( \@Ac +tions ) . ". ]\n"; }

    Take out the "[" and replace it with something from this:

    my %stage_direction_delimiter = ( left => '[', right => ']' );

    I realize that this variable name is too long, but you get the idea. If someone wants to "tweak" things, they'll thank you!

    Cheers,
    Ovid

    Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

      Hmmm...good point. Don't get me wrong, I'm not saying this is strong crypto by any stretch of the imagination. It does, however, have more than a one-to-one substitution for the commonly used characters. For example, if "r" comes up as the first of the two characters it grabs, it chooses from 10 different names. For less frequently used letters, say "x" for example, it is a one-to-one substitution. At first I was thinking the Acme:: name space, but it does actually provide a somewhat higher degree of security than Crypt::Rot13, for example. Since Rot13 is a true one-to-one subsitution cipher, I suppose a better argument could be made for moving Rot13 to the Acme:: namespace than for keeping Playwright under the Crypt:: namespace, though.
      ___________________
      Kurt
        The difference is that Crypt::Rot13 has a useful purpose (in usenet etc) and that your module, while interesting, isn't really any use in production code.

        gav^

Re: Requesting feedback
by fuzzyping (Chaplain) on Mar 25, 2002 at 23:38 UTC
    I give it a 9.3 on the fun scale... a 10.0 on the I'm-sick-cuz-I-have-entirely-too-much-time-on-my-hands scale. I might be doing something wrong with the package, but I thought you might be interested in the following errors I tripped across...
    [jason@dnsops jason]$ ./play.pl Input string: Hello World Use of uninitialized value in string eq at Crypt/Playwright.pm line 31 +2, <STDIN> line 1. Use of uninitialized value in string eq at Crypt/Playwright.pm line 31 +2, <STDIN> line 1. Kurt: It is much heavier than it looks. Emilia: Do you think anyone will notice? Jane: Have you ever seen one of these? Karen: Well it looks like...I don't know what it looks like. Zeus: Do you think anyone will notice? Dan: It looks sort of squashed. [ Sheri writes a poem. ]

    According to my search, here is the code from line 312:
    if ( $chars[ $i + 1 ] eq "\n" || $chars[ $i + 1 ] eq "" ) { $out .= $t +emp . " " . RandomElement( \@pSpace ) . "\n" . StageDirections();
    -fuzzyping
      I found the same error. There is an "off by 1" problem when the input string does not end with a "\n".
      The counting variable $i is incremented by 2 at each loop, and decreased by one when a "\n" is met.
      Using "Hello world\n", it will work.


      cchampion
Re: Requesting feedback
by sifukurt (Hermit) on Mar 25, 2002 at 21:53 UTC
    Ok, if you've come this far, you must be interested in seeing the code. Here it is: Please post replies to the parent node.

    Thanks for your time and input.
    ___________________
    Kurt
Re: Requesting feedback
by dug (Chaplain) on Mar 25, 2002 at 22:14 UTC