5
\$\begingroup\$

I have recently gotten back into development and I am wondering if the script I have just created is clearly documented and easily understandable throughout each step. Is it easy to understand?

class Encryption {

   public function Encrypt ($String){
        /*
         *  Create an Encrypted String.
         *  @Var string
         *
         */
        $Size = mcrypt_get_iv_size(MCRYPT_CAST_256,MCRYPT_MODE_CBC);
        $iv = mcrypt_create_iv($Size,MCRYPT_RAND);
        $Hash = strlen($String);
        return
            array(
                "EncryptedString" => openssl_encrypt($String, "AES-256-CBC",$Hash,0,$iv),
                "IV" => $iv,
                "Hash" => $Hash
            );
    }

   public function Merge($Array){
        /*
         *  Merge The Array Returned From the String Encryption into a string
         *
         *      Information:
         *          1) Convert Each Element from the array into their personal variables
         *          2) Create an Array from the first element. Will be used at the later stages
         *          3) Get the length of the current IV passed
         *          4) Create an empty string to be manipulated using the attaching loop
         *
         */
     #  $Array = $this->Encrypt($String);
        $Encrypted_String = $Array['EncryptedString'];
        $IV = $Array['IV'];
        $Hash = $Array['Hash'];
        $Hash_Count = count(str_split($Hash));
        $EncStr_Arr = str_split($Encrypted_String);
        $Count = strlen($IV);
        $Increment = 0;
        $String = "";
            // NEW: Appending the Hash Count, to be used in later functions. So The correct Hash can be obtained.
        $String .= $Hash_Count.$Count.$Hash; // Append the Count and Hash (as EncryptedString and IV are different lengths. Used to compensate
        While ($Increment < $Count){
            /*
             *  This loop appends to the string created in the order:
             *  Even: IV Character
             *  Odd: EncryptedString Character
             *
             *  Unset Elements of the Array for each iteration
             */
            $String .= $IV[$Increment];
            $String .= $Encrypted_String[$Increment];
            unset($EncStr_Arr[$Increment]);
            $Increment++;
        }
        /*
         *  After The loop has broken (Increment reaches the count of the IV), implode the Remainder elements (encrypted string)
         *  and return string + imploded array
         */

        $Encrypted_String = implode("",$EncStr_Arr);
        return $String.$Encrypted_String;
    }


   public function UnMerge($String){
        /*
         *  function splits a string into 3 elements of an array to a usable format for the decryption
         *      1) Split the String into an Array, 1 character = 1 element
         *      2) The hash count is obtained (pushed to first character in the merge)
         *      3) Unset the Hash Number
         *      4) IV length is stored and created into the 2 digit number to be used later
         *      5) Unset the IV Containers in elements 1 & 2
         *      6) Reset the Array Index to 0
         *      7) See While loop Comments
         *
         */
        $String_Array = str_split($String);

       $Hash_Count = $String_Array[0];
       unset($String_Array[0]);
        $IVLength = $String_Array[1].$String_Array[2];
       $Hash = NULL;
        unset($String_Array[1]);
        unset($String_Array[2]);
       $Hash_Incrementer = 0;
       $String_Array = array_values($String_Array);
       while ($Hash_Incrementer < $Hash_Count){
           /*
            *  Use a while loop to pull correct Hash number from the string & unsetting as we go
            */
           $Hash .= $String_Array[$Hash_Incrementer];
           unset($String_Array[$Hash_Incrementer]);

           $Hash_Incrementer++;

       }
       /*
        *  Increment the array values to start from 1 instead of 0
        */
       array_unshift($String_Array, null);
       unset($String_Array[0]);

        $IV = null;
        $Encryption = null;
        foreach ($String_Array AS $Key => $Value){
            /*
             * Through each iteration of the array (using the keys) decides if the key is odd or even.
             *  If odd: Appending to the empty $IV var
             *  If Even: Appending to the encrypted string
             */
            if($Key&1) {
                $IV .= $Value;

            } else {
                $Encryption .= $Value;
            }
            unset($String_Array[$Key]); // Unset as we go
            if ($IVLength*2 == $Key){
                /*
                 *
                 *  If Correct IV length is equal to the Key then end the foreach loop.
                 *
                 *
                 * Length after manipulation in earlier functions = 41
                 * Pushing the three extra elements into the array = 44
                 * IV Length Counted = 16.
                 *
                 *          Maths:
                 *              41 + 3 = 44
                 *              16*2 = 32 + 1 = 33
                 *              41 - 33 = 8 (Remainding Characters from the EncryptedString
                 */
                break;
            }
        }


        return array(
            /*
             * Return an Array with correct information to be used for Decryption.
             *  "EncryptedString" = What was pushed in earlire foreach with an imploded array of the remainding Chars
             *  "IV" = The Correct IV
             * "Hash" = A Type juggled integer containing the Hash as created in the Encryption
             */
            "EncryptedString" => $Encryption.implode("",$String_Array),
            "IV" => $IV,
            "Hash" => (int)$Hash
        );
    }

   function Decrypt($Array){
        /*
         *  Decrypt The Encrypted String with passing correct information as managed by UnMergeString
         */
        return openssl_decrypt($Array['EncryptedString'],"AES-256-CBC",$Array['Hash'],0,$Array['IV']);
   }
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

In my opinion, no your class is not clearly documented and easily understandable.

Here are the main reasons I find this code... dirty:

  • The naming is misleading.
  • The naming is unusual.
  • There are too many comments.
  • Inconsistencies.

Is that it? Well, let me explain:

Misleading Names

  • The first thing I did was look at the class name, because that alone should tell you exactly what the class will do. No ifs-ands-or-buts. Sweet, the class is about encryption.

    Now, by definition, "Encryption" is:

    In cryptography, encryption is the process of encoding messages or information in such a way that only authorized parties can read it.

    This was simply from Googling "define encryption". How is this relevant? Well, if you check out the last bit of your code, it's decrypting (decoding if you want a polar of the quote). This seems like a stupid thing to point out, but in all honesty, I was expecting two classes, an encryption and a decryption class! This is what I mean by misleading names.

  • The next greatest glaring thing is the Merge and UnMerge functions (both of which I need to criticize later). When I saw Merge, I got lost. What in the world could an encrypting class need to merge? And why is it so prominent in the code?

    So I take a look at the contents, and it's A) defining variables to equal other variables, and B) obfuscating the encryption in a strange manner. In a completely education attitude, I find this function a waste of space and time. I think it takes up unnecessary space by appending a mish-mash of characters to an already encoded string, and I feel like the variable tide grew and washed up a bunch of unnecessary variables! What's the point in the whole while loop? If you have a reasonable explanation, I'd be more than welcome to be enlightened by the way.

    As of now though, I see absolutely no reason for this to be it's own method.

    Would I say the same about the UnMerge method? Most definitely. Without the Merge method, this un-merger wouldn't exist.

Naming is one of the most important things a programmer can get right. Here's a couple pointers for naming classes:

  • Class names are always nouns, not verbs. (Kudos to you)
  • Class names should be descriptive in nature without implying implementation. Since implementation can change, to imply implementation in the name forces the class name and all references to it to change or else the code can become misleading. Remember: "describing but not specifying".

Unusual Naming

Now, here's the fun part. Hehe I've got a lot to say! But before I begin, I want you to know that it's important to choose a code formatting standard that the readers will understand. There are many, many, many standards to choose from for PHP, and the choice is up to you. The three I listed are quite similar in style.

Having a style that people recognize makes reading almost 200% easier. Since I wasn't used to your style, it took me longer to read your code than it normally would have. Here's a couple notes (taken from PHP.net):

  • Method names follow the 'studlyCaps' (also referred to as 'bumpy case' or 'camel caps') naming convention, with care taken to minimize the letter count. The initial letter of the name is lowercase, and each letter that starts a new 'word' is capitalized.

  • Variable names must be meaningful. ... Variable names should be in lowercase. Use underscores to separate between words.

I focus on white space in the Inconsistencies section.

Too Many Comments

This one could be debated in certain situations, but in your case, I find your comments detrimental to the readability and conciseness of your code.

The comments are non-standard (PHPdoc), very spread out, in inappropriate places, and I think far too long.

PHPdocs are almost identical to JavaDocs, if you're familiar with that. If not, I suggest this standard to get you started.

I think you too much white space in between comments and inside comments. It forces the eyes to jump around.

If the variables names, method names, and class names are all excellent, comments are only needed in essential places. I think that because the names of these objects are vague, you're having to do a lot more commenting than necessary!

I think you're giving too much information. With the PHPdocs, it's all in that one place, compact, uniform, and concise.

There are however, times when inline comments are actually necessary, and it's perfectly acceptable to use them then. Comments aren't code, so of course it really doesn't matter what you do. I'm just thinking about us readers ;)

Inconsistencies

These are the little things we often forget about, which is why Code Review is here!

  • Compare Encrypt ($String) and Merge($Array). Do you want one space or none? Choose.
  • Compare $iv, $Size, and $IV. You need to decide what capitalization pattern should be followed.
  • Is it While or while? Don't fix it if it ain't broke. Nowhere in the PHP documentation are these keywords capitalized.

These are just a couple examples. I think you get the point!


Well, that is why I think it's unclear code! As far as your actual code logic, I could write another post on the security and alternate implementations and efficiency, but that would make this far too long! :)

\$\endgroup\$
5
  • \$\begingroup\$ This is greatly appreciated. When I jump back onto the computer I will happily review this entire post in more depth and make changes accordingly & give an overview of the merge and unmerge function. Thank you again for your time \$\endgroup\$
    – Daryl Gill
    Commented Jul 20, 2014 at 13:26
  • \$\begingroup\$ I have addressed some of the points provided and will continue to make adjustments accordingly to this post. As of now, the current version looks like: gist.github.com/DGDevelopments/b72340ec9070b8f4e3fd \$\endgroup\$
    – Daryl Gill
    Commented Jul 21, 2014 at 16:07
  • \$\begingroup\$ @darylgill Avoid updating this single question. The best thing you can do is 1) Mark the correct answer (you did), 2) revise your code, and then 3) start a new question! Doing so will get your question the attention and focus it deserves :) \$\endgroup\$
    – Alex L
    Commented Jul 21, 2014 at 16:17
  • \$\begingroup\$ Thanks for that information, unsure how things work correctly over this side of SO, but wouldn't multiple questions on a single code chunk clutter the database a little bit? \$\endgroup\$
    – Daryl Gill
    Commented Jul 21, 2014 at 16:19
  • 1
    \$\begingroup\$ Actually we prefer it! It's easy to separate the improvements and see the differences. It also gives everyone an equal shot at answering. If you haven't already, the FAQ has lots of answers, and being apart of the Code Review chat can quickly get you up to speed on this site! \$\endgroup\$
    – Alex L
    Commented Jul 21, 2014 at 16:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.