13
\$\begingroup\$

Since I am new to Java, I wanted to see if there were better ways of answering the same question. Are parts of my code redundant or maybe there's an easier way to get the same result?

Problem J4: Big Bang Secrets

The encoding algorithm is a Caesar cipher with a shift (\$S\$) that depends on a parameter (\$K\$) and its position in the word (\$P\$, where \$P = 1\$ for the first letter of each word): \$S = 3\$, \$P + K\$. For example, when \$K = 3\$, ZOOM is encoded as FXAB:

  • \$S_1 = 3 \cdot 1 + 3 = 6\$, so Z \$\rightarrow\$ F
  • \$S_2 = 3 \cdot 2 + 3 = 9\$, so O \$\rightarrow\$ X
  • \$S_3 = 3 \cdot 3 + 3 = 12\$, so O \$\rightarrow\$ A
  • \$S_4 = 3 \cdot 4 + 3 = 15\$, so M \$\rightarrow\$ B

The challenge is to write a decoder. The first line of input contains \$K\$ (\$K \lt 10\$). The second line contains the encoded message, containing up to 20 characters in uppercase.

public class Decoder {

public static void main(String[] args) {
    try {

        BufferedReader in = new BufferedReader(new InputStreamReader(
                System.in));
        int k = Integer.parseInt(in.readLine());
        String word = in.readLine();

        char[] cArray = word.toCharArray();

        for (int i = 0; i < word.length(); i++) {

            char out = (char) ((cArray[i]) - (((3 * (i + 1)) + k) % ('Z' - 'A')));

            if (out < 'A') {
                char wrap = (char) (('Z' + 1) - ('A' - out));
                System.out.print(wrap);
            } else {

                System.out.print(out);
            }
        }

    } catch (IOException e) {

        System.out.println("Error");

    }
}
}
\$\endgroup\$

3 Answers 3

10
\$\begingroup\$

Your program is great in that it does the job, but there are some ways to improve the implementation significantly.

As a design decision, some programs need to process data as it is being captured from the inputs. This program is not one of those, so it makes sense to make the input and processing steps very discrete.

Then, using some of the more modern Java features (the ones that have been there since Java7), you should use a try-with-resources statement to manage the input exceptions.

I would also recommend creating a container and calling method to handle the two-part inputs (the key, and the encrypted text).

Sometimes it's easier to show, than to tell. Let's start with the core decode routine. The two methods should make sense:

public static final String decode(final int key, final String encoded) {
    final int len = encoded.length();
    // prepare a space to store the decoded value
    final char[] decoded = new char[len];
    for (int i = 0; i < len; i++) {
        // use i+1 here since the algorithm has the first char at position 1, not 0.
        decoded[i] = decodeChar(encoded.charAt(i), i + 1, key);
    }
    // convert the decoded chars back to a String.
    return new String(decoded);
}

private static char decodeChar(final char encoded, final int position, final int key) {
    // modulo 26 eliminates multiple wrap-arounds.
    int rotate = (3 * position + key) % 26;
    // apply the rotation shift to the input
    int decval = (encoded - 'A') + 26 - rotate;
    // use another % 26 to keep the letters in range.
    return (char)('A' + (decval % 26));
}

Note how using the function extraction makes the code simpler?

Now, how to get the input in to that function? That would be simple:

public static void main(String[] args) {
    CypherText input = getEncoded();
    String decoded = decode(input.getKey(), input.getText());
    System.out.println("Decoded: " + decoded);
}

Note, that CypherText class is new.... but you can clearly see how the decode(...) would fit. Biw is the CypherText done?

private static final class CypherText {
    private final int key;
    private final String text;

    public CypherText(int key, String text) {
        super();
        this.key = key;
        this.text = text;
    }

    public int getKey() {
        return key;
    }

    public String getText() {
        return text;
    }

}

public static CypherText getEncoded() {
    try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))){
        int k = Integer.parseInt(reader.readLine());
        String word = reader.readLine();
        return new CypherText(k, word);
    } catch (IOException e) {
        e.printStackTrace();
        return new CypherText(0, "");
    }
}

I put this in an Ideone here

\$\endgroup\$
0
5
\$\begingroup\$

I'll make the following suggestions:

  • Use Scanner for a slightly more convenient interface than new BufferedReader(new InputStreamReader(…)).
  • For readability, split up the work into
    • Input / output
    • Instantiating a cipher for parameter K
    • Calculating how much to shift an individual letter
    • Applying the shift for an individual letter
  • Use modulo arithmetic to eliminate special cases.
  • Print the result all at once as a new String(…).
  • If you need to print an error message, print it to System.err, to avoid contaminating System.out, where it would be interpreted as legitimate output.

Suggested implementation

import java.util.Scanner;

public class BigBangCipher {
    private int k;

    public BigBangCipher(int k) {
        this.k = k;
    }

    /**
     * Shift for a character, given a 0-based index.
     */
    private int shift(int pos) {
        return 3 * (pos + 1) + this.k;
    }

    /**
     * Applies a Caesar cipher shift for an uppercase character.
     */
    private static char caesar(char c, int shift) {
        return (char)('A' + (c + shift - 'A' + 26) % 26);
    }

    public String decode(CharSequence cipherText) {
        char[] s = new char[cipherText.length()];
        for (int i = 0; i < s.length; i++) {
            s[i] = caesar(cipherText.charAt(i), -this.shift(i));
        }
        return new String(s);
    }

    public static void main(String[] args) {
        try (Scanner in = new Scanner(System.in)) {
            int k = Integer.parseInt(in.nextLine());
            BigBangCipher cipher = new BigBangCipher(k);
            System.out.println(cipher.decode(in.nextLine()));
        }
    }
}
\$\endgroup\$
5
\$\begingroup\$

Your solution seems generally sound. I would make a few changes:

You are surrounding your whole code in a try-catch block. Instead, surround the area where the IOException has a chance of being thrown (Also, use System.err to print for errors):

try {
    BufferedReader in = new BufferedReader(new InputStreamReader(
            System.in));
} catch (IOException) {
    System.err.println("Error");
}

And since the in is inside the try-catch and it is required outside, do:

BufferedReader in = null;
try {
    in = new BufferedReader(new InputStreamReader(
            System.in));
} catch (IOException) {
    System.err.println("Error");
    return;
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ If an IOException occurs anywhere (for example, during in.readLine()), you do want the flow of control to skip over the entire body of the program. Therefore, enclosing all of the contents of the main() function in the try block was the right thing to do. \$\endgroup\$ Jan 29, 2015 at 8:41
  • \$\begingroup\$ @200_success That's why return was over there. \$\endgroup\$ Jan 29, 2015 at 17:17
  • \$\begingroup\$ But putting the catch at the end makes it obvious that an exception will make the flow jump to the end. \$\endgroup\$ Jan 29, 2015 at 17:57
  • \$\begingroup\$ @200_success How about System.exit(1)? \$\endgroup\$ Jan 29, 2015 at 18:01
  • \$\begingroup\$ don's use System.exit() in called code. Instead you should probably early return an error value and let calling code handle it. This seems to be the generally better option here, too. \$\endgroup\$
    – Vogel612
    Jan 29, 2015 at 18:32

Not the answer you're looking for? Browse other questions tagged or ask your own question.