Rot -n algorithm in Java

Question

This is a rot-n (rotate n times) encrypt/decrypt algorithm, where n can be any integer (+/-).

It's written to encrypt/decrypt all the alphabets (upper/lower case) and leaving behind all non-alphas.

rot-n where n is the key, is given in the first line of input file.

Everything is working fine. I just need help in optimizing it.

Challenge description

Your submission will be tested against an input file that contains ASCII characters. The input file starts at the very first line with a N-digits positive or negative integer number that is your cipher, followed by a new-line character. The second line starts with the payload of the encrypted text until the end of file. Note that only words (alphanumeric sequences) are encrypted and that every other character (i.e. punctuation) must not be processed by your algorithm and must also be copied ‘as is’ to the standard output.*

import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.util.Scanner;
public class Fightsopa {
public void process(String input, int n) {
 int abcd = n;
 if (abcd < 0) {
 abcd = (26 - ((0 - abcd) % 26));
 }
 int rot = 26 - abcd % 26; //for decrypting
 // int rot= abcd % 26; //for encrypting
 char out = 0;
 for (char ch : input.toCharArray()) {
 if (ch >= 65 && ch <= 90) {
 if (ch + rot <= 90) {
 out = (char) (ch + rot);
 System.out.print(out);
 } else {
 out = (char) (ch + rot - 26);
 System.out.print(out);
 }
 } else if (ch >= 97 && ch <= 122) {
 if (ch + rot <= 122) {
 out = (char) (ch + rot);
 System.out.print(out);
 } else {
 out = (char) (ch + rot - 26);
 System.out.print(out);
 }
 } else {
 System.out.print(ch);
 }
 }
}
public void handler(File fFile) throws FileNotFoundException {
 Scanner scanner = new Scanner(new FileReader(fFile));
 int rot = 0;
 int count = 0;
 while (scanner.hasNextLine()) {
 if (count == 0) {
 rot = scanner.nextInt();
 scanner.nextLine();
 } else {
 String wordp = scanner.nextLine();
 process(wordp, rot);
 System.out.println();
 }
 count++;
 }
 scanner.close();
}
public static void main(String args[]) throws FileNotFoundException {
 if (args.length > 0) {
 File fFile = new File(args[0]); //Input file having the first line as the key 
 Fightsopa fsopa = new Fightsopa();
 fsopa.handler(fFile);
 }
}
}

Answer 1

I agree with Landei, try separating the user interface from the logic. As far as I see your code doesn't handle digits (alphanumeric includes digits as well), so maybe you want to fix it. It's not clear how to rotate digits, but I prefer Landei's map if there is not any special requirement. Anyway, I didn't rewrite the core logic in the following notes.

1. I'd start with extracting out a method:

   final StringBuilder result = new StringBuilder();
   for (final char ch : input.toCharArray()) {
    final char out = doRotate(ch, rotation);
    result.append(out);
   }
   System.out.println(result);
   public char doRotate(final char ch, final int rot) {
    if (ch >= CHARCODE_UPPER_A && ch <= CHARCODE_UPPER_Z) {
    if (ch + rot <= CHARCODE_UPPER_Z) {
    return (char) (ch + rot);
    } else {
    return (char) (ch + rot - 26);
    }
    } else if (ch >= CHARCODE_LOWER_A && ch <= CHARCODE_LOWER_Z) {
    if (ch + rot <= CHARCODE_LOWER_Z) {
    return (char) (ch + rot);
    } else {
    return (char) (ch + rot - 26);
    }
    }
    return ch;
   }
   

   Note the removed last else branch which was unnecessary. return 0 is enough. It removes lots of duplicated System.out.println calls. (Maybe one time you want to write the results to a network socket or to a file.) Furthermore, it makes testing easier.

   @RunWith(Parameterized.class)
   public class FightsopaTest extends Fightsopa {
    private final char expectedChar;
    private final char inputChar;
    private final int rotate;
    public FightsopaTest(final char expectedChar, final char inputChar, 
    final int rotate) {
    this.expectedChar = expectedChar;
    this.inputChar = inputChar;
    this.rotate = rotate;
    }
    @Parameters
    public static Collection<Object[]> data() {
    final Object[][] data = new Object[][] { 
    { 'b', 'a', 1 }, 
    { 'a', 'z', 1 }, 
    { 'a', 'a', 26 }, 
    { 'b', 'a', 27 }, 
    };
    return Arrays.asList(data);
    }
    @Test
    public void testRotate() throws Exception {
    final char value = new Fightsopa().doRotate(inputChar, rotate);
    assertEquals(expectedChar, value);
    }
   }
   

   (Of course it could have been written as a single testRotate method with lots of assert calls but this wouldn't enable defect localization.)

2. I'd rename the process to processLine and extract out a normalizeRotation method too.

   public String processLine(final String input, final int rotation) {
    final StringBuilder result = new StringBuilder();
    for (final char ch : input.toCharArray()) {
    final char out = doRotate(ch, rotation);
    result.append(out);
    }
    return result.toString();
   }
   private int normalizeRotation(final int rotation) {
    int abcd = rotation;
    if (abcd < 0) {
    abcd = (26 - ((0 - abcd) % 26));
    }
    final int normalizedRotation= abcd % 26; //for encrypting
    return normalizedRotation;
   }
   

   Actually, this normalization logic should be run only once, right after the reading of the rotation value from the input file (see later).

3. Using guard clauses makes the code flatten and more readable.

   public static void main(final String args[]) throws FileNotFoundException {
    if (args.length != 1) {
    System.err.println("Missing parameter.");
    return;
    }
   

   References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code

4. Use variable names which explains the purpose, for example:

   final File inputFile = new File(args[0]);
   

5. Method names should be verbs, handler should be handle. (Check Code Conventions for the Java Programming Language, Naming Conventions.)

6. The handle method doesn't close the Scanner (and the underlying FileReader) in all execution paths. Put the close in a finally block:

   final Scanner scanner = new Scanner(new FileReader(inputFile));
   try {
    ...
   } finally {
    scanner.close();
   }
   

7. I'd break down the handle method to two methods: a readRotation and a processFile. I makes the count variable unnecessary, omits the if (inside the while loop) and makes the code more readable. (Here is the normalizeRotation call.)

   public String handle(final File inputFile) 
    throws FileNotFoundException, InputErrorException {
    final Scanner scanner = new Scanner(new FileReader(inputFile));
    try {
    final int rotation = readRotation(scanner);
    final int normalizedRotation = normalizeRotation(rotation);
    return processFile(scanner, normalizedRotation);
    } finally {
    scanner.close();
    }
   }
   private int readRotation(final Scanner scanner) throws InputErrorException {
    if (!scanner.hasNext()) {
    throw new InputErrorException("Unable to read rotation.");
    }
    final int rotation = scanner.nextInt();
    scanner.nextLine();
    if (!scanner.hasNext()) {
    throw new InputErrorException("Unable to read rotation.");
    }
    return rotation;
   }
   private String processFile(final Scanner scanner, final int rot) {
    final StringBuilder result = new StringBuilder();
    while (scanner.hasNextLine()) {
    final String line = scanner.nextLine();
    final String processedLine = processLine(line, rot);
    result.append(processedLine);
    result.append("\n");
    }
    return result.toString();
   }
   

8. Currently, the Fightsopa still violates the Single Responsibility Principle. It does the file processing and the encrypting too. It would be loosely coupled if a separate RotateCoder class did the encoding and Fightsopa class did the file processing.

   public class RotateCoder {
    private static final int CHARCODE_LOWER_A = 97;
    private static final int CHARCODE_LOWER_Z = 122;
    private static final int CHARCODE_UPPER_A = 65;
    private static final int CHARCODE_UPPER_Z = 90;
    private final int normalizedRotation;
    public RotateCoder(final String config) {
    final int rotation = Integer.parseInt(config);
    normalizedRotation = normalizeRotation(rotation);
    }
    private int normalizeRotation(final int rotation) {
    int abcd = rotation;
    if (abcd < 0) {
    abcd = (26 - ((0 - abcd) % 26));
    }
    final int normalizedRotation = abcd % 26; // for encrypting
    return normalizedRotation;
    }
    public char encrypt(char ch) {
    if (ch >= CHARCODE_UPPER_A && ch <= CHARCODE_UPPER_Z) {
    if (ch + normalizedRotation <= CHARCODE_UPPER_Z) {
    return (char) (ch + normalizedRotation);
    } else {
    return (char) (ch + normalizedRotation - 26);
    }
    } else if (ch >= CHARCODE_LOWER_A && ch <= CHARCODE_LOWER_Z) {
    if (ch + normalizedRotation <= CHARCODE_LOWER_Z) {
    return (char) (ch + normalizedRotation);
    } else {
    return (char) (ch + normalizedRotation - 26);
    }
    }
    return ch;
    }
   }
   

   Fightsopa:

   public class Fightsopa {
    public Fightsopa() {
    }
    public String processLine(final String input, final RotateCoder coder) {
    final StringBuilder result = new StringBuilder();
    for (final char ch : input.toCharArray()) {
    final char out = coder.encrypt(ch);
    result.append(out);
    }
    return result.toString();
    }
    public String handle(final File inputFile) 
    throws FileNotFoundException, InputErrorException {
    final Scanner scanner = new Scanner(new FileReader(inputFile));
    try {
    final RotateCoder coder = createCoder(scanner);
    return processFile(scanner, coder);
    } finally {
    scanner.close();
    }
    }
    private RotateCoder createCoder(final Scanner scanner) 
    throws InputErrorException {
    if (!scanner.hasNext()) {
    throw new InputErrorException("Unable to read rotation.");
    }
    final String coderConfig = scanner.nextLine();
    return new RotateCoder(coderConfig);
    }
    private String processFile(final Scanner scanner, final RotateCoder coder) {
    final StringBuilder result = new StringBuilder();
    while (scanner.hasNextLine()) {
    final String line = scanner.nextLine();
    final String processedLine = processLine(line, coder);
    result.append(processedLine);
    result.append("\n");
    }
    return result.toString();
    }
    public static void main(final String args[]) 
    throws FileNotFoundException, InputErrorException {
    if (args.length != 1) {
    System.err.println("Missing parameter.");
    return;
    }
    final File inputFile = new File(args[0]);
    final Fightsopa fsopa = new Fightsopa();
    final String result = fsopa.handle(inputFile);
    System.out.println(result);
    }
   }
   

9. The main method should be in a separate class (because of the Single Responsibility Principle again) and it should catch the exceptions. Instead of printing the full stacktrace, prints only the message and log (with a logger) the details to a log file.

• cypronmaya: One word,about your's- Awesome So i should improve 1) Naming conventions 2) Loose coupling 3) Normalization logic should be run only once(better to be in constructor) 4) Using Guard Clauses 5) Closing scanner's etc., Thanks for taking time and breaking down into parts, that helped me a lot. I just wrote the code ,thinking only to solve. But for a programmer that's not only thing, he should be able to write clean,and best code :)
• cypronmaya: can we do anything about reducing Cyclomatic Complexity 7.000
• palacsint: 7000? Which method?
• cypronmaya: not 7000, it's 7 for whole program,it's mostly because of encrypt method i think
• palacsint: 7 isn't too much. Anyway, I agree, encrypt definitely isn't the most beautiful part of the code. I thought that you will rewrite it as Landei suggested, so didn't care too much about this method.

Answer 2

First, your process method violates the Single Responsibility Principle: It calculates something, and it prints it out. Such coupling makes it inflexible. It's better to return a String, and letting the caller decide what to do with it.

Second, you're code is way too complicated, handling all the different cases which are very dependent on the ASCII layout. Imagine next you want to support digits as well - oh my...

I would suggest to do something along the lines...

public String process(String input, int n) {
 String abc = "abcdefghijklmnopqrstuvwxyz";
 String abcABC = abc + abc.toUpperCase();
 int len = abcABC.length();
 Map<Character, Character> map = new HashMap<Character, Character>();
 for (int i = 0; i < len; i++) {
 map.put(abcABC.charAt(i), abcABC.charAt((i + n + len) % len));
 }
 StringBuilder sb = new StringBuilder();
 for(int i = 0; i < input.length(); i++) {
 Character ch = map.get(input.charAt(i));
 if (ch == null) {
 throw new IllegalArgumentException("Illegal character " + input.charAt(i));
 }
 sb.append(ch);
 }
 return sb.toString();
}

Note that I need to know nothing about ASCII encoding using that approach, so supporting other chars is a no-brainer.

• cypronmaya: @Second suggestion : I've wrote it to achieve the puzzle description stated above.I've made it depend on ASCII layout only because of the puzzle. ---If i've wanted to support all ascii set private String charset; private int len; sampa() { StringBuilder sb = new StringBuilder(); for(int i = 0; i < 128; i++) { sb.append((char)i); } charset=sb.toString(); len=charset.length(); } Can u suggest me ,what changes i could do to my code to make it optimized.
• cypronmaya: BTW, your code can only do rot only with in a range i.e (+/-)abcABC.length()