2
\$\begingroup\$

An easy, terribly written but working, Caeasar (Monoalphabetic) cipher implementation, with a GUI (Graphical User Interface).

Cipher.java

class Cipher {

    static final String alphabet = "abcdefghijklmnopqrstuvwxyz";

    public static char shift(char ch, int amount) {
        return alphabet.charAt((alphabet.indexOf(ch) + amount) % alphabet.length());
    }

    public static String caesarCipher(String text, int amount) {
        String result = "";
        for (char ch : text.toCharArray()) {
            if (alphabet.indexOf(ch) >= 0) {
                result += shift(ch, amount);
            }
        }
        return result;
    }

    public static void main(String[] args) {
        assert shift('a', 1) == 'b';
        assert shift('z', 2) == 'b';
        assert caesarCipher("abc", 2).equals("cde");
    }
}

Gui.java

import javax.swing.*;        
import java.awt.*;
import java.awt.event.*;

public class Gui {

    private static void createAndShowGUI() {
        //Create and set up the window.
        JFrame frame = new JFrame("HelloWorldSwing");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

        JPanel body = new JPanel();

        final JTextField label = new JTextField("aaaaaaaaaaaaaaaaaaaaaaa");
        body.add(label);

        final JTextField key = new JTextField("1");
        body.add(key);

        final JTextField result = new JTextField("bbbbbbbbbbbbbbbbbbbbbb");


        JButton activationButton = new JButton("Encrypt");
        activationButton.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent ae)
            {
                String plainText = label.getText();
                String keyval = key.getText();
                result.setText(Cipher.caesarCipher(plainText, Integer.parseInt(keyval)));
            }
        });

        body.add(activationButton);

        body.add(result);

        frame.add(body);

        //Display the window.
        frame.pack();
        frame.setVisible(true);
    }

    public static void main(String[] args) {
        javax.swing.SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                createAndShowGUI();
            }
        });
    }
}
\$\endgroup\$
3
\$\begingroup\$

Input validation

It would be good to validate user input. If I enter "hello" as the key value, The UI won't like it very much.

Shifting

An idea to simplify the shifting. Instead of taking modulo for each character, double the alphabet (concatenate the original alphabet twice), and normalize the key value. The solution will use twice the space for the alphabet, but the shifting will be much faster without the modulo.

Skipping unknown characters

The current algorithm checks if a character is in the alphabet, skipping without translation if it's not. So if you convert an English sentence with space characters, the word boundaries will be lost, which will be a problem when reversing the process.

Finding the index twice

The current algorithm checks is in the alphabet, if yes it calls another to perform the shift. That method finds the index again for the second time, to calculate the shifted index. This second lookup could be eliminated if you rename "shift" to "translate" and change its behavior to lookup the index once and reuse it.

Dummy data

Instead of hardcoding the dummy result it would be better to compute it using the cipher.

Naming constants

The convention is to use SHOUTCASE for constants like the alphabet.

Testing

A bunch of assertions in a main method are no substitute for proper unit testing.

\$\endgroup\$
  • \$\begingroup\$ Input validation <- not a Review of the code, just a possible improvement. Shifting <- using the modulo (%) makes the algorithm more clear, I am not into micro-optimizations. Skipping unknown characters <- this cipher is already totally insecure, not skipping spaces would make it even more trivial to crack. Finding the index twice <- Are you really suggesting such micro-micro optimization? Dummy data <- I do not think I will mess up the examples, it is not worth the added complexity. Naming constants <- I already used the const keyword (yeah sticking to standards is sensible after-all.) \$\endgroup\$ – Caridorc Sep 17 '15 at 10:34
  • \$\begingroup\$ Testing <- A bunch of assertions are all that is needed for so trivial code, if complexity increases, I will switch to Unit-Tests. \$\endgroup\$ – Caridorc Sep 17 '15 at 10:35
  • \$\begingroup\$ Validating untrusted input is extremely important. It's no accident I wrote this point at the very top. Such comment is very valuable in a code review. Avoiding to find the index twice is not a micro-optimization, and yes, I encourage you to get rid of it. Dummy data: there's a saying "you play like you practice". You have a method to generate it, it makes sense to use it. \$\endgroup\$ – Stop ongoing harm to Monica Sep 17 '15 at 10:51

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.