Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
Community Bot
Community Bot
  • 1

You may want to consider using some third-party librariesthird-party libraries to help you parse command-line options, instead of doing yourself.

You may want to consider using some third-party libraries to help you parse command-line options, instead of doing yourself.

You may want to consider using some third-party libraries to help you parse command-line options, instead of doing yourself.

added 432 characters in body
Source Link
h.j.k.
h.j.k.
  • 19.4k
  • 3
  • 37
  • 93

In the methods getMode(Scanner), getKey(Scanner) and getHash(Scanner), they can simply call the appropriate methods to get user input, and throws IOException to propagate any I/O exceptions back to the main() method. Validation errors should be handled within the methods though, so that they can be expected to return one result.

Constant List collections

You are declaring Arrays.asList('A', 'B', 'C', /* ... */ 'Z') repetitively. You can consider putting it as an unmodifiable public static final List<Character> collection, and then only for the case where you want to shuffle it, create a new instance based on it to do so.

In the methods getMode(Scanner), getKey(Scanner) and getHash(Scanner), they can simply call the appropriate methods to get user input, and throws IOException to propagate any I/O exceptions back to the main() method.

In the methods getMode(Scanner), getKey(Scanner) and getHash(Scanner), they can simply call the appropriate methods to get user input, and throws IOException to propagate any I/O exceptions back to the main() method. Validation errors should be handled within the methods though, so that they can be expected to return one result.

Constant List collections

You are declaring Arrays.asList('A', 'B', 'C', /* ... */ 'Z') repetitively. You can consider putting it as an unmodifiable public static final List<Character> collection, and then only for the case where you want to shuffle it, create a new instance based on it to do so.

Source Link
h.j.k.
h.j.k.
  • 19.4k
  • 3
  • 37
  • 93

Indentation and braces style

The indentation is frankly messy, and I don't think it's entirely due to the Markdown formatting.

Also, please be consistent in your braces style. For example:

if(arr.contains(Character.toUpperCase(s.charAt(i))) == false)
    str.append(s.charAt(i));
else {
    index = arr.indexOf(Character.toUpperCase(s.charAt(i)));
    str.append(arr.get(Math.abs(index + key) % arr.size()));
}

There's no need to skim on the extra { and } characters, and without them it makes scanning the code slightly harder, IMHO.

Negating if conditions

if(arr.contains(Character.toUpperCase(s.charAt(i))) == false) { /* ... */ }

That can be better expressed in the following way without using == false:

if(!arr.contains(Character.toUpperCase(s.charAt(i)))) { /* ... */ }

Variables names

In VigenereCaesar:

static String encrypt(String s, String k, int h){ /* ... */ }

It's not immediately obvious what these variables represent. Using the full form is recommended.

Command-line parsing

You may want to consider using some third-party libraries to help you parse command-line options, instead of doing yourself.

try-with-resource

You should also be using try-with-resource once for safe and efficient handling of your Scanner instance and the underlying System.in input stream. For example:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        String mode = getMode(scanner);
        if (mode.equals("encrypt")) {
            String key = getKey(scanner);
            String hash = getHash(scanner);
            // do something with key and hash
        } else if (mode.equals("decrypt")) {
            // similar to above
        } else {
            // Indicate input error
        }
    }
}

In the methods getMode(Scanner), getKey(Scanner) and getHash(Scanner), they can simply call the appropriate methods to get user input, and throws IOException to propagate any I/O exceptions back to the main() method.