This is a Java program I implemented to encrypt a string using the ADVGVX cipher. It takes in the message, the passphrase used to generate the Polybius square, and keyword for the transposition part of the encryption. It prints the encrypted string. What would you suggest fixing and improving?
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;
}
}
2 Answers 2
I have some suggestions for your code.
Always add curly braces to loop & if
In my opinion, it's a bad practice to have a block of code not surrounded by curly braces; I saw so many bugs in my career related to that, if you forget to add the braces when adding code, you break the logic / semantic of the code.
Avoid using C-style array declaration
In the main method, you declared a C-style array declaration with the args variable.
before
String args[]
after
String[] args
In my opinion, this style is less used and can cause confusion.
Extract some of the logic to methods.
When you have logic that does the same thing, you can generally move it into a method and reuse it.
In your main method, you can extract most of the logic that asks the user a question to new methods; this will shorten the method and make the code more readable.
I suggest the following refactor:
- Create a new method
askUserAndReceiveAnswerthat ask a question as a string and return a string with the answer.
private static String askUserAndReceiveAnswer(Scanner scan, String s) {
System.out.print(s);
return scan.nextLine();
}
This method can be reused 3x time in your code.
- Create a new method that asks the user, for a valid keyphrase.
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;
}
- Create a new method that asks the user, for a valid keyword.
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.StringBuilder to concatenate String in a loop.
It's generally more efficient to use the builder in a loop, since the compiler is unable to make it efficient in a loop; since it creates a new String each iteration. There are lots of good explanations with more details on the subject.
Cipher#substitute Before
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;
}
after
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();
}
Instead of having empty body in a condition, invert the logic.
In your code, you have multiple conditions that have an empty body; I highly suggest that you invert the logic to remove the confusion those can create.
Cipher#validKeyphrase
Before
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;
}
}
After
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;
}
}
Simplify the boolean conditions.
This can be simplified
if (!foo.isEmpty()) {
return false;
}
return true;
to
return foo.isEmpty();
Here's my nitpick on code practices. You should really be more consistent and more neat with regard to code style. Furthermore, you should definitely use more methods, better error reporting and better keywords.
As for the design, I would expect to be able to instantiate a Cipher (e.g. a constructor with the square as input) and then have non-static encrypt and decrypt methods on them that take a message and identically sized passphrase. Those methods in turn should be subdivided using private methods.
public class Cipher {
That's not specific enough for a class name.
System.out.print("Enter message to encrypt: ");
...
System.out.print("Enter valid keyphrase: ");
I'd make it a bit more clear what is expected from the user, e.g. that you have to enter a line, or what kind of keyphrase is acceptable.
char [][] square = new char[6][6];
That you perform the user interface within the main is somewhat acceptable, but the business logic should not be in the main method.
The variables 6 should be in a constant or two.
char[] letters = keyphrase.toCharArray();
Later on we'll find that the letters should also contain digits. We call those alphanumericals (alphaNumericals).
int counter = -1;
...
square[i][j] = letters[counter];
Try and avoid invalid values. For instance, in this case letters[counter++] would have let you start with a zero.
int transX = substitution.length()/keyword.length()+1;
Always use spaces around operators, e.g. substitution.length() / keyword.length() +1;.
// dimensions of transposition array
int transY = keyword.length();
I'm a bit worried about the variable naming here, transY doesn't sound like a dimention to me. And the fact that you need to prefix trans indicates that you should have created a method (see next comment).
// fills in the transposition square
If you have to make such a comment then you might as well create a method, e.g. fillTranspositionSquare() right?
for (int i=0; i< transX; i++){
For sure, if transX is the maximum for x, you're not naming your variable i, right?
String result = "";
This is definitely a code smell. Assigning null or an empty string is almost never needed.
Also, this is where you got bored explaining your code in comments. It would not be necessary if you'd had used well named methods.
StringBuilder sb = new StringBuilder(result);
Now your StringBuilder has a capacity of zero character, in all likelihood. Instead, you already know how large it will be in the end, right? So calculate the size beforehand and use the StringBuilder(int capacity) constructor.
if (s.length() != 36){
Never use literals like that. First of all, 36 is 6 x 6. Just use the dimensions to calculate that number, and if it is indeed static, put it in a constant.
String S = s.toLowerCase();
You already had an s and decided to use S for a lowercase string? Seriously? And why is s not called keyphrase? Hint: you can use simple names while typing, but modern IDE's will let you rename variables afterwards. That way you can type concisely and make it more verbose afterwards.
return false;
No, here you should have a more complex result, e.g. an enum to indicate the kind of failure. Just returning false for any kind of failure won't allow you to indicate to the user what is wrong.
public static String substitute(char[][] arr, String s){
Wait, the polybiusSquare has become arr? Why is that?
String result = "";
Mentioned already, here String resultBuilder = new StringBuilder(s.length()) would certainly be better.
arrLoop: {
If you need labels then you're doing it wrong, most of the time. Note that the label is for the for loop, so the brace is not necessary. If you ever need to use a label, make it fully upper case. However, in this case the double for loop can easily be put inside a separate method, so it is not required.
Note that the spacing for < is completely inconsistent. Not using enough spacing is bad enough, having an inconsistent style is considered worse.
What [needs fixing]?For starters, the acronym in the post body. (Then, as of 2020年9月5日, CR is one of the sites needing a newline following a~~~or```closing a code block.) \$\endgroup\$