Szyfruje wiadomość za pomocą szyfru ADFGVX
To jest program Java, który zaimplementowałem, aby zaszyfrować ciąg znaków przy użyciu szyfru ADVGVX. Pobiera wiadomość, hasło użyte do wygenerowania kwadratu Polybiusa i słowo kluczowe dla transpozycji części szyfrowania. Drukuje zaszyfrowany ciąg. Co byś sugerował naprawienia i ulepszenia?
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;
}
}
Odpowiedzi
Mam kilka sugestii dotyczących twojego kodu.
Zawsze dodawaj nawiasy klamrowe do loop&if
Moim zdaniem to zła praktyka, aby blok kodu nie był otoczony nawiasami klamrowymi; Widziałem w mojej karierze tyle błędów związanych z tym, że jeśli zapomnisz dodać nawiasy klamrowe podczas dodawania kodu, złamiesz logikę / semantykę kodu.
Unikaj używania C-styledeklaracji tablicy
W metodzie main zadeklarowałeś C-styledeklarację tablicy ze argszmienną.
przed
String args[]
po
String[] args
Moim zdaniem ten styl jest rzadziej używany i może powodować zamieszanie.
Wyodrębnij część logiki do metod.
Jeśli masz logikę, która robi to samo, możesz ogólnie przenieść ją do metody i użyć jej ponownie.
W swojej głównej metodzie możesz wyodrębnić większość logiki, która zadaje użytkownikowi pytanie dotyczące nowych metod; skróci to metodę i sprawi, że kod będzie bardziej czytelny.
Proponuję następujący refaktor:
- Utwórz nową metodę,
askUserAndReceiveAnswerktóra zadaje pytanie jako ciąg znaków i zwraca ciąg z odpowiedzią.
private static String askUserAndReceiveAnswer(Scanner scan, String s) {
System.out.print(s);
return scan.nextLine();
}
Ta metoda może być ponownie wykorzystana 3 razy w kodzie.
- Utwórz nową metodę, która pyta użytkownika o prawidłowe hasło.
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;
}
- Utwórz nową metodę, która pyta użytkownika o prawidłowe słowo kluczowe.
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;
}
Służy java.lang.StringBuilderdo łączenia ciągu znaków w pętli.
Generalnie bardziej wydajne jest użycie konstruktora w pętli, ponieważ kompilator nie jest w stanie uczynić go wydajnym w pętli; ponieważ tworzy nowy ciąg w każdej iteracji. Istnieje wiele dobrych wyjaśnień zawierających więcej szczegółów na ten temat.
Zastąpienie szyfru # przed
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;
}
po
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();
}
Zamiast mieć puste ciało w warunku, odwróć logikę.
W swoim kodzie masz wiele warunków, które mają pustą treść; Zdecydowanie sugeruję odwrócenie logiki, aby usunąć zamieszanie, jakie mogą one wywołać.
Cipher # validKeyphrase
Przed
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;
}
}
Po
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;
}
}
Uprość warunki logiczne.
Można to uprościć
if (!foo.isEmpty()) {
return false;
}
return true;
do
return foo.isEmpty();
Oto mój czubek na temat praktyk kodowania. Naprawdę powinieneś być bardziej spójny i schludniejszy w odniesieniu do stylu kodu. Ponadto zdecydowanie powinieneś używać więcej metod, lepszego raportowania błędów i lepszych słów kluczowych.
Jeśli chodzi o projekt, spodziewałbym się, że będę w stanie utworzyć instancję a Cipher(np. Konstruktor z kwadratem jako dane wejściowe), a następnie mieć na nich metody niestatyczne encrypti decryptmetody, które przyjmują rozmiar a messagei identyczny rozmiar passphrase. Te metody z kolei należy podzielić za pomocą privatemetod.
public class Cipher {
To nie jest wystarczająco szczegółowe dla nazwy klasy.
System.out.print("Enter message to encrypt: ");
...
System.out.print("Enter valid keyphrase: ");
Wyjaśniłbym nieco bardziej szczegółowo, czego oczekuje się od użytkownika, np. Że musisz wprowadzić linię lub jaki rodzaj frazy jest akceptowalny.
char [][] square = new char[6][6];
Wykonanie interfejsu użytkownika w ramach mainjest w pewnym stopniu akceptowalne, ale logika biznesowa nie powinna znajdować się w głównej metodzie.
Zmienne 6powinny mieć stałą lub dwie.
char[] letters = keyphrase.toCharArray();
Później przekonamy się, że letterspowinien również zawierać cyfry. Nazywamy te znaki alfanumeryczne ( alphaNumericals).
int counter = -1;
...
square[i][j] = letters[counter];
Spróbuj uniknąć nieprawidłowych wartości. Na przykład w tym przypadku letters[counter++]pozwoliłbyś zacząć od zera.
int transX = substitution.length()/keyword.length()+1;
Zawsze używaj spacji wokół operatorów, np substitution.length() / keyword.length() +1;.
// dimensions of transposition array
int transY = keyword.length();
Martwię się trochę o nazewnictwo zmiennych tutaj, transY nie brzmi dla mnie jak wymiar. A fakt, że potrzebujesz prefiksu, transwskazuje, że powinieneś był utworzyć metodę (patrz następny komentarz).
// fills in the transposition square
Jeśli musisz zrobić taki komentarz, to równie dobrze możesz stworzyć metodę, np. fillTranspositionSquare()Prawda?
for (int i=0; i< transX; i++){
Na pewno, jeśli transXjest to maksimum dla x, nie nazywasz swojej zmiennej i, prawda?
String result = "";
To zdecydowanie zapach kodu. Przypisywanie nulllub pusty ciąg prawie nigdy nie jest potrzebne.
Tutaj też znudziło Ci się wyjaśnianie kodu w komentarzach. Nie byłoby to konieczne, gdybyś używał dobrze nazwanych metod.
StringBuilder sb = new StringBuilder(result);
Teraz Twój StringBuilderma pojemność zero znaków, najprawdopodobniej. Zamiast tego już wiesz, jak duży będzie w końcu, prawda? Więc oblicz wcześniej rozmiar i użyj StringBuilder(int capacity)konstruktora.
if (s.length() != 36){
Nigdy nie używaj takich literałów. Przede wszystkim 36 to 6 x 6. Po prostu użyj wymiarów, aby obliczyć tę liczbę, a jeśli jest rzeczywiście statyczna, umieść ją w stałej.
String S = s.toLowerCase();
Masz już si zdecydowałeś się użyć Sdla ciągu małych liter ? Poważnie? A dlaczego snie jest nazywany keyphrase? Wskazówka: możesz używać prostych nazw podczas pisania, ale nowoczesne IDE pozwolą ci później zmienić nazwy zmiennych. W ten sposób możesz pisać zwięźle, a później uczynić je bardziej szczegółowymi.
return false;
Nie, tutaj powinieneś otrzymać bardziej złożony wynik, np. Wyliczenie wskazujące rodzaj błędu. Samo zwrócenie fałszu w przypadku jakiegokolwiek błędu nie pozwoli Ci wskazać użytkownikowi, co jest nie tak.
public static String substitute(char[][] arr, String s){
Czekaj, polybiusSquarestało się arr? Dlaczego?
String result = "";
Wspomniany już, tutaj na String resultBuilder = new StringBuilder(s.length())pewno byłoby lepiej.
arrLoop: {
Jeśli potrzebujesz etykiet, przez większość czasu robisz to źle. Zwróć uwagę, że etykieta dotyczy forpętli, więc klamra nie jest konieczna. Jeśli kiedykolwiek będziesz musiał użyć etykiety, wpisz ją w całości wielkimi literami. Jednak w tym przypadku podwójną pętlę for można łatwo umieścić w oddzielnej metodzie, więc nie jest to wymagane.
Zauważ, że odstępy dla <są całkowicie niespójne. Niewystarczające odstępy są wystarczająco złe, a niespójny styl jest uważany za gorszy.