5
\$\begingroup\$

I have created an Encryption and Decryption Algorithm that encrypts plain text using a given cipher and also decrypts it back to the original notation. It works great.

This was my code for a coding test and the reviewer told me that the class structure is inverse of what is the right way of doing it and that it is sloppy at some places.

I want to know what is wrong with my structuring of classes, if anything. Does it look sloppy? Can someone please help me identify places where it is lacking Best Practices for Java?

I have a MyBuilder class which creates an instance of MyAlgorithm. MyAlgorithm has the methods to encrypt and decrypt text, and it extends another class called EncryptionAndDecryption where the actual logic of the algorithm resides.


public class MyAlgorithm extends EncryptionAndDecryption {

    public enum Action {
        ENCRYPT,
        DECRYPT
    }

    protected MyAlgorithm(final String cipherCharSet) {
        super(cipherCharSet);
    }

    public String encryptText(final String cipher, final String plainText) {
        return encryptOrDecryptText(cipher, plainText, Action.ENCRYPT);
    }

    public String decryptText(String cipher, String encryptedText) {
        return encryptOrDecryptText(cipher, encryptedText, Action.DECRYPT);
    }

    public boolean encryptDirectory(String cipher, String baseDirectory) {
        // for each files and sub files in each sub directories, call the following method;
    // encryptOrDecryptFile(cipher, file, baseDirectory, Action.ENCRYPT));
    }

    public boolean decryptDirectory(String cipher, String baseDirectory) {
        // for each files and sub files in each sub directories, call the following method;
    // encryptOrDecryptFile(cipher, file, baseDirectory, Action.DECRYPT));
    }
}
public class EncryptionAndDecryption {

    private final String CIPHER_CHAR_SET;

    protected EncryptionAndDecryption(final String cipherCharSet) {
        this.CIPHER_CHAR_SET = cipherCharSet;
    }

    protected final String encryptOrDecryptText(final String cipher, final String inputText, final MyAlgorithm.Action action) {
         // method logic here
    }

    protected final void encryptOrDecryptFile(final String cipher, final Path filePath, final String baseDirectory, final MyAlgorithm.Action action) {
        // method logic here
    }
}
public class MyBuilder {

    private String CIPHER_CHAR_SET;

    private MyBuilder() {}

    public static MyBuilder builder() {
        return new MyBuilder();
    }

    public MyBuilder withCipherSet(String cipherSet) {
        this.CIPHER_CHAR_SET = cipherSet;
        return this;
    }

    public MyAlgorithm build() {
        return new MyAlgorithm(CIPHER_CHAR_SET);
    }
}
public class MyEncryptionApp {

    public static final String CIPHER_CHAR_SET = "abcdefghijklmnopqrstuvwxyz";

    public static void main(String args[]) {
        if (args.length != 3) {
            System.out.println("Exact 3 parameters required - [action] [key] [target]");
            System.exit(1);
        }

        String action, key, target;
        action = args[0];
        key = args[1];
        target = args[2];

        MyAlgorithm myAlgorithm = MyBuilder.builder()
                .withCipherSet(CIPHER_CHAR_SET)
                .build();

        if ("encrypt".equalsIgnoreCase(action)) {
            System.out.println(myAlgorithm.encryptText(key, target));

        } else if ("decrypt".equalsIgnoreCase(action)) {
            System.out.println(myAlgorithm.decryptText(key, target));

        } else if ("encryptDir".equalsIgnoreCase(action)) {

            if(myAlgorithm.encryptDirectory(key, target)) {
                System.out.println("Directory encryption Successful");
            }

        } else if ("decryptDir".equalsIgnoreCase(action)) {

            if(myAlgorithm.decryptDirectory(key, target)) {
                System.out.println("Directory decryption Successful");
            }

        } else {
            System.out.println("action [" + action + "] not implemented");
        }

    }
}

\$\endgroup\$
3
\$\begingroup\$

Hello and welcome to Code Review! I try to give a possible explanation of what the reviewer meant when he was talking about inverse class structure; in the main method this code is present:

MyAlgorithm myAlgorithm = MyBuilder.builder()
               .withCipherSet(CIPHER_CHAR_SET)
               .build();

But from your code MyAlgorithm is a subclass of EncryptionAndDecryption and this is the class where you defined the operations of encryption and decryption , so the method shoud have return an instance of class EncryptionAndDecryption instead of one subclass. Other problem: in the class EncryptionAndDecryption we have the below methods:

protected final String encryptOrDecryptText(final String cipher, final String inputText, final MyAlgorithm.Action action)

protected final void encryptOrDecryptFile(final String cipher, final Path filePath, final String baseDirectory, final MyAlgorithm.Action action)

With parameter MyAlgorithm.Action action the class EncryptionAndDecryption will be dependant from its subclass MyAlgorithm and without its subclass it cannot be even compiled. In the class EncryptionAndDecryption I would make two distinct methods for encryption and decryption of strings, putting these related to files and directory in the subclass MyAlgorithm, deleting the enum value you are using to distinguish between the two operations. Style problem : in the main method is present a list of if like this below:

if ("encrypt".equalsIgnoreCase(action)) {}

Normally it is the string compared to the string literal like:

if (action.equalsIgnoreCase("encrypt")) {}

You can substitute the list of if with a switch like documented in strings-switch.

\$\endgroup\$
  • \$\begingroup\$ MyAlgorithm myAlgorithm = MyBuilder.builder() .withCipherSet(CIPHER_CHAR_SET) .build(); The reason it returns MyAlgorithm is because I have thought of it as a public facing class which has 4 separate methods to encrypt and String and Files. It doesn't know anything about the implementation. The super class EncryptionAndDecryption contains 2 methods that has actual logic needed for the 4 methods in sub class. These are marked protected and final so no one outside of the package can access them, nor can a child override them. Is this unconventional? \$\endgroup\$ – winzhack999 Sep 19 '19 at 18:34
  • \$\begingroup\$ I agree that the use of Enum in super class creates a dependency on the child class, instead I should have made a separate enum file. \$\endgroup\$ – winzhack999 Sep 19 '19 at 18:34
  • \$\begingroup\$ if ("encrypt".equalsIgnoreCase(action)) {} Agreed, that normally people do it other way round, but I recently learned that doing this way helps prevent NullPointerException. And, ofcourse switch would have been much better choice here. \$\endgroup\$ – winzhack999 Sep 19 '19 at 18:34
  • 1
    \$\begingroup\$ @winzhack999 I understand your point, maybe an alternative could be nesting class EncryptionAndDecryption inside class MyAlgorithm like in Base64 and so your builder returns just a MyAlgorithm instance. This is one possible choice and the reviewer probably had in his mind his personal solution to solve the problem. \$\endgroup\$ – dariosicily Sep 20 '19 at 7:46

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.