1
\$\begingroup\$

I have implemented a play fair cipher. The code works fine but I would like to improve the clarity and/or performance of the code. First a quick description of how it works.

You are given a string to encode along with a key. The text can only contain alphabets (i.e. no spaces or punctuation). Also this cipher is case-insensitive.

Suppose we have "ENCODE ME" as text to be encoded and "FOO" as the key. We start by removing spaces from the text and duplicate letters from the key then converting them into uppercase.

Now the text becomes "ENCODEME" and key becomes "FO". Then we make a five by five grid of characters and fill in the key first like below.

F 0 . . . 
. . . . .
. . . . . 
. . . . .

Now we fill in the remaining English alphabets in the grid. Notice that we have only 25 cells in the grid which is one less than the number of alphabets in English. So the solution to this problem is we have to treat two letters the same. People usually treat I and J as the same alphabet. Now the grid becomes.

F O A B C
D E G H I 
K L M N P
Q R S T U
V W X Y Z

Now lets encode the string. First we have to break down the string into pairs of two characters like EN CO DE ME If the string is not of even length we append X at the end of the string.

Pick first pair EN and find them in the grid. notice they form two corners (top left and right bottom) of a rectangle. We have to pick the other two corners of the rectangle they are H and L as shown velow

. . . . .
. E . H . 
. L . N . 
. . . . .
. . . . .

keep doing that with all the pairs and you get the encoded string. Below are some edge cases.

  1. If both characters in a pair in the same row you pick next for every character. For example a b c d e you will choose c for b and since e is the last character you pick a.
  2. For letters in same column you do the same as above but vertically.

  3. if both letters of the pair are same i,e, they overlap you apply the rule 2 on both characters.

You can reverse this whole process for decryption.

Crypto.java

public class Crypto {
    private static boolean verbose = true;
    public static void setVerbose(boolean verbosity) {
        verbose = verbosity;
    }
    public static String encrypt(String text , String key) {
        if (verbose) System.out.println("Text to encode : " +  text);
        if (verbose) System.out.println("With key : " +  key);
        text = text.toUpperCase().replace("J" , "I").replace(" ","");
        if (verbose) System.out.println("Processed text : " +  text);
        key = processKey(key);
        if (verbose) System.out.println("Processed key : " +  key);
        char[][] grid = buildGrid(key);
        return processText(grid , text , key);
    }
    public static String decrypt(String text , String key) {
        if (verbose) System.out.println("Text to decode : " +  text);
        if (verbose) System.out.println("With key : " +  key);
        text = text.toUpperCase().replace("J" , "I").replace(" ","");
        if (verbose) System.out.println("Processed text : " +  text);
        key = processKey(key);
        if (verbose) System.out.println("Processed key : " +  key);
        char[][] grid = buildGrid(key);
        return reverseProcess(grid , text , key);
    }
    private static String  processKey(String key) {
        key = key.toUpperCase().replace('J','I');
        String newKey = "";
        boolean[] map = new boolean[26];
        for (int i = 0 ; i < key.length() ; i++)
            if (!map[key.charAt(i) -65]) {
                newKey += key.charAt(i);
                map[key.charAt(i) -65 ] = true;
            }
        return newKey;
    }
    private static char[][] buildGrid( String key) {
        char[][] grid = new char[5][5];
        boolean[] map = new boolean[26];
        map['J' - 65] = true;
        for (int i = 0 ; i < key.length() ; i++) {
            grid[i/5][i%5] = key.charAt(i);
            map[key.charAt(i) - 65] = true;
        }
        int alpha = 0;
        for (int i = key.length() ; i < 25 ; i++) {
            if (!map[alpha])
                grid[i/5][i%5] = (char)(alpha + 65);
            else {
                while (map[alpha]) alpha++;
                grid[i/5][i%5] = (char)(alpha + 65);
            }
            alpha ++;
        }
        if (verbose) printGrid(grid);
        return grid;
    }
    //for encryption
    private static String processText(char[][] grid , String text , String key) 
    {
        String result = "";
        if (text.length() % 2 != 0) text += "X";
        for (int i = 0 ; i < text.length() ; i+= 2) {
            char a = text.charAt(i);
            char b = text.charAt(i+1);
            int a_index = getIndex(grid , a);
            int b_index = getIndex(grid , b);
            if (verbose) System.out.print("encoding : " + a + "" + b);
            int p = a_index / 5;
            int q = a_index % 5;
            int u = b_index / 5;
            int v = b_index % 5;
            if (p == u && q == v) {
                q++;
                v++;
                q = q % 5;
                v = v % 5;
                a = grid[p][q];
                b = grid[u][v];
            }
            else if (p == u) {
                q++;
                v++;
                q = q % 5;
                v = v % 5;
                a = grid[p][q];
                b = grid[u][v];
            }
            else if (q == v) {
                p++;
                u++;
                p = p % 5;
                u = u % 5;
                a = grid[p][q];
                b = grid[u][v];
            }
            else {
                a = grid[p][v];
                b = grid[u][q];
            }
            result += a + "" + b;
            if (verbose) System.out.println(" as : " + a + "" + b);
        }
        if (verbose) System.out.println("Result : " + result);
        return result;
    }
    //for decryption
    private static String reverseProcess(char[][] grid , String text , String key) {
        String result = "";
        if (text.length() % 2 != 0) text += "X";
        for (int i = 0 ; i < text.length() ; i+= 2) {
            char a = text.charAt(i);
            char b = text.charAt(i+1);
            int a_index = getIndex(grid , a);
            int b_index = getIndex(grid , b);
            if (verbose) System.out.print("decoding : " + a + "" + b);
            int p = a_index / 5;
            int q = a_index % 5;
            int u = b_index / 5;
            int v = b_index % 5;
            if (p == u && q == v) {
                q--;
                v--;
                if (q < 0) q = 4;
                if (v < 0) v = 4;
                a = grid[p][q];
                b = grid[u][v];
            }
            else if (p == u) {
                q--;
                v--;
                if (q < 0) q = 4;
                if (v < 0) v = 4;
                a = grid[p][q];
                b = grid[u][v];
            }
            else if (q == v) {
                p--;
                u--;
                if (p < 0) p = 4;
                if (u < 0) u = 4;
                a = grid[p][q];
                b = grid[u][v];
            }
            else {
                a = grid[p][v];
                b = grid[u][q];
            }
            result += a + "" + b;
            if (verbose) System.out.println(" as : " + a + "" + b);
        }
        if (verbose) System.out.println("Result : " + result);
        return result;
    }
    private static int getIndex(char[][] grid , char key) {
        int idx = -1;
        for (int i = 0 ; i < 25 ; i++)
            if (grid[i/5][i%5] == key) {
                idx = i;
                break;
            }
        return idx;
    }
    private static void printGrid(char[][] g) {
        for (int i = 0 ; i < 5 ; i++) {
            for (int j = 0 ; j < 5 ; j++)
                System.out.print(g[i][j] + " ");
            System.out.println();
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

I would :

1) Refactor methods encrypt() and decrypt() wich do almost the same thing. Additionaly, in those methods, I would clean the input string by removing every character that is not between 'A' and 'Z'.

2) replace all the "if (verbose) ..." lines by a call to a logger. And setVerbose() by a setLoggingLevel(). That, may clear up your code a bit, provide more flexibility about the data that you want to trace and about where you want that data to end up (the console for now, but maybe a file later).

3) This point is subject to debate: in your class everything is static. This means that you need to provide a key for each text you want to encipher/decipher. If you have multiple texts to encipher/decipher, you can consider creating a setKey() method or provide the key as a paramter to a new constructor to the Crypto class. By the way, the class would benefit from a more descriptive name as it performs Playfair encyption/decryption, and not any other.

4) I would use constants (private static final MY_CONSTANT=3;) rather than 65, 25, 5, etc.

5) It is usually better to stay away from arrays[]. But, here, the problem is easy to express with arrays, so I would keep that.

This is what I have from the top of my head. I didn't look into the algorithmic cogs of your class, though.

\$\endgroup\$
  • 3
    \$\begingroup\$ Can you please elaborate on why we should stay away form arrays. Any place where I have used arrays is because the size of those arrays is independent of the input to to the cipher method. \$\endgroup\$ – Sayam Qazi Apr 18 '17 at 14:36

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.