Beefy Boxes and Bandwidth Generously Provided by pair Networks
Don't ask to ask, just ask
 
PerlMonks  

Perl Cipher & questions on semantics/layout optimisation.

by Pseudomander (Novice)
on Feb 25, 2012 at 05:59 UTC ( #956063=perlquestion: print w/ replies, xml ) Need Help??
Pseudomander has asked for the wisdom of the Perl Monks concerning the following question:

Hello all, this is my first post here and in relation to my first Perl script. I've encountered no bugs however most recently having learnt python my presentation /methods aren't very perl reflective. I was wondering if people could give some pointers on: A. The generally accepted presentation. B. Any ways I could have coded this more effectively/effeciently.

##Authored by Troy Osborne (Pseudomander) 4:45am 25/02/2012 EST +10## ##Free to redistribute, use, copy, upload & adapt this work, all I ask + is if it in anyway represents the original version please give credi +t where due## $key=""; @karray= split(//,$key); $offsetpattern=""; $offsetpattern2=""; while (length($key) < 4) { print "Please Enter Your Desired Key (5 or more characters) \n --> + :"; $key=<>; } for ($i=0;$i<length($key);$i++) { $temp =0; for ($i2=0;$i2<length($key);$i2++) { $temp += (ord(@karray[i])+ord(@karray[i2])); $offsetpattern .= chr((ord(@karray[i])+ord(@karray[i2])+ o +rd($offset2[$i % length($key)]))%256); } $offsetpattern2.=chr(int($temp /length($key))%256); } @offset = split(//,$offsetpattern); @offset2 = split(//,$offsetpattern2); print "Encrypt or Decrypt (E/D)\n --> :"; $inp=<>; if (($inp=~/E/) or ($inp=~/e/)) #Encryption { print "Enter Plaintext\n --> :"; $plaintext = <>; $len = length($plaintext)-1; @array = split(//, $plaintext); for ($i=0;$i<$len;$i++) { $char = chr((ord(@array[$i]) - ord($offset[$i % length($key)*l +ength($key)])+ ord($offset2[$i % length($key)]))%256); print chr(ord($char)-($i%6)); } } elsif (($inp=~/D/) or ($inp=~/d/)) #Decryption { $a=""; print "Enter Cyphertext\n --> :"; $cyphertext = <>; $len = length($cyphertext)-1; @array = split(//, $cyphertext); for ($i=0;$i<$len;$i++) { $char = chr((ord(@array[$i]) + ord($offset[$i % length($key)*l +ength($key)]) - ord($offset2[$i % length($key)]))%256); print chr(ord($char)+($i%6)); } } $wait =<>;

Comment on Perl Cipher & questions on semantics/layout optimisation.
Download Code
Re: Perl Cipher & questions on semantics/layout optimisation.
by roboticus (Canon) on Feb 25, 2012 at 06:35 UTC

    Pseudomander:

    Actually, there are several bugs in your program. First, a simple test. Using the key 'ABCDEFG' and encrypt the string 'Now is the time for all good men to come'. Then decrypt the resulting string with the key 'ABCDEFG' -- that works just fine. Now let's demonstrate the bug: Try decrypting the result with key values 'XBCDEFG' or 'LMN'. It seems that the key you enter doesn't really do much.

    If you add use strict and use warnings, then after prefixing some of the variables with 'my', you'll find a few errors to fix--You're not always indexing your arrays with variables! So adding strict and warnings would be my first suggestion.

    When you get those errors out, you'll then see a page of warnings with your program output that should point you to the bug I noticed: You're not initializing @karray with your key string.

    While your indenting looks pretty reasonable, I wouldn't double-space the code so much. It just makes everything longer and a bit harder to read.

    ...roboticus

    When your only tool is a hammer, all problems look like your thumb.

Re: Perl Cipher & questions on semantics/layout optimisation.
by repellent (Priest) on Feb 25, 2012 at 07:43 UTC
    Hi, and welcome to Perlmonks! Here's my take on your code:

    Find for the following line. It's in the first inner for-loop.
    $offsetpattern .= chr((ord(@karray[i])+ord(@karray[i2])+ ord($offset2[ +$i % length($key)]))%256);

    Notice that you have the bare variables i and i2. They ought to be $i and $i2 instead (you forgot the $). Also, the array @offset2 is not defined yet at this code point. Plus, indexing into an array should be $karray[$i] instead of @karray[$i].

    Then, I notice that you call ord and chr a lot. You are joining characters only to split them later again, and that is (wasted) redundant work. In the case of the key array, you can start off by mapping the ordinals:
    my @karray = map ord, (split //, $key);

    That saves you the work of calling ord like ord($karray[$i2]) a whole bunch of times. This can also apply to your offset arrays. Keeping all your array elements as ordinals will make your code more concise; no need for intermediate strings.

    By the way:
    my @karray = map ord, (split //, $key); my $len = length($key); # prints true for both print "true" if $len == @karray; print "true" if $len-1 == $#karray;
Re: Perl Cipher & questions on semantics/layout optimisation.
by GrandFather (Cardinal) on Feb 25, 2012 at 08:01 UTC

    First off: you absolutely must use strictures (use strict; use warnings;). You have a large number of errors in your code that strictures would have picked up. Aside from that the code doesn't look very Perlish in places and looks fairly newbieish in other places. The following code may be of interest:

    use strict; use warnings; my $key = ""; while (length ($key) < 4) { print "Please Enter Your Desired Key (5 or more characters) \n --> + : "; $key = <>; chomp $key; } my @karray = split (//, $key); my @offset1; my @offset2; for my $ordChar1 (map {ord} @karray) { my $sum = 0; for my $ordChar2 (map {ord} @karray) { $sum += $ordChar1 + ord $ordChar2; push @offset1, ($ordChar1 + $ordChar2) % 256; } push @offset2, int ($sum / length $key) % 256; } 1 while prompt (); sub prompt { my ($mode, $offset1, $offset2) = @_; print "Encrypt or Decrypt (E/D)\n --> :"; my $inp = <>; chomp $inp; return 1 if $inp !~ s/^(e|d)$/lc $1/ie; print "Enter ", ($inp eq 'e' ? 'plain text' : 'cypher text'), "\n +--> : "; chomp (my $text = <>); my @chars = split (//, $text); if ($inp eq 'e') { print encrypt(\@chars, \@offset1, \@offset2); } else { print decrypt(\@chars, \@offset1, \@offset2); } return; } sub encrypt { my ($chars, $offset1, $offset2) = @_; my $outText; my $len = @$chars; for my $index (0 .. $len - 1) { $outText .= chr (( ord ($chars->[$index]) - ord ($offset1->[$index % $len * $len]) + ord ($offset2->[$index % $len]) ) % 256 - $index % 6); } return $outText; } sub decrypt { my ($chars, $offset1, $offset2) = @_; my $outText; my $len = @$chars; for my $index (0 .. $len - 1) { $outText .= chr (( ord ($chars->[$index]) + ord ($offset1->[$index % $len * $len]) - ord ($offset2->[$index % $len]) ) % 256 + $index % 6); } return $outText; }
    True laziness is hard work
      First off: you absolutely must use strictures (use strict; use warnings;).
      GMAFB. You "absolutely must" type "use strict;\nuse warnings;\n\n" at the start of your posts to avoid a scolding by GrandFather and his pathetic ilk. Sometimes these things catch bugs; sometimes they don't. But they're always good for cheap upvotes.

        I often give the "use strictures" advice because it gives the greatest bang for the buck in terms of catching the sorts of mistakes newbie (and other) programmers make. In that respect it is cheap advice of great value.

        In the case of the reply to the OP I strengthened my usual "should" to "absolutely must" because strictures would have caught several different types of error in the OP's code. A cursory test of the original code would seem to show it worked. However due to bare word errors and various other types of error Perl glosses over by default the code was not doing anything like what the OP expected.

        I'm not sure why you are fixated on "cheap upvotes". If that were the only advice I ever gave maybe there would be some basis for the implied accusation, but without much work you could find out for yourself that that doesn't apply to me, or indeed to any other monk I can think of.

        I sure as an experienced and competent programmer you have no need for help from the dumb computer, but for me tools such as strictures, a good IDE and unit tests make me an order of magnitude more productive with much greater confidence in my work. I shall continue to advise newbies (and anyone else who will listen) to use stictures and any other such tool that make their life easier and improves the quality of their code.

        True laziness is hard work
Re: Perl Cipher & questions on semantics/layout optimisation.
by oko1 (Deacon) on Feb 25, 2012 at 20:51 UTC

    Hello, and welcome! It's always good to see someone bravely exposing their code to the merciless scrutiny... ahem. :) No, seriously - this is my favorite (and most effective, I think) way to learn; try out what you've come up with, and ask the experts for their comments. Might sting a bit at first, but you get a _big_ jump on knowledge.

    Having said that, I'll start ripping off the Band-Aids. :) I'll remove some of the excessive doublespacing to make it a bit more readable.

    $key=""; @karray= split(//,$key);

    The above code actually does nothing useful. Since '$key' is empty, '@karray' will also be empty.

    while (length($key) < 4) { print "Please Enter Your Desired Key (5 or more characters) \n --> + :"; $key=<>; }

    Entering a 3-char long key will exit this loop. Since you're not using the chomp function, the key will always end in '\n' - and thus be 1 char longer than the number of characters you enter, which will pass your 'while()' test. This disagrees with your prompt. This also applies to all of your following user input routines.

    for ($i=0;$i<length($key);$i++) { $temp = 0; for ($i2=0;$i2<length($key);$i2++) { $temp += (ord(@karray[$i])+ord(@karray[$i2])); $offsetpattern .= chr((ord(@karray[$i])+ord(@k +array[$i2])+ ord($offset2[$i % length($key)]))%256);

    Since you don't have a variable called '@offset2', and '@karray' is empty, your code is fatally broken, and the rest of the it will not do what you expect it to do - so the rest of my comments will pertain to the general issues in your script rather than specifics.

    As someone has already noted, your script isn't very "Perlish". This means more than you might think: the "Perlish" usages take effective advantage of the language's features, which gains more and more benefit as the script goes on. For example, you traverse arrays by getting the length of a string, decrementing that length, then laboriously counting (possibly incorrectly) from the first character to the last, and using that counter value to index into the array that you create by splitting the string:

    $cyphertext = <>; $len = length($cyphertext)-1; @array = split(//, $cyphertext); for ($i=0;$i<$len;$i++) { $char = chr((ord(@array[$i])...

    The Perlish way of getting both the index and the element would be more like this:

    chomp(my $blah = <STDIN>); # This should be validated... my @input = split //, $blah; for my $index (0 .. $#input){ print "$index: $input[$index]\n"; }

    Clean and simple. Just as another general comment, the three-term 'for' loop is fairly rare in Perl: there are a few uses for it, but it's usually not needed. It's much easier - and much more sensible, usually - to count things (that's what we humans usually do) than indices of things, which is a bit more abstract and requires an extra level of thought (and is thus a potential source of errors.)

    for (my $i = 1; $i < 10; $i++){ print "$i\n"; # Ooops - only counted to 9! } for my $i (1..10){ print "$i\n"; # Does the right and obvious thing } print "$_\n" for 1..10; # Same thing, even more Perlish

    Recommendation: grab a copy of Joseph Hall's "Effective Perl Programming". Good reading, including for beginners. Again, good on ya for asking for criticism so early on!

    -- 
    I hate storms, but calms undermine my spirits.
     -- Bernard Moitessier, "The Long Way"
Re: Perl Cipher & questions on semantics/layout optimisation.
by Pseudomander (Novice) on Feb 26, 2012 at 07:23 UTC

    Thank you all for your assistance, from the suggestions I've gotten from here and over on the Perl board at HackForums I've made a few updates. I'm yet to include structures but have practised using them as evident in a poem I'm about to upload. I think I'm going to enjoy it here. Very quick, detailed responses and wonderfully helpful insight.

    ##Authored by Troy Osborne (Pseudomander) 4:45am 25/02/2012 AEST +10 # +# ##Free to redistribute, use, copy, upload & adapt this work, all I ask + is if it in anyway represents the original version please give credi +t where due## use warnings; $key=""; $offsetpattern=""; $offsetpattern2=""; while (length($key) < 4) { print "Please Enter Your Desired Key (5 or more characters) \n --> + :"; chomp($key=<>); } @karray= split(//,$key); for ($i=0;$i<length($key);$i++) { $temp =0; for ($i2=0;$i2<length($key);$i2++) { $temp += (ord($karray[$i])+ord($karray[$i2])); $offsetpattern .= chr((ord($karray[$i])+ord($karray[$i2])) +%256); } $offsetpattern2.=chr(int($temp /length($key))%256); } @offset = split(//,$offsetpattern); @offset2 = split(//,$offsetpattern2); print "Encrypt or Decrypt (E/D)\n --> :"; $inp=<>; if (($inp=~/E/) or ($inp=~/e/)) #Encryption { print "Enter Plaintext\n --> :"; chomp($plaintext = <>); $len = length($plaintext)-1; @array = split(//, $plaintext); for ($i=0;$i<$len;$i++) { $char = chr((ord($array[$i]) - ord($offset[$i % length($key)*l +ength($key)])+ ord($offset2[$i % length($key)]))%256); print chr(ord($char)-($i%6)); } } elsif (($inp=~/D/) or ($inp=~/d/)) #Decryption { print "Enter Cyphertext\n --> :"; chomp($cyphertext = <>); $len = length($cyphertext)-1; @array = split(//, $cyphertext); for ($i=0;$i<$len;$i++) { $char = chr((ord($array[$i]) + ord($offset[$i % length($key)*l +ength($key)]) - ord($offset2[$i % length($key)]))%256); print chr(ord($char)+($i%6)); } } <>;

      You asked for guidance concerning presentation and effective/efficient coding. You were given a large stack of replies, most of which you seem to have ignored. You have fixed the bugs we pointed out (even though you thought you had none), but seem not to have made a single style change of the many suggested.

      We are generally keen to help, but become less keen when ignored. Re-read roboticus's, repellent's, oko1's and my replies again to get the details. A list of key points that you have ignored would include:

      • roboticus (and others) use strictures
      • roboticus don't double space your code (this may be a foible of copy and paste, but fix it)
      • repellent avoid redundant work
      • oko1 use Perl - especially Perl's for loops
      • GrandFather refactor your code to avoid large chunks of duplicated code

      Most of those replies touched on most of those points either directly or indirectly. As you didn't seem to pick up on them first time round you should really go back and read them in detail until you understand what they are saying. Each respondent will be eager to explain further anything that seems unclear if you show an interest.

      True laziness is hard work

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://956063]
Approved by GrandFather
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others wandering the Monastery: (6)
As of 2014-08-30 12:34 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    The best computer themed movie is:











    Results (293 votes), past polls