8
\$\begingroup\$

I wrote a small program for hyperskill/jetbrains academy to encrypt the message "we found a treasure!" and print only the ciphertext (in lower case) by replacing each letter with the letter that is in the corresponding position from the end of the English alphabet (a→z, b→y, c→x, ... x→c, y →b, z→a) and without replace spaces or the exclamation mark.

I would like code review comments in terms of correct use of java.

package encryptdecrypt;

public class Main {

    final static int lower_case_a = 'a';
    final static int lower_case_z = 'z';

    public static void main(String[] args) {
        String s = "we found a treasure!";

        StringBuffer reverse = new StringBuffer();

        for (int i = 0; i < s.length(); i++) {
            char c = s.charAt(i);

            if ((int)c >= lower_case_a && (int)c <= lower_case_z)
                reverse.append((char)(lower_case_z - c + lower_case_a));
            else
                reverse.append((char) c);
        }
        System.out.println(reverse.toString());
    }
 }
\$\endgroup\$
  • 6
    \$\begingroup\$ FWIW, what you've implemented is commonly known as the Atbash cipher (using the 26-letter English alphabet), in case anyone wants to Google for it. \$\endgroup\$ – Ilmari Karonen Oct 14 at 0:05
7
\$\begingroup\$

With

final static int lower_case_a = 'a';
final static int lower_case_z = 'z';

While it's arguably good that you have 'a' and 'z' stored as variables and not floating around, I don't think they're named well. lower_case_a tells you exactly as much as 'a' itself, so you aren't really gaining much. The purpose of the variable would make for a better name. Also, since those are constants, they should be upper case, and underscore-separated. I'd change those lines to:

final static int ALPHABET_LOWER_BOUND = 'a';
final static int ALPHABET_UPPER_BOUND = 'z';

Also, as mentioned in the above link, Java uses camelCase, not snake_case.


if ((int)c >= lower_case_a && (int)c <= lower_case_z)
    reverse.append((char)(lower_case_z - c + lower_case_a));
else
    reverse.append((char) c);

This has a few notable things:

  • Do not omit braces here. I won't go as far as to say that they should never be omitted, but the cases where it's appropriate to not use them are pretty slim. If you ever make changes to those lines and don't notice that you didn't use braces, at best you'll need to deal with syntax errors, and at worst you'll get weird behavior that you'll need to debug. Just use them. It costs next to nothing and prevents a range of errors.

  • Instead of writing (int)c in a few places, I'd probably instead save that in a variable.

  • (char) c isn't necessary. c is already a char.

  • You could use a ternary since each branch is just giving its data to append.

Something closer to:

int code = c;
char newChar = (code >= ALPHABET_LOWER_BOUND && code <= ALPHABET_UPPER_BOUND)
                 ? (ALPHABET_UPPER_BOUND - c + ALPHABET_LOWER_BOUND) : c;

reverse.append(newChar);

Although whether you'd want to use a ternary here is subjective. That line is a little long.

I also agree with @dariosicily though. You never use i for anything other than indexing, so you might as well just use a foreach loop and iterate the string directly.

\$\endgroup\$
  • 4
    \$\begingroup\$ Nit: you have "lower bound" and "upper bound" the wrong way around; code > ALPHABET_UPPER_BOUND would mean "code is above the upper bound, i.e. outside the accepted range". What we want to say is "code is equal to or higher than the lowest allowed value", so code >= ALPHABET_LOWER_BOUND. \$\endgroup\$ – IMSoP Oct 14 at 9:25
  • \$\begingroup\$ Also you didn't replace lower_case_[a/z] here: (lower_case_z - c + lower_case_a) \$\endgroup\$ – Rafalon Oct 14 at 13:35
  • \$\begingroup\$ Fixed both. Thanks guys. \$\endgroup\$ – Carcigenicate Oct 14 at 16:04
4
\$\begingroup\$

A few points:

  • Constants are always written in uppercase in Java
  • If you are not using those constants anywhere else, make them private
  • Create a method to do your reversal (e.g. public String applyCipher(String input) {...}). This way, you can write unit tests for it, confirming all corner cases of your algorithm (like leaving alone the letters not in that range, capital letters, digits, punctuation) and proving that your code works
  • Use for (char c: input.toCharArray()) {...}, as you don't need the index of the character
  • Never use if blocks without brackets. (see https://stackoverflow.com/a/2125078/671543)
  • Character class has a getType, which can tell you that a char is a lowercase letter. Character.getType(c) == Character.LOWERCASE_LETTER LOWERCASE_LETTER contains way more than the English alphabet, so don't do that
  • To avoid char arithmetic, you can do something like this:
String alphabet = "abcdefghijklmnopqrstuvwxyz";
String reversed = new StringBuilder(alphabet).reverse().toString();
Map<Character, Character> table = IntStream
    .range(0, alphabet.size())
    .boxed()
    .collect(Collectors.toMap(alphabet::charAt, reversed::charAt));

and use the table to map your characters directly

\$\endgroup\$
  • 1
    \$\begingroup\$ The last point is a really bad idea, since Character.LOWERCASE_LETTER is something very different to [a-z]. If you wanted to handle unicode that would require a whole lot more than simply using that character class. \$\endgroup\$ – Voo Oct 15 at 12:31
  • \$\begingroup\$ @Voo good point, the OP specifies "english alphabet". I don't like the idea of treating chars as numbers, though, so I'll suggest something else \$\endgroup\$ – njzk2 Oct 16 at 1:36
3
\$\begingroup\$
        StringBuffer reverse = new StringBuffer();

A StringBuffer is only needed if you are going to access the same variable from multiple threads. Your program is single-threaded, so you're not doing that. (Even in multi-threaded programs, you may not use the variable across threads. But here you are simply singled-threaded.) Just use a StringBuilder instead.

        StringBuilder ciphered = new StringBuilder(s.length());

The StringBuilder has the same interface as the StringBuffer, but it is more efficient. It doesn't waste time synchronizing the accesses.

I don't like the name reverse, as to me it implies changing the order of the characters rather than their place in the alphabet.

Since you know the length of the string that you are building, you might as well tell the constructor at initialization. That way it can allocate the correct amount of memory once rather than going through repeated resizing operations.

Java 8 Streams

If you are using Java 8 or later, you could do this with a Stream instead. Something like

String ciphered = s.codePoints()
                   .map(c -> (c >= 'a' && c <= 'z') ? 'a' + 'z' - c : c)
                   .collect(StringBuilder::new,
                            StringBuilder::appendCodePoint,
                            StringBuilder::append)
                   .toString();

I removed the named constants as they actively harm readability. And renamed I'm not sure about the functionality, as this only works for contiguous alphabets. In my opinion, this works better as being strictly limited to the Latin1 alphabet. For the same reason, I don't use Character.isLowerCase. That will attempt to apply the Latin1 transformation to letters from other alphabets, which simply won't work.

See also

\$\endgroup\$
2
\$\begingroup\$

Some my suggestions about your code: instead of iterating over the string s because you are not using the i index inside your loop you could iterate over the char array from s like below:

char[] arr = s.toCharArray();
for (char c : arr) { //here the body }

Inside the loop you can use Character.isLowerCase method and you can use a ternary operator to append your character to the StringBuffer evaluating if it is a lowercase character or not:

String s = "we found a treasure!";
char[] arr = s.toCharArray();
StringBuffer reverse = new StringBuffer();

for (char c : arr) {
    char ch = Character.isLowerCase(c) ? (char)(219 - c) : c; //<-- 219 = int('a') + int('z')
    reverse.append(ch);
}

Inside the loop I put 219 because it is a const value so it is not useful recalculate it for every iteration of the loop.

Note: due to the right observations I decided to strike part of my code solution, so it remains just the code mentioned in other answers.

\$\endgroup\$
  • 3
    \$\begingroup\$ I'm pretty sure the compiler is smart enough to realize that additions of constants can be replaced by an actual constant. The loss in readability is not worth it. \$\endgroup\$ – njzk2 Oct 13 at 19:51
  • \$\begingroup\$ @njzk2 I'm agree, I put a comment to explain why I'm using directly a number but probably it would have been better create a local variable as sum of a and z and use the variable in the loop. The optimal idea would be create a method that returns the expected string as you thought in your answer to make easier the tests. \$\endgroup\$ – dariosicily Oct 13 at 20:31
  • \$\begingroup\$ @downvoter, please explain the reason for downvote. \$\endgroup\$ – dariosicily Oct 14 at 20:19
  • \$\begingroup\$ The previous comment already explained it I thought: a) this is the mother of all premature optimization and b) the compiler is more than capable of hoisting the computation out of the loop so it's not even an optimization. There simply is no reason whatsoever to ever do this here. Even if you had a compiler from the 70s that couldn't do this, the performance hit would be completely unmeasurable. \$\endgroup\$ – Voo Oct 15 at 12:13
  • 1
    \$\begingroup\$ (There's also the additional problem that the code introduces an error contrary to the original code, since Character.isLowerCase handles Unicode but the rest of the code assumes just simple ASCII) \$\endgroup\$ – Voo Oct 15 at 12:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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