Criptografa uma mensagem usando a cifra ADFGVX

Question

Este é um programa Java que implementei para criptografar uma string usando a cifra ADVGVX. Ele recebe a mensagem, a frase secreta usada para gerar o quadrado de Políbio e a palavra-chave para a parte de transposição da criptografia. Ele imprime a string criptografada. O que você sugere consertar e melhorar?

import java.util.*;

public class Cipher {
    public static void main(String args[]){

        // get input!

        // make scanner object for input
        Scanner scan = new Scanner(System.in);

        // the message to encrypt
        System.out.print("Enter message to encrypt: ");
        String mes = scan.nextLine();

        // keyphrase, used to make polybius square
        System.out.print("Enter keyphrase for Polybius square: ");
        String keyphrase = scan.nextLine();

        while (!validKeyphrase(keyphrase) ){
            System.out.print("Enter valid keyphrase: ");
            keyphrase = scan.nextLine();
        }

        // keyword for transposition
        System.out.print("Enter keyword for transposition: ");
        String keyword = scan.nextLine();

        while (keyword.length() <= 1 && keyword.length() > mes.length()){
            System.out.println("Keyword length must match message length.");
            System.out.print("Enter keyword: ");
            keyword = scan.nextLine();
        }

        // take keyphrase and chuck into polybius square
        char [][] square = new char[6][6];

        // putting keyphrase into a character array
        char[] letters = keyphrase.toCharArray();

        // filling the polybius square
        int counter = -1;
        for (int i = 0; i< 6; i++){
            for (int j=0; j< 6; j++){
                counter++;
                square[i][j] = letters[counter];
            }
        }

        // after the substitution
        String substitution = substitute(square, mes);

        // dimensions of transposition array
        int transY = keyword.length();
        int transX = substitution.length()/keyword.length()+1;

        char [][] newSquare = new char[transX][transY];

        // fills in the transposition square
        counter = -1;
        for (int i=0; i< transX; i++){
            for (int j=0; j< transY; j++){
                counter++;
                if (counter < substitution.length())
                    newSquare[i][j] = substitution.charAt(counter);
            }
        }

        // the keyword as a character array
        char [] keyArr = keyword.toCharArray();

        // switching columns based on a bubble sort

        boolean repeat = true;

        while (repeat){
            repeat = false;

            for (int i=0; i<keyArr.length-1; i++){
                if (keyArr[i+1] < keyArr[i]){
                    repeat = true;

                    //dealing with the keyArr
                    char temp = keyArr[i+1];
                    keyArr[i+1] = keyArr[i];
                    keyArr[i] = temp;

                    // dealing with the newSquare array
                    for (int n = 0; n < transY -1 ; n++){
                        temp = newSquare[n][i+1];
                        newSquare[n][i+1] = newSquare[n][i];
                        newSquare[n][i] = newSquare[n][i];
                        newSquare[n][i] = temp;
                    }
                }
            }
        }


        String result = "";
        StringBuilder sb = new StringBuilder(result);

        for (int i=0; i< transX; i++){
            for (int j=0; j< transY; j++){
                if (newSquare[i][j] != '\0')
                    sb.append(newSquare[i][j]);

            }
        }

        for (int i=0; i< sb.toString().length(); i++){
            System.out.print(sb.toString().charAt(i));
            if (i %2 == 1){
                System.out.print(" ");
            }
        }
        System.out.println();



    }

    // must contain exactly 36 characters
    // must contain all unique characters
    // must contain a-z/A-Z and 0-9

    public static boolean validKeyphrase(String s){
        if (s.length() != 36){
            return false;
        }
        String S = s.toLowerCase();

        Set<Character> foo = new HashSet<>();
        for (int i=0; i< S.length(); i++){
            foo.add(S.charAt(i));
        }

        if (foo.size() != S.length()){
            return false;
        }

        for (int i='a'; i<='z'; i++){
            if (foo.remove((char) i)){}
            else
                return false;
        }

        for (int i='0'; i<='9'; i++){
            if (foo.remove((char) i)){}
            else
                return false;
        }

        if (!foo.isEmpty())
            return false;

        return true;
    }

    public static String substitute(char[][] arr, String s){
        String result = "";
        final char[] cipher = {'A', 'D', 'F', 'G', 'V', 'X'};

        for (int k = 0; k < s.length(); k++){
            arrLoop: {
                for (int i=0; i< 6; i++){
                    for (int j=0; j< 6; j++){
                        if (s.charAt(k) == arr[i][j] ){
                            result += cipher[i];
                            result += cipher[j];
                            break arrLoop;
                        }
                    }
                }
            }
        }

        return result;
    }
}

Answer 1

Tenho algumas sugestões para o seu código.

Sempre adicione colchetes a loop&if

Em minha opinião, é uma prática ruim ter um bloco de código não cercado por chaves; Eu vi tantos bugs em minha carreira relacionados a isso, se você esquecer de colocar as chaves ao adicionar o código, você quebra a lógica / semântica do código.

Evite usar C-styledeclaração de array

No método principal, você declarou uma C-styledeclaração de array com a argsvariável.

antes

String args[]

após

String[] args

Na minha opinião, esse estilo é menos usado e pode causar confusão.

Extraia parte da lógica para métodos.

Quando você tem uma lógica que faz a mesma coisa, geralmente pode movê-la para um método e reutilizá-la.

Em seu método principal, você pode extrair a maior parte da lógica que faz ao usuário uma pergunta sobre novos métodos; isso encurtará o método e tornará o código mais legível.

Eu sugiro o seguinte refatorador:

1. Crie um novo método askUserAndReceiveAnswerque faça uma pergunta como uma string e retorne uma string com a resposta.

private static String askUserAndReceiveAnswer(Scanner scan, String s) {
   System.out.print(s);
   return scan.nextLine();
}

Este método pode ser reutilizado 3 vezes em seu código.

1. Crie um novo método que solicite ao usuário uma frase-chave válida.

private static String askUserForValidKeyPhrase(Scanner scan) {
   String keyphrase = askUserAndReceiveAnswer(scan, "Enter keyphrase for Polybius square: ");

   while (!validKeyphrase(keyphrase)) {
      System.out.print("Enter valid keyphrase: ");
      keyphrase = scan.nextLine();
   }
   return keyphrase;
}

1. Crie um novo método que solicite ao usuário uma palavra-chave válida.

private static String askUserForValidKeyword(Scanner scan, String mes) {
   String keyword = askUserAndReceiveAnswer(scan, "Enter keyword for transposition: ");

   while (keyword.length() <= 1 && keyword.length() > mes.length()) {
      System.out.println("Keyword length must match message length.");
      System.out.print("Enter keyword: ");
      keyword = scan.nextLine();
   }
   return keyword;
}

Use java.lang.StringBuilderpara concatenar String em um loop.

Geralmente é mais eficiente usar o construtor em um loop, uma vez que o compilador não é capaz de torná-lo eficiente em um loop; uma vez que cria uma nova String a cada iteração. Existem muitas boas explicações com mais detalhes sobre o assunto.

Cifra # substituto antes

public static String substitute(char[][] arr, String s) {
   String result = "";
   final char[] cipher = {'A', 'D', 'F', 'G', 'V', 'X'};

   for (int k = 0; k < s.length(); k++) {
      arrLoop: {
         for (int i = 0; i < 6; i++) {
            for (int j = 0; j < 6; j++) {
               if (s.charAt(k) == arr[i][j]) {
                  result += cipher[i];
                  result += cipher[j];
                  break arrLoop;
               }
            }
         }
      }
   }

   return result;
}

após

public static String substitute(char[][] arr, String s) {
   StringBuilder result = new StringBuilder();
   final char[] cipher = {'A', 'D', 'F', 'G', 'V', 'X'};

   for (int k = 0; k < s.length(); k++) {
      arrLoop: {
         for (int i = 0; i < 6; i++) {
            for (int j = 0; j < 6; j++) {
               if (s.charAt(k) == arr[i][j]) {
                  result.append(cipher[i]);
                  result.append(cipher[j]);
                  break arrLoop;
               }
            }
         }
      }
   }

   return result.toString();
}

Em vez de ter um corpo vazio em uma condição, inverta a lógica.

Em seu código, você tem várias condições que possuem um corpo vazio; Eu sugiro que você inverta a lógica para remover a confusão que isso pode criar.

Cifra # validKeyphrase

Antes

for (int i = 'a'; i <= 'z'; i++) {
   if (foo.remove((char) i)) {

   } else {
      return false;
   }
}

for (int i = '0'; i <= '9'; i++) {
   if (foo.remove((char) i)) {

   } else {
      return false;
   }
}

Após

for (int i = 'a'; i <= 'z'; i++) {
   if (!foo.remove((char) i)) {
      return false;
   }
}

for (int i = '0'; i <= '9'; i++) {
   if (!foo.remove((char) i)) {
      return false;
   }
}

Simplifique as condições booleanas.

Isso pode ser simplificado

if (!foo.isEmpty()) {
   return false;
}

return true;

para

return foo.isEmpty();

Answer 2

Aqui está o meu detalhamento sobre as práticas de código. Você realmente deve ser mais consistente e organizado em relação ao estilo do código. Além disso, você definitivamente deve usar mais métodos, melhores relatórios de erros e melhores palavras-chave.

Quanto ao design, eu esperaria ser capaz de instanciar a Cipher(por exemplo, um construtor com o quadrado como entrada) e, em seguida, ter métodos não estáticos encrypte decryptcom messagetamanhos idênticos passphrase. Esses métodos, por sua vez, devem ser subdivididos por meio de privatemétodos.

---

public class Cipher {

Isso não é específico o suficiente para um nome de classe.

---

System.out.print("Enter message to encrypt: ");
...
System.out.print("Enter valid keyphrase: ");

Eu deixaria um pouco mais claro o que se espera do usuário, por exemplo, que você precisa inserir uma linha ou que tipo de frase-chave é aceitável.

---

char [][] square = new char[6][6];

Que você execute a interface do usuário no mainé um tanto aceitável, mas a lógica de negócios não deve estar no método principal.

As variáveis 6devem estar em uma ou duas constantes.

---

char[] letters = keyphrase.toCharArray();

Mais tarde, descobriremos que o letterstambém deve conter dígitos. Chamamos isso de alfanuméricos ( alphaNumericals).

---

int counter = -1;
...
square[i][j] = letters[counter];

Tente evitar valores inválidos. Por exemplo, neste caso letters[counter++]teria deixado você começar com um zero.

---

int transX = substitution.length()/keyword.length()+1;

Sempre use espaços ao redor dos operadores, por exemplo substitution.length() / keyword.length() +1;.

---

// dimensions of transposition array
int transY = keyword.length();

Estou um pouco preocupado com a nomenclatura da variável aqui, transY não parece uma dimensão para mim. E o fato de você precisar prefixar transindica que você deve ter criado um método (veja o próximo comentário).

---

// fills in the transposition square

Se você tiver que fazer esse comentário, também pode criar um método, por exemplo, fillTranspositionSquare()certo?

---

for (int i=0; i< transX; i++){

Com certeza, se transXfor o máximo para x, você não está nomeando sua variável i, certo?

---

String result = "";

Este é definitivamente um cheiro de código. A atribuição nullou uma string vazia quase nunca é necessária.

Além disso, é aqui que você se cansa de explicar seu código nos comentários. Não seria necessário se você tivesse usado métodos bem nomeados.

---

StringBuilder sb = new StringBuilder(result);

Agora seu StringBuildertem capacidade de zero caracteres, com toda a probabilidade. Em vez disso, você já sabe o quão grande será no final, certo? Portanto, calcule o tamanho com antecedência e use o StringBuilder(int capacity)construtor.

---

if (s.length() != 36){

Nunca use literais assim. Em primeiro lugar, 36 é 6 x 6. Basta usar as dimensões para calcular esse número e, se for estático, coloque-o em uma constante.

---

String S = s.toLowerCase();

Você já tinha um se decidiu usar Spara uma string minúscula ? A sério? E por que snão é chamado keyphrase? Dica: você pode usar nomes simples durante a digitação, mas IDEs modernos permitirão que você renomeie as variáveis ​​posteriormente. Dessa forma, você pode digitar de forma concisa e torná-lo mais prolixo depois.

---

return false;

Não, aqui você deve ter um resultado mais complexo, por exemplo, um enum para indicar o tipo de falha. Apenas retornar false para qualquer tipo de falha não permitirá que você indique ao usuário o que está errado.

---

public static String substitute(char[][] arr, String s){

Espere, o polybiusSquaretornou-se arr? Por que é que?

---

String result = "";

Já mencionado, aqui String resultBuilder = new StringBuilder(s.length())certamente seria melhor.

---

arrLoop: {

Se você precisa de rótulos, está fazendo errado, na maioria das vezes. Observe que o rótulo é para o forloop, portanto, a chave não é necessária. Se alguma vez você precisar usar uma etiqueta, coloque-a totalmente em letras maiúsculas. No entanto, neste caso, o loop for duplo pode ser facilmente colocado dentro de um método separado, portanto, não é necessário.

Observe que o espaçamento de <é completamente inconsistente. Não usar espaçamento suficiente é ruim o suficiente, ter um estilo inconsistente é considerado pior.