Replacing each letter with the letter that is in the corresponding position from the end of the English alphabet

Question

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());
    }
 }

Answer 1

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.

Answer 2

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

Answer 3

        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

• Java Stream .map capitalize first letter only
• Simplest way to print an IntStream as a String

Answer 4

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.