The Atbash Cipher

Question

I am doing some exercises in C# and I have just finished writing my own implementation of the atbash cipher, here is the code:

public class AtbashCipher
{
    private string _plain = "abcdefghijklmnopqrstuvwxyz";
    private string _cipher = "zyxwvutsrqponmlkjihgfedcba";
    private readonly string _input;
    private readonly List<int> _indices = new List<int>();

    public AtbashCipher(string input)
    {
        _input = input;
    }

    public string Encode()
    {
        Splitter(_input).ForEach(x => _indices.Add(_plain.IndexOf(x)));
        return _indices.Aggregate("", (current, index) => current + _cipher[index]);
    }

    public string Decode()
    {
        Splitter(_input).ForEach(x => _indices.Add(_cipher.IndexOf(x)));
        return _indices.Aggregate("", (current, index) => current + _plain[index]);
    }

    private List<char> Splitter(string input)
    {
        return input.ToCharArray().ToList();
    }
}

It feels a little repetitive at times, and all the variables in the top of my class looks messy. Is there a more elegant way to do this?

Answer 1

Well, if you really don't want repetition, you could do something like

public static class AtbashCipher
{
    private static string Rotate(string input, bool direction)
    {
        char min = direction ? 'z' : 'a';
        char max = direction ? 'a' : 'z';

        return input.ToList().Aggregate("", (current, character) => current + (char)(min - (character - max)));
    }

    public static string Encode(string plaintext)
    {
        return Rotate(plaintext, true);
    }

    public static string Decode(string cyphertext)
    {
        return Rotate(cyphertext, false);
    }
}

Might be more performant to use a StringBuilder and a foreach loop, instead of an Aggregate() call, if you expect really large inputs.

Answer 2

Since that AtBash Cipher is a simple reverse, you don't need both _plain and _cipher. There is no reason for _indices to be at the class level; it should instead be locally defined in both Encode and Decode.

You may even consider doing away with having a class instance and instead make the class and its methods static. If you keep as a class instance, Decode could be simplified to simply returning _input!!!

You should be sure to set the input string to lower case in case someone enters "Hello". Something like:

_input = input.ToLowerInvariant();

Except you should try to avoid the Turkish "ll" issue of surrogate pairs to be treated as single character. The way to avoid is to use upper case.

Also the Splitter should just use the char[] and not convert ToList(). Then again, you don't need really Splitter.

Those are all things to think about. Here's my take on it:

public static class AtbashCipher
{
    private const string DefaultAlphabet = "abcdefghijklmnopqrstuvwxyz";

    public static string Encode(string input, string alphabet = DefaultAlphabet)
    {
        var plain = alphabet.ToUpperInvariant();
        var letters = new List<char>(input.Length);
        foreach (var c in input.ToUpperInvariant())
        {
            if (char.IsLetter(c))
            {
                // Get index of plain alphabet, but subtract from end
                var index = plain.Length - plain.IndexOf(c) - 1;
                letters.Add(plain[index]);
            }
        }
        return new string(letters.ToArray());
    }

    public static string Decode(string input, string alphabet = DefaultAlphabet)
    {
        // reverse alphabet to get cipher
        string cipher = new string(alphabet.ToUpperInvariant().Reverse().ToArray());

        var letters = new List<char>(input.Length);
        foreach (var c in input.ToUpperInvariant())
        {
            if (char.IsLetter(c))
            {
                var index = cipher.Length - cipher.IndexOf(c) - 1;
                letters.Add(cipher[index]);
            }
        }
        return new string(letters.ToArray());
    }
}

You may notice that Encode and Decode look very similar except for whether plain or cipher is chosen. In the name of DRY (Don't Repeat Yourself), this can be reduced to:

public static class AtbashCipher
{
    private const string DefaultAlphabet = "abcdefghijklmnopqrstuvwxyz";

    public static string Encode(string input, string alphabet = DefaultAlphabet)
    {
        var plain = alphabet.ToUpperInvariant();
        return Transform(input, plain);
    }

    public static string Decode(string input, string alphabet = DefaultAlphabet)
    {
        var cipher = new string(alphabet.ToUpperInvariant().Reverse().ToArray());
        return Transform(input, cipher);
    }

    private static string Transform(string input, string code)
    {
        var letters = new List<char>(input.Length);
        foreach (var c in input.ToUpperInvariant())
        {
            if (char.IsLetter(c))
            {
                var index = code.Length - code.IndexOf(c) - 1;
                letters.Add(code[index]);
            }
        }
        return new string(letters.ToArray());
    }
}

Answer 3

Atbash is its own inverse, so you do not need separate Encode() and Decode() methods: Atbash(b) = y; Atbash(y) = b for example. A single Transform() method is all you need.

Neither do you need your _plain and _cipher arrays, you can use the built in character ordering. A character c is (c - 'a') places ahead of 'a' in the alphabet, so its Atbash coding is the same number of places before 'z': ('z' - (c - 'a')). That can be simplified by gathering the constant parts: (('z' + 'a') - c).

My C# is very rusty and out of date, so this is in pseudocode:

constant zANDa <- ascVal('z') + ascVal('a')

method Transform(inText) returns String
  inText <- toLowercase(inText)
  outText <- ""
  foreach character c in inText
    if (IsAlphabetic(c))
      outText.append(char(zANDa - ascVal(c)))
    else
      // Non-alphabetic characters.
      outText.append(c)
    end if
  end foreach
  return outText
end Transform

For uppercase letters use constant ZandA <- ascVal('Z') + ascVal('A').