How to optimize this Playfair encryption?

Question

For the implementation of the Playfair encryption I needed a custom struct called Cell. This is because I not only need an array of characters I also want to get Elements in a matrix based on their "cartesian" position.

public struct Cell 
    {
        public char character;
        public int X;
        public int Y;

        public Cell(char _character,int _X, int _Y) 
        {
            this.character = _character;
            this.X = _X;
            this.Y = _Y;
        }
    }

Then I implement the method which calculates the cipher.

public static string PlayfairCipher(string keyWord,string plainText)
{

    //Define alphabet
    //There is no J in the alphabet instead I is used
    char[] alphabet = "ABCDEFGHIKLMNOPQRSTUVWXYZ".ToCharArray();

    #region Adjust Key
    keyWord = keyWord.Trim();
    keyWord = keyWord.Replace(" ", "");
    keyWord = keyWord.Replace("J", "I");
    keyWord = keyWord.ToUpper();

    StringBuilder keyString = new StringBuilder();

    foreach(char c in keyWord)
    {
        if(!keyString.ToString().Contains(c))
        {
            keyString.Append(c);
            alphabet = alphabet.Where(val => val != c).ToArray();
        }
    }
    #endregion

    #region Adjust plain text
    plainText = plainText.Trim();
    plainText = plainText.Replace(" ", "");
    plainText = plainText.Replace("J", "I");
    plainText = plainText.ToUpper();

    //If the Length of the plain text is odd add X
    if((plainText.Length % 2 > 0))
    {
        plainText += "X";
    }

    List<string> plainTextEdited = new List<string>();

    //Split plain text into pairs
    for (int i = 0; i < plainText.Length;i += 2)
    {
        //If a pair of chars contains the same letters replace one of them with X
        if (plainText[i].ToString() == plainText[i + 1].ToString())
        {
            plainTextEdited.Add(plainText[i].ToString() + 'X');
        }
        else {
            plainTextEdited.Add(plainText[i].ToString() + plainText[i + 1].ToString());
        }


    }
    #endregion

    #region Create 5 x 5 matrix

    List<Cell> matrix = new List<Cell>();
    int keyIDCounter = 0;
    int alphabetIDCounter = 0;
    //Fill the matrix. First with the key characters then with the alphabet
    for (int x = 0; x < 5;x++)
    {
        for (int y = 0; y < 5; y++)
        {
            if (keyIDCounter < keyString.Length)
            {

                Cell cell = new Cell(keyString[keyIDCounter],x,y);
                matrix.Add(cell);
                keyIDCounter++;
            }
            else {
                Cell cell = new Cell(alphabet[alphabetIDCounter], x, y);
                matrix.Add(cell);
                alphabetIDCounter++;
            }
        }
    }
    #endregion


    #region Write cipher

    StringBuilder cipher = new StringBuilder();

    foreach(string pair in plainTextEdited)
    {

        int indexA = matrix.FindIndex(c => c.character == pair[0]);
        Cell a = matrix[indexA];

        int indexB = matrix.FindIndex(c => c.character == pair[1]);
        Cell b = matrix[indexB];

        //Write cipher
        if (a.X == b.X)
        {
            cipher.Append(matrix[matrix.FindIndex(c => c.X == (a.X + 1)%5 && c.Y == a.Y)].character);
            cipher.Append(matrix[matrix.FindIndex(c => c.X == (b.X + 1)%5 && c.Y == b.Y)].character);
        }
        else if(a.Y == b.Y)
        {
            cipher.Append(matrix[matrix.FindIndex(c => c.X == a.X && c.Y == (a.Y + 1) % 5)].character);
            cipher.Append(matrix[matrix.FindIndex(c => c.X == b.X % 5 && c.Y == (b.Y + 1) % 5)].character);
        }else
        {
            cipher.Append(matrix[matrix.FindIndex(c => c.X == a.X && c.Y == b.Y)].character);
            cipher.Append(matrix[matrix.FindIndex(c => c.X == b.X % 5 && c.Y == a.Y)].character);
        }

    }
    #endregion

    return cipher.ToString();

}

Should I split this into more separate methods? Do I really need the Cell struct or is there another way to get an element's coordinates? Also how is the overall implementation and code readability? What should I improve regarding future implementations?

Answer 1

1. Your Cell struct should be immutable - you can achieve that by making all the fields readonly.

2. In Cell character should be PascalCase.

3. You repeat the code for trimming and replacing characters for they key and plain text. This should be moved into a method.

4. There is some convoluted code which iterates of the keyWord and adds stuff to a keyString. If I read it correctly then what this is doing is: Build a list of all unique characters and remove those from the alphabet. LINQ is perfect for doing that in a concise way:

   var uniqueKeyChars = keyWord.Distinct().ToList();
   alphabet = alphabet.Except(uniqueKeyChars).ToArray(); 
   

5. I would consider moving the whole pad-to-even-length and then split into pairs business into a separate methods as well. Also using a Tuple to represent pairs of characters seems to be more appropriate.

   public static IEnumerable<Tuple<char, char>> GetPairs(this IEnumerable<char> input)
   {
       while (input.Any())
       {
           var pair = input.Take(2);
           char first = pair.First();
           char second = pair.Skip(1).Any() ? pair.Last() : 'X';
           yield return Tuple.Create(first, second);
           input = input.Skip(2);
       }
   }
   

   Usage:

   var plainTextPairs = plainText.GetPairs();
   

6. If you have followed the suggestions above you will now have a uniqueKeyChars array as well as the alphabet with the uniqueKeyChars removed. This should make building the matrix easier:

   var cellAlphabet = new Queue<char>(uniqueKeyChars.Concat(alphabet));
   for (int x = 0; x < 5; x++)
   {
       for (int y = 0; y < 5; y++)
       {
           var cell = new Cell(cellAlphabet(cellAlphabet.Dequeue()), x, y);
           matrix.Add(cell);
       }
   }
   

7. When creating the cipher if you split the index finding from the appending then the code becomes slightly more readable (below assuming you use the extension method shown above):

   foreach(string pair in plainTextPairs)
   {
       int indexA = matrix.FindIndex(c => c.character == pair.Item1);
       Cell a = matrix[indexA];
   
       int indexB = matrix.FindIndex(c => c.character == pair.Item2);
       Cell b = matrix[indexB];
   
       int cipherCellIndexA;
       int cipherCellIndexB;
   
       //Write cipher
       if (a.X == b.X)
       {
           cipherCellIndexA = matrix.FindIndex(c => c.X == (a.X + 1) % 5 && c.Y == a.Y);
           cipherCellIndexB = matrix.FindIndex(c => c.X == (b.X + 1)%5 && c.Y == b.Y);
       }
       else if(a.Y == b.Y)
       {
           cipherCellIndexA = matrix.FindIndex(c => c.X == a.X && c.Y == (a.Y + 1) % 5);
           cipherCellIndexB = matrix.FindIndex(c => c.X == b.X % 5 && c.Y == (b.Y + 1) % 5);
       }else
       {
           cipherCellIndexA = matrix.FindIndex(c => c.X == a.X && c.Y == b.Y);
           cipherCellIndexB = matrix.FindIndex(c => c.X == b.X % 5 && c.Y == a.Y);
       }
   
       cipher.Append(matrix[cipherCellIndexA].Character);
       cipher.Append(matrix[cipherCellIndexB].Character);
   }
   

   Of course of you would make your matrix an actual 2d array then you could skip the whole FindIndex shebang and just pick the cell by the index which should speed up things a bit.