Skip to main content
3 of 3
finished review
ChrisWue
  • 20.4k
  • 4
  • 42
  • 107
  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.

ChrisWue
  • 20.4k
  • 4
  • 42
  • 107