3
\$\begingroup\$

I posted a custom byte cipher on this site a few months back, and after spending three days recoding it from scratch, wanted to post it again to see if you can offer any improvements. (I am not looking for: "You should use var instead of explicit declaration". I'm looking for significant code improvements, not coding style improvements.)

The base concept of my cipher works a little like a Caesar cipher, except it also includes a few "twists".

using System.Text;

namespace DreamforceFramework.Framework.Cryptography
{
    /// <summary>
    /// DreamforceByteCipher
    /// Gordon Kyle Wallace, "Krythic"
    /// Copyright (C) 2015 Gordon Kyle Wallace, "Krythic" - All Rights Reserved
    /// </summary>
    public static class DreamforceByteCipher
    {
        /// <summary>
        /// Encrypts a string into a byte array.
        /// </summary>
        /// <param name="data"></param>
        /// <param name="password"></param>
        /// <returns></returns>
        public static byte[] Encrypt(string data, string password)
        {
            byte[] bytes = Encoding.UTF8.GetBytes(data);
            string passwordHash = DreamforceHashing.GenerateSHA256(password);
            byte[] hashedPasswordBytes = Encoding.ASCII.GetBytes(passwordHash);
            int passwordShiftIndex = 0;
            bool shiftFlag = false;
            for (int i = 0; i < bytes.Length; i++)
            {
                int shift = hashedPasswordBytes[passwordShiftIndex];
                bytes[i] = shift <= 128
                    ? (byte)(data[i] + (shiftFlag
                        ? (byte)(((shift << 2)) % 255)
                        : (byte)(((shift << 4)) % 255)))
                    : (byte)(data[i] - (shiftFlag
                        ? (byte)(((shift << 4)) % 255)
                        : (byte)(((shift << 2)) % 255)));
                passwordShiftIndex = (passwordShiftIndex + 1) % 64;
                shiftFlag = !shiftFlag;
            }
            return bytes;
        }

        /// <summary>
        /// Decrypts a byte array back into a string.
        /// </summary>
        /// <param name="data"></param>
        /// <param name="password"></param>
        /// <returns></returns>
        public static string Decrypt(byte[] data, string password)
        {
            string passwordHash = DreamforceHashing.GenerateSHA256(password);
            byte[] hashedPasswordBytes = Encoding.UTF8.GetBytes(passwordHash);
            int passwordShiftIndex = 0;
            bool shiftFlag = false;
            for (int i = 0; i < data.Length; i++)
            {
                int shift = hashedPasswordBytes[passwordShiftIndex];
                data[i] = shift <= 128
                    ? (byte)(data[i] - (shiftFlag
                        ? (byte)(((shift << 2)) % 255)
                        : (byte)(((shift << 4)) % 255)))
                    : (byte)(data[i] + (shiftFlag
                        ? (byte)(((shift << 4)) % 255)
                        : (byte)(((shift << 2)) % 255)));
                passwordShiftIndex = (passwordShiftIndex + 1) % 64;
                shiftFlag = !shiftFlag;
            }
            return Encoding.ASCII.GetString(data);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Oh, forgot to mention the speed. This algorithm can Read, Encrypt, and Decrypt Huckleberry Finn in a total time of roughly (00:00:00.1157119). \$\endgroup\$ – Krythic Sep 24 '15 at 23:03
  • \$\begingroup\$ Just a heads up that, while you've put "All rights reserved" in your code comments, any content posted to Stack Exchange sites is licensed Creative Commons Attribution-ShareAlike 3.0 Unported. \$\endgroup\$ – Nick Udell Nov 24 '15 at 12:56
3
\$\begingroup\$

The bodies of methods are strikingly similar. In fact, you can make the similarity even more striking by reverting the shift <= 128 condition (and swapping branches, of course) in Decrypt:

            data[i] = shift > 128
                ? (byte)(data[i] + (shiftFlag
                    ? (byte)(((shift << 2)) % 255)
                    : (byte)(((shift << 4)) % 255)))
                : (byte)(data[i] - (shiftFlag
                    ? (byte)(((shift << 4)) % 255)
                    : (byte)(((shift << 2)) % 255)));

Now the difference is literally in one operator. I don't know idiomatic way to pass the operator into a method as a parameter (honestly I don't know C# whatsoever) - but I have no doubt there is. Lambda perhaps?

That done, you may factor out the non-DRY part into a helper method:

    byte[] transform(byte[] data, string password, operator compare)
    {
        string passwordHash = DreamforceHashing.GenerateSHA256(password);
        byte[] hashedPasswordBytes = Encoding.ASCII.GetBytes(passwordHash);
        int passwordShiftIndex = 0;
        bool shiftFlag = false;
        for (int i = 0; i < bytes.Length; i++)
        {
            int shift = hashedPasswordBytes[passwordShiftIndex];
            data[i] = compare(shift)
                ? (byte)(data[i] + (shiftFlag
                    ? (byte)(((shift << 2)) % 255)
                    : (byte)(((shift << 4)) % 255)))
                : (byte)(data[i] - (shiftFlag
                    ? (byte)(((shift << 4)) % 255)
                    : (byte)(((shift << 2)) % 255)));
            passwordShiftIndex = (passwordShiftIndex + 1) % 64;
            shiftFlag = !shiftFlag;
        }
        return data;
    }

and call it from both Encrypt and Decrypt.

\$\endgroup\$
  • \$\begingroup\$ If I am reading your suggestion correctly(I am on my phone), the code you presented now has no way of decrypting the data. If you look at the code, there is the initial +/- branch. This IS different for both functions. \$\endgroup\$ – Krythic Sep 24 '15 at 23:52
  • \$\begingroup\$ @Krythic Honestly I don't know what is this initial branch. Could you be more specific? \$\endgroup\$ – vnp Sep 24 '15 at 23:58
  • \$\begingroup\$ "[i] + (shiftFlag" is different for both functions. Decrypt is inverted. \$\endgroup\$ – Krythic Sep 25 '15 at 0:03
  • \$\begingroup\$ @Krythic Exactly. I inverted the meaning of comparison, and swapped branches. It is an invariant rewrite. Semantically it is identical to original. \$\endgroup\$ – vnp Sep 25 '15 at 0:16
  • \$\begingroup\$ You aren't seeing what I'm talking about. You didn't invert the right thing, or leave an option for it to be inverted. All you did was change the initial branch, which tested for byte-half as a makeshift conditional for which path to take. It wouldn't decrypt. \$\endgroup\$ – Krythic Sep 25 '15 at 0:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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