4
\$\begingroup\$

(Everything needed for running the app is here.)

I have refactored my previous version a bit.

App.java:

package net.coderodde.ciphertool;

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.Toolkit;
import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.SwingUtilities;
import net.coderodde.encryption.CipherTools;
import net.coderodde.file.FileTools;

/**
 * This class implements the command line app for encrypting and decrypting 
 * files with a key.
 * 
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Feb 29, 2016) 
 */
public class App {

    private static final String HELP_MESSAGE = 
     "Usage: java -jar File.jar [-e | -d <key> FILE1 [FILE2 [...]]]\n" +
     "Where       -e   encrypt the files.\n" +
     "            -d   decrypt the files.\n" + 
     "         <key>   the key in decimal; " + 
                      "use prefix \"0x\" for hexadecimal.\n" +
     "If you omit all arguments a GUI is started instead.";

    private boolean graphicalInterfaceRequested;
    private boolean printHelpMessage;
    private Mode mode;
    private int key;
    private String[] args;

    public void processArguments(String[] args) {
        this.args = args.clone();

        if (args.length == 0) {
            graphicalInterfaceRequested = true;
            return;
        }

        if (args.length < 3) {
            printHelpMessage = true;
            return;
        }

        switch (args[0]) {
            case "-e":
                mode = Mode.ENCRYPTING;
                break;

            case "-d":
                mode = Mode.DECRYPTING;
                break;
        }

        if (mode == null) {
            printHelpMessage = true;
            return;
        }

        try {
            this.key = parseKey(args[1]);
        } catch (NumberFormatException ex) {
            printHelpMessage = true;
        }
    }

    public boolean helpMessageRequested() {
        return printHelpMessage;
    }

    public void exec() {
        if (graphicalInterfaceRequested) {
            SwingUtilities.invokeLater(() -> { execAsGUI(); });
        } else {
            execAsCommandLine();
        }
    }

    private void execAsGUI() {
        JFrame frame = new JFrame("File cipher tool");
        JButton buttonEncrypt = new JButton("Encrypt");
        JButton buttonDecrypt = new JButton("Decrypt");

        buttonEncrypt.setPreferredSize(new Dimension(200, 40));
        buttonDecrypt.setPreferredSize(new Dimension(200, 40));

        buttonEncrypt.addActionListener(
                new MyActionListener(frame, Mode.ENCRYPTING));

        buttonDecrypt.addActionListener(
                new MyActionListener(frame, Mode.DECRYPTING));

        frame.getContentPane().setLayout(new BorderLayout());
        frame.getContentPane().add(buttonEncrypt, BorderLayout.NORTH);
        frame.getContentPane().add(buttonDecrypt, BorderLayout.SOUTH);

        frame.pack();
        frame.setResizable(false);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        moveToCenter(frame);
        frame.setVisible(true);
    }

    static int parseKey(String keyString) throws NumberFormatException {
        keyString = keyString.trim().toLowerCase();

        if (keyString.length() >= 2
                && (keyString.startsWith("0x")
                || keyString.startsWith("0X"))) {
            return Integer.parseInt(keyString.substring(2), 16);
        } 

        return Integer.parseInt(keyString);
    }

    private static void moveToCenter(JFrame frame) {
        Dimension screen = Toolkit.getDefaultToolkit().getScreenSize();
        frame.setLocation((screen.width  - frame.getWidth())  >> 1,
                          (screen.height - frame.getHeight()) >> 1);
    } 

    private void execAsCommandLine() {
        List<File> fileList = getFileList(args);

        switch (mode) {
            case ENCRYPTING:
                encryptAll(fileList, key);
                break;

            case DECRYPTING:
                decryptAll(fileList, key);
                break;
        }
    }

    static void encryptAll(List<File> fileList, int key) {
        fileList.stream().forEach((File file) -> {
            try {
                byte[] data = FileTools.readFile(file);
                byte[] encryptedData = CipherTools.encrypt(data, key);
                FileTools.writeFile(file, encryptedData);
            } catch (IOException ex) {
                System.err.println("ERROR: " + ex.getMessage());
            }
        });
    }

    static void decryptAll(List<File> fileList, int key) {
        fileList.stream().forEach((File file) -> {
            try {
                byte[] data = FileTools.readFile(file);
                byte[] decryptedData = CipherTools.decrypt(data, key);
                FileTools.writeFile(file, decryptedData);
            } catch (IOException ex) {
                System.err.println("ERROR: " + ex.getMessage());
            }
        });
    }

    private List<File> getFileList(String[] args) {
        // We subtract 2 in order to omit the fist two arguments in 'args' that
        // are the switch and the key.
        List<File> fileList = new ArrayList<>(args.length - 2);

        for (int i = 2; i < args.length; ++i) {
            fileList.add(new File(args[i]));
        }

        return fileList;
    }

    private String getHelpMessage() {
        return HELP_MESSAGE;
    }

    public static void main(String[] args) throws IOException {
        try {
            App app = new App();
            app.processArguments(args);

            if (app.helpMessageRequested()) {
                System.out.println(app.getHelpMessage());
                System.exit(0);
            }

            app.exec();
        } catch (RuntimeException ex) {
            System.err.println("ERROR: " + ex.getMessage());
            System.exit(1);
        }
    }
}

MyActionListener.java:

package net.coderodde.ciphertool;

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.File;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import javax.swing.JFileChooser;
import javax.swing.JFrame;
import javax.swing.JOptionPane;
import static net.coderodde.ciphertool.App.parseKey;

/**
 * This class implements the action listener for the GUI buttons.
 *
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Mar 1, 2016)
 */
final class MyActionListener implements ActionListener {

    private final JFrame ownerFrame;
    private final Mode mode;

    MyActionListener(JFrame ownerFrame, Mode mode) {
        this.mode = Objects.requireNonNull(mode, "The input mode is null.");
        this.ownerFrame = ownerFrame;
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        File[] files = askUserToChooseFiles(ownerFrame);

        if (files == null) {
            return;
        }

        Integer key = askKeyFromUser(
            ownerFrame,
            mode == Mode.ENCRYPTING ?
            "Enter the encryption key. (Use prefix \"0x\" for hexadecimal.)" :
            "Enter the decryption key: (Use prefix \"0x\" for hexadecimal.)");

        if (key == null) {
            return;
        }

        List<File> fileList = Arrays.asList(files);

        switch (mode) {
            case ENCRYPTING:
                App.encryptAll(fileList, key);
                break;

            case DECRYPTING:
                App.decryptAll(fileList, key);
                break;

            default:
                throw new IllegalStateException(
                        "Should not ever get here. Please, debug.");
        }
    }

    private static File[] askUserToChooseFiles(JFrame ownerFrame) {
        JFileChooser chooser = new JFileChooser();
        chooser.setMultiSelectionEnabled(true);
        int status = chooser.showOpenDialog(ownerFrame);

        if (status == JFileChooser.APPROVE_OPTION) {
            return chooser.getSelectedFiles();
        }

        return null;
    }

    private static Integer askKeyFromUser(JFrame ownerFrame, String prompt) {
        String keyString = JOptionPane.showInputDialog(
                                       ownerFrame,
                                       prompt,
                                       "",
                                       JOptionPane.QUESTION_MESSAGE);

        if (keyString == null || keyString.isEmpty()) {
            // User cancelled the entry of the key.
            return null;
        }

        try {
            return parseKey(keyString);
        } catch (NumberFormatException ex) {
            JOptionPane.showMessageDialog(
                    ownerFrame,
                    "\"" + keyString + "\" is an invalid key.",
                    "",
                    JOptionPane.ERROR_MESSAGE);
            return null;
        }
    }
}

Please, tell me anything that comes to mind.

\$\endgroup\$
4
\$\begingroup\$
private boolean graphicalInterfaceRequested;
private boolean printHelpMessage;
private Mode mode;
private int key;
private String[] args;

These most probaby should in no way be fields. Remember: Tell don't ask
Just looking at these I think these should be local variables, optionally you may want to encapsulate them into a "Settings"-class of some kind. They're only valid for one invocation of the Program anyways...


public void processArguments(String[] args) {

You might be interested in something like Apache Commons CLI, a command-line-parser that supports about all possible ways of passing arguments to a program...


    fileList.stream().forEach((File file) -> {
        try {
            byte[] data = FileTools.readFile(file);
            byte[] encryptedData = CipherTools.encrypt(data, key);

Since you obviously have Java 8 available I can't help but wonder why you use the old-fashioned File-API.

Get used to using Path:

   fileList.stream().forEach(path -> {
       try {
           byte[] data = Files.readAllBytes(path);
           byte[] encryptedData = CipherTools.encrypt(data, key);

Kiss your "roll your own"-class goodbye :)


private List<File> getFileList(String[] args) {
    // We subtract 2 in order to omit the fist two arguments in 'args' that
    // are the switch and the key.
    List<File> fileList = new ArrayList<>(args.length - 2);

    for (int i = 2; i < args.length; ++i) {
        fileList.add(new File(args[i]));
    }

    return fileList;
}

Why so complicated?

private List<Path> getFileList(String[] args) {
    return Arrays.stream(args).skip(2).map(Paths::get).collect(Collectors.toList());
}

        if (app.helpMessageRequested()) {
            System.out.println(app.getHelpMessage());
            System.exit(0);
        }

        app.exec();

This shouldn't be main's responsibility. Encapsulate your responsibilites, pass the arguments to App's constructor and just say "run":

public static void main (String[] args) {
    App app = new App(args);
    app.run();
}

also: Any exceptions that bubble beyond main are automatically printed to stdout, including a stacktrace. The try-block is really unnecessary...


MyActionListener(JFrame ownerFrame, Mode mode) {
    this.mode = Objects.requireNonNull(mode, "The input mode is null.");
    this.ownerFrame = ownerFrame;
}

Why are you checking the mode, but not the ownerFrame? Why are you even checking the mode at all?... It doesn't make sense to completely mistrust one of two arguments (you both control) in a constructor for a listener...

Either distrust both, or trust both arguments. Since you have the total control over calling code, I expect to see the latter...

Also why is this constructor default-visible? Either you completely decapsulate your GUI (including moving the GUI out of App.java), or you keep it all in one file. I'd recommend the former.


    File[] files = askUserToChooseFiles(ownerFrame);

    if (files == null) {
        return;
    }

You actually didn't encapsulate the possibility that your User doesn't select any files into askUserToChooseFiles? null is a serious pain in the backside to maintenance. AVOID IT when you reasonably can. Or rather. DON'T USE IT, unless you actually cannot avoid it.

Same thing for askKeyFromUser...


    List<File> fileList = Arrays.asList(files);

Encapsulation failure strikes again. Just return a List from askUserToChooseFiles.. that makes the whole thing incredibly much easier.

\$\endgroup\$
  • \$\begingroup\$ Good answer; I'd disagree with strongly discouraging null use here, as there's no strong case to make for it in this instance; I also think that altering the encryptAll and decryptAll arguments to take Path instead of File is a non-significant change and you ought to put helpers for converting from File to Path to preserve the public API, but then again, as a standalone program that's not a problem and thus a moot point. \$\endgroup\$ – Pimgd Mar 21 '16 at 13:47

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.