Caesar Cipher MVC implementation

Question

I implemented the Caesar Cipher in Java, and I wanted to practice MVC.

This is my model:

package biz.tugay.caesarcipher;

import java.util.Locale;

/*
    Encyrpts a clear text using Caeser Cipher (https://en.wikipedia.org/wiki/Caesar_cipher)
    with given shift amount.

    Provided shift amount (i.e. key) must be a positive integer less than 26.
    Only English alphabet is supported and encyrpted text will be in uppercase.

    Shift amount 0 will return the same clear text.
 */
public final class CaesarCipher {

    private String clearText;
    private int key;

    public String encryptText() {
        final StringBuilder cipherTextBuilder = new StringBuilder();

        final String clearTextUpperCase = clearText.toUpperCase(Locale.US);
        final char[] clearTextUpperCaseCharArray = clearTextUpperCase.toCharArray();

        for (final char c : clearTextUpperCaseCharArray) {
            if (c < 65 || c > 90) { // If the character is not between A .. Z, append white space.
                cipherTextBuilder.append(" ");
                continue;
            }
            final Character encryptedCharacter = encryptCharacter(c);
            cipherTextBuilder.append(encryptedCharacter);
        }

        return cipherTextBuilder.toString();
    }

    private Character encryptCharacter(final char c) {
        final int initialShift = c + key;
        final int finalShift;

        if (initialShift > 90) {
            // This is the case where we go beyond Z, we must cycle back to A.
            finalShift = (initialShift % 90) + 64;
        } else {
            // We are in the boundries so no need to cycle..
            finalShift = initialShift;
        }

        return (char) finalShift;
    }

    public void setClearText(String clearText) {
        this.clearText = clearText;
    }


    public void setKey(int key) {
        this.key = key;
    }
}

My View:

package biz.tugay;

import java.util.Scanner;

public class CaesarCipherView {

    public String askForClearText() {
        System.out.println("Please enter clear text: ");
        final String clearText = new Scanner(System.in).nextLine();
        return clearText;
    }

    public int askForKey() {
        System.out.println("Please enter a positive integer number between 1 - 25");
        final int key = new Scanner(System.in).nextInt();
        return key;
    }

    public void warnUser() {
        System.out.println("Please enter a valid key value!");
    }

    public void presentEncrpytedText(String encryptText) {
        System.out.println("Here is your encrypted text: ");
        System.out.println(encryptText);
    }
}

The controller:

package biz.tugay;

import biz.tugay.caesarcipher.CaesarCipher;

public class CaesarCipherController {

    private final CaesarCipherView caesarCipherView;
    private final CaesarCipher caesarCipher;

    public CaesarCipherController(final CaesarCipherView caesarCipherView, final CaesarCipher caesarCipher) {
        this.caesarCipherView = caesarCipherView;
        this.caesarCipher = caesarCipher;
    }

    public void getClearTextFromUser() {
        String clearText = caesarCipherView.askForClearText();
        caesarCipher.setClearText(clearText);
    }

    public void getCipherKeyFromUser() {
        int key;
        try {
            key = caesarCipherView.askForKey();
            if (key < 1 || key > 26) {
                caesarCipherView.warnUser();
                getCipherKeyFromUser();
            } else {
                caesarCipher.setKey(key);
            }
        } catch (Exception e) {
            caesarCipherView.warnUser();
            getCipherKeyFromUser();
        }
    }

    public void encryptAndPresent(){
        final String encryptText = caesarCipher.encryptText();
        caesarCipherView.presentEncrpytedText(encryptText);
    }
}

This is how I bootstrap my application:

package biz.tugay;

import biz.tugay.caesarcipher.CaesarCipher;

public class App {
    public static void main(String[] args) {
        final CaesarCipher caesarCipher = new CaesarCipher();
        final CaesarCipherView caesarCipherView = new CaesarCipherView();
        final CaesarCipherController caesarCipherController = new CaesarCipherController(caesarCipherView, caesarCipher);

        while (true) {
            caesarCipherController.getClearTextFromUser();
            caesarCipherController.getCipherKeyFromUser();
            caesarCipherController.encryptAndPresent();
        }
    }
}

Here is a sample run:

Please enter clear text:  KORAY TUGAY Please enter a positive integer
number between 1 - 25 0 Please enter a valid key value! Please enter a
positive integer number between 1 - 25 2 Here is your encrypted text: 
MQTCA VWICA Please enter clear text:    

Is my understanding of Model View Controller correct? Should the controller do the encryption? Do I need a separate model?

Answer 1

Your understanding of MVC seems perfectly fine. You have the business logic in the model. The user input in the view. And lastly a controller to couple the user input to the model and back.

The only problem I have with your program is that it isn't interesting enough. Using MVC here feels too much like you're over-designing things.

What if we had just 2 classes. The CaesarCipher class for the encryption with only this method:

public static String encryptText(String plainText, int key) {...}

And your Viewer class but with one added method that does this:

while(true){
    String text = askForClearText();
    int key = askForKey();
    presentEncrpytedText(CaesarCipher.encryptText(text,key));
}

This still looks pretty decent to me ...

---

It becomes more interesting if we add some behaviour. Let's write a program that does the following things:

The user can choose between 2 things:
1) change the cypher
2) choose to either encode/decode a piece of text with the current cypher.

Now I would slightly change your CaesarCipher class. Let's remove the text from the state and just pass it in as a parameter for the encrypt method and for a new decrypt method.

public class CeasarCypher {
    private int key = 0;

    public String encryptText(String clearText) {
        //encrypt the given text
    }

    public String decryptText(String encryptedText){
        //decrypt the given text
    }

    public void setKey(int key){
        this.key = key;
    }
}

Now we need to make a choice about who initiates anything in your program. Either it's the controller that asks the view for user input. This is what you did.
Or have the view decide what the user has input and then tell this to the controller so it can handle things.

Since we're looking at the overall design, let's first guess a little bit what we want to change in the next versions of the program. One thing I would want to do is later on provide a graphical interface for the user. I would image a single screen that has a textbox to input a text. And 3 buttons for the user choices. A button to change the key (which might popup a new input screen to enter the cypher), a button to encrypt the given text and a last button to decrypt the given tekst.

In this scenario it makes a lot more sense that the view initiates everything. The moment a button is pressed, the view for tells the controller for example: "the user wants to decrypt some tekst".

Let's make sure that we get the full advantage of using MVC. Namely that we can swap to a different view and everything still works.

To be able to do this we need to figure out what methods the Controller has to provide for the View to initiate things. And what methods the view has to provide for the controller to provide feedback.

I would go with something like this:

public class Controller {
    View view;
    Cypher cypher;

    public Controller(View view, Cypher cypher) {
        this.view = view;
        view.setController(this);
        this.cypher = cypher;
    }

    public void changeKey() {
        cypher.setKey(view.getKey());
    }

    public void encryptText(){
        String clearText = view.getClearText();
        String encryptedText = cypher.encrypt(clearText);
        view.presentEncryptedText(encryptedText);
    }

    public void decryptText(){
        String encryptedText = view.getEncryptedText();
        String clearText = cypher.encrypt(clearText);
        view.presentEncryptedText(encryptedText);
    }
}

And the view

public abstract class View {
    public abstract void setController(Controller controller);
    public abstract void presentEncryptedText(String text);
    public abstract void presentDecryptedText(String text);
    public abstract String getEncryptedText();
    public abstract String getDecryptedText();
    public abstract int getKey();
}

Now we can provide either a textual or a graphical view.

For the textual view we can do something like this:

public class TextualView implements View {
    private Controller controller;
    Scanner scanner = new Scanner(System.in);

    public void setController(Controller controller) {
        this.controller = controller;
    }

    public void presentEncryptedText(String text) {
        System.out.println("This is your encrypted text: "+text);
    }

    public void presentDecryptedText(String text) {
        System.out.println("This is your decrypted text: "+text);
    }

    public void mainMenu() {
        boolean userWantsToQuit = false;
        while(!userWantsToQuit) { 
            System.out.println("What would you like to do?"){
            System.out.println("0) Quit");
            System.out.println("1) Change key");
            System.out.println("2) Encrypt message");
            System.out.println("3) Decrypt message");
            int choice = scanner.nextInt();
            switch(choice){
                case 0 : 
                    userWantsToQuit = true;
                    break;
                case 1 :
                    controller.changeKey();
                    break;
                case 2 :
                    controller.encryptMessage();
                    break;
                case 3 :
                    controller.decryptMessage();
                    break;
                default: 
                    System.out.println("Please enter a valid number.");
            }
        }
    }

    public String getEncryptedText(){
         System.out.println("Please enter the text you want to encrypt");
         return scanner.nextLine();
    }

    public String getDecryptedText() {
         System.out.println("Please enter the text you want to decrypt");
         return scanner.nextLine();
    }

    public int getKey() {
         System.out.println("Please enter a new key");
         return scanner.nextInt();
    }
}

Now we have a little bigger program where we have a View that handles all user input/output, a model with the business logic (the encryption/decryption) and a controller to bind the 2.

If we want to use a graphical view instead, we just create a new class to implement one, and pass that to the Controller. If we want to use a different Cypher algorithm, we just create a new class for that and pass that to the Controller. No need to change any of the other classes (including the controller) with either of those 2 changes.

---

---

To end this review, let me point out a few minor details about your current code.

if (key < 1 || key > 26) {

This seems like business logic to me and thus belongs into the CeasarCipher class.

---

Magic numbers:

for (final char c : clearTextUpperCaseCharArray) {  
    if (c < 65 || c > 90) {

Why 65 and 90? What do they mean? Use if( c < 'A' || c > 'Z') instead.

Same here:

    if (initialShift > 90) {
    // This is the case where we go beyond Z, we must cycle back to A.
    finalShift = (initialShift % 90) + 64;

What do the numbers 90 and 64 mean?

---

public void getCipherKeyFromUser() {
    int key;
    try {
        key = caesarCipherView.askForKey();
        if (key < 1 || key > 26) {
            caesarCipherView.warnUser();
            getCipherKeyFromUser();
        } else {
            caesarCipher.setKey(key);
        }
    } catch (Exception e) {
        caesarCipherView.warnUser();
        getCipherKeyFromUser();
    }
}

What happens if the user keeps entering wrong numbers? At a certain point, we get a stackoverflow exception. Java doesn't optimise recursion that well. In this case it might not be so bad since the method contains very little data. But in general in java it's better to use a while loop then recursion. (See my code for an example).

---

For simplicity sake I made this same 'mistake':

final int key = new Scanner(System.in).nextInt();

If the user enters a letter instead of a number, our program will crash. It's probably better to check for valid input first.

---

Constantly creating new Scanner(System.in).

I know you can't close that scanner, because from that point on, you can never get input from System.in anymore either. But then it's probably better to only create the scanner once and reuse it every time you need user input. (See my code for example again).

---

And that's about all I got for you :) I want to stress out that all the things in this post were only minor remarks about the code. Most of it was already pretty good. That's probably also the reason nobody had answered you yet. Good job!