0

What would be considered better, cleaner code from the following options? The code basically sends a file through a Socket using an OutputStream that encrypts it. The code does not look that basic when read but it does not need to be understood at a technical level, this is not the point of the question. The comments tell what is going on at a general level.

I am obviously not looking for an objective answer as design is usually very subjective and depends on many factors. I just want opinions that create constructive feedback around the problem.

Option 1: The one method way. I consider this WAY cleaner and easier to read, but obviously, it brakes many OO principles such as cohesion and modularity.

public class FileSender {

public static final String KEY = "00000000000000000000000000000000";
public static final String ALGORITHM = "AES";
public static final String FILE = "C:\\Users\\Andres\\Desktop\\file.txt";
public static final String HASH_ALG = "MD5";

public static void main(String [] args) throws Exception {
    CipherOutputStream cipherOut = null;
    FileInputStream fileReader = null;
    Socket client = null;
    FileInputStream fis = null;
    try {
        System.out.println("Running client ...");

        //Connects to serverSocket.

        client = new Socket("localhost", 8081); 

        //Creates MD5 Hash.

        MessageDigest md = MessageDigest.getInstance(HASH_ALG);
        File file = new File(FILE);
        fis = new FileInputStream(file);
        byte[] byteArray = new byte[1024];
        int bytesCount = 0;
        while ((bytesCount = fis.read(byteArray)) != -1)
            md.update(byteArray, 0, bytesCount);
        byte[] hash = md.digest();
        String checksum = String.format("%032x",new BigInteger(1, hash));
        fis.close();

        //Creates "secret" key and inits the cipher.

        SecretKeySpec keySpec = new SecretKeySpec(hex2byte(KEY), ALGORITHM);
        Cipher cipher = Cipher.getInstance(ALGORITHM + "/ECB/PKCS5PADDING");
        cipher.init(Cipher.ENCRYPT_MODE, keySpec);
        cipherOut = new CipherOutputStream(client.getOutputStream(), cipher);

        //Sends the file.

        byte[] fileBuffer = new byte[1024];
        fileReader = new FileInputStream(FILE);
        int len;
        while ((len = fileReader.read(fileBuffer)) != -1)
            cipherOut.write(fileBuffer, 0, len);
        System.out.println("Ended client .... ");
    }catch(IOException e) {
        e.printStackTrace();
    }finally{
        if(fileReader != null) fileReader.close();
        if(cipherOut != null) cipherOut.close();
        if(client != null) client.close();
        if(fis != null) fis.close();
    }
}

public static byte[] hex2byte(String s) {
    return DatatypeConverter.parseHexBinary(s);
}
}

Option 2: The modular way, dividing the code. This is obviously the correct way in most cases because it cares about OO principles, but is the code cleaner?

public class FileSender {

    public static final String KEY = "00000000000000000000000000000000";
    public static final String ALGORITHM = "AES";
    public static final String FILE = "C:\\Users\\Andres\\Desktop\\file.txt";
    public static final String HASH_ALG = "MD5";

    private String fileName;
    private String serverMachine;
    private int serverPort;

    public FileSender(String fileName, String serverMachine, int serverPort){
        this.fileName = fileName;
        this.serverMachine = serverMachine;
        this.serverPort = serverPort;
    }

    //High level method that uses the class details.
    public void sendFile(){
        CipherOutputStream cipherOut = null;
        Socket client = null;
        String checkSum;
        try {
            System.out.println("Running client ...");
            client = connectToServer();
            checkSum = getHashChecksum();
            cipherOut = initCipherStream(client);
            transferData(cipherOut);
        } catch (Exception e) {
            System.out.println(e.getMessage());
        } finally {

        }
    }

    //Creates MD5 checksum
    private String getHashChecksum() {
        FileInputStream fis = null;
        try {
            MessageDigest md = MessageDigest.getInstance(HASH_ALG);
            File file = new File(this.fileName);
            fis = new FileInputStream(file);
            byte[] byteArray = new byte[1024];
            int bytesCount = 0;
            while ((bytesCount = fis.read(byteArray)) >= 0)
                md.update(byteArray, 0, bytesCount);
            byte[] hash = md.digest();
            fis.close();
            return String.format("%032x",new BigInteger(1, hash));
        } catch (Exception e) {
            e.printStackTrace();
            throw new RuntimeException("Error in checksum");
        } finally {
            try {
                fis.close();
            } catch (IOException e) {
                throw new RuntimeException(
                    "Error in checksum. Provided file can't be closed.");
            }
        }
    }

    //Connects client to server.
    private Socket connectToServer() throws Exception{
        try {
            Socket client = new Socket(this.serverMachine, this.serverPort);
            return client;
        } catch (IOException e) {
            throw new Exception("Server can't be reached");
        }
    }

    //Creates output stream to send file.
    private CipherOutputStream initCipherStream(Socket client) {
        //Crea la llave.
        SecretKeySpec keySpec = new SecretKeySpec(hex2byte(KEY), ALGORITHM); 

        Cipher cipher;

        try {
            cipher = Cipher.getInstance(ALGORITHM + "/ECB/PKCS5PADDING");
            cipher.init(Cipher.ENCRYPT_MODE, keySpec);
            return new CipherOutputStream(client.getOutputStream(), cipher);
        } catch (Exception  e) {
            e.printStackTrace();
            throw new RuntimeException("Error in cipher stream initialization.");
        }

    }

    //Sends file
    private void transferData(CipherOutputStream cipherOut) {
        FileInputStream fileReader = null;
        try {
            //Missing: send MD5 hash. TODO
            fileReader = new FileInputStream(this.fileName);
            byte[] fileBuffer = new byte[1024];
            int len;
            while ((len = fileReader.read(fileBuffer)) != -1)
                cipherOut.write(fileBuffer, 0, len);
        } catch (Exception e) {
            throw new RuntimeException("File " + this.fileName + " not found");
        } finally{
            try {
                if(fileReader != null) fileReader.close();
            } catch (IOException e) {
                throw new RuntimeException("Socket stream can't be closed");
            }
        }
    }

    private byte[] hex2byte(String s) {
        return DatatypeConverter.parseHexBinary(s);
    }

    public static void main(String [] args) throws Exception {
        FileSender fileSender = new FileSender(FILE, "localhost", 8081);
        fileSender.sendFile();
    }
}

What do you think about either approach? Would you recommend another strategy to make better, more readable code?

  • 3
    Second approach is better. I would also break out the File -> Inputstream initialisation and make a generic sender – Peter Gelderbloem May 11 '17 at 16:15
  • 1
    If you're interested on feedback on any and all aspects of your code, the Code Review site might be better for this question. But as a design review, the question is still on-topic here. – amon May 11 '17 at 16:25
  • Yes, thanks, I asked on meta about the best place for this and was pointed too to CodeReview. Still, I'm not sure if I should close the question because I actually like the answer. – AFP_555 May 11 '17 at 16:36
  • There's a ton of cruft here so it's hard to make sense of what a clean design is. There's a whole segment of code used to generate a checksum that is never used, for example. – JimmyJames May 11 '17 at 16:40
  • 4
    Once you think about writing tests for the code, you instantly recognize the first approach as basically untestable, except as a whole. – 9000 May 11 '17 at 19:19
5

The decision in my mind rests whether this program will become larger or stay about as is. In what you are showing I would totally agree that a program of this size doesn't merit complexity of more than one class or true object oriented implementation.

However, if your program grows well over 50 lines of code, you'll run out of steam using the monolithic approach, due to technical debt, specifically in this case, due to the lack of modular architecture, lack of separation of concerns, inappropriate conflation of responsibilities. Thus, between the two, the latter style is better.

However, you need to keep going with it, as doesn't go far enough.

There is only one class, and too many essentially functionally unrelated methods are in the same class. The methods in this one class are all related to a particular use case, rather than being separated into single responsibilities.

For one, the main should probably be in its own Main class.

Generally speaking, a data transfer operation should operate on already opened (file) streams rather than opening the file then transferring and then closing the file, because the way it stands it is conflating too many responsibilities together. (as @PeterGelderbloem also points out in comment).

The encoding key and methods should be encapsulated into their own abstraction such that other approaches can be used by an application (possibly at the same time) by instantiating the right object and without having to change any of the other (e.g. data transferring) code.


Utility methods like hex2byte isn't much of an abstraction on its own and I would probably just use DatatypeConverter.parseHexBinary directly rather than building a better abstraction for this one time use.

(A better abstraction would group/pair two conversions together (forward and reverse) so that the user could be confident of converting from string and to string using compatible approaches, and the whole abstraction could be implemented using a different compatible pair without having to worry about the rest of the code using the proper other member of pair since the abstraction groups them together.)

2

This really should probably go to code review but here goes:

To start, you really should clean up your option 1. Making use of the try() with syntax eliminates a lot of the boilerplate of closing things and checking nulls etc. Then some other minor things that you might or might not like but we get down to a much smaller set of code:

import static javax.xml.bind.DatatypeConverter.parseHexBinary;

import java.io.FileInputStream;
import java.io.IOException;
import java.math.BigInteger;
import java.net.Socket;
import java.security.InvalidKeyException;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;

import javax.crypto.Cipher;
import javax.crypto.CipherOutputStream;
import javax.crypto.NoSuchPaddingException;
import javax.crypto.spec.SecretKeySpec;

public class FileSender {

    public static final String KEY = "00000000000000000000000000000000";
    public static final String ALGORITHM = "AES";
    public static final String FILE = "C:/Users/Andres/Desktop/file.txt";
    public static final String HASH_ALG = "MD5";

    public static void main(String [] args) throws Exception {
        System.out.println("Running client ...");
        MessageDigest md = MessageDigest.getInstance(HASH_ALG);

        byte[] byteArray = new byte[1024];

        try (FileInputStream fis = new FileInputStream(FILE)) {
            for (int bytesCount = 0; (bytesCount = fis.read(byteArray)) != -1;) {
                md.update(byteArray, 0, bytesCount);
            }
        }

        String checksum = String.format("%032x",new BigInteger(1, md.digest()));

        //Create "secret" key and inits the cipher.
        SecretKeySpec keySpec = new SecretKeySpec(parseHexBinary(KEY), ALGORITHM);
        Cipher cipher = Cipher.getInstance(ALGORITHM + "/ECB/PKCS5PADDING");
        cipher.init(Cipher.ENCRYPT_MODE, keySpec);

        // send the file
        try (Socket client = new Socket("localhost", 8081);
             CipherOutputStream cipherOut = new CipherOutputStream(
                 client.getOutputStream(), 
                 cipher);
             FileInputStream fileReader = new FileInputStream(FILE);
        ) {
            byte[] fileBuffer = new byte[1024];
            for (int len; (len = fileReader.read(fileBuffer)) != -1;) {
                cipherOut.write(fileBuffer, 0, len);
            }
        }
        System.out.println("Ended client .... ");
    }
}

Now that we have that, it's easy to see that there are two things going on here: creating a check sum and sending the file.

    public static String createCheckSum() throws NoSuchAlgorithmException, IOException {
      MessageDigest md = MessageDigest.getInstance(HASH_ALG);

      byte[] byteArray = new byte[1024];

      try (FileInputStream fis = new FileInputStream(FILE)) {
          for (int bytesCount = 0; (bytesCount = fis.read(byteArray)) != -1;) {
              md.update(byteArray, 0, bytesCount);
          }
      }

      return String.format("%032x",new BigInteger(1, md.digest()));
    }

    public static void sendFile() 
        throws NoSuchPaddingException, 
               InvalidKeyException, 
               NoSuchAlgorithmException, 
               IOException {

        SecretKeySpec keySpec = new SecretKeySpec(parseHexBinary(KEY), ALGORITHM);
        Cipher cipher = Cipher.getInstance(ALGORITHM + "/ECB/PKCS5PADDING");
        cipher.init(Cipher.ENCRYPT_MODE, keySpec);

        try (Socket client = new Socket("localhost", 8081);
             CipherOutputStream cipherOut = new CipherOutputStream(
                 client.getOutputStream(), 
                 cipher);
             FileInputStream fileReader = new FileInputStream(FILE);
        ) {
            byte[] fileBuffer = new byte[1024];
            for (int len; (len = fileReader.read(fileBuffer)) != -1;) {
                cipherOut.write(fileBuffer, 0, len);
        }
    }
}

Then your main simply calls the two methods. If this is all you need to do right now, then I think you are done (aside from the checksum thing.) If you need to reuse this, the next step would be to refactor out the constants and make them parameters.

In order to build good OO code in Java, you really need to be good at writing good reusable methods. All the OO features of Java revolve around methods and trying to use polymporphism with methods that are not well structured is not going to get you very far.

  • 1
  • 1
    @CandiedOrange Good call. I swear I tried that and it didn't compile. I thought it was a limitation of the syntax. – JimmyJames May 11 '17 at 20:23
  • @JimmyJames - the syntax is a bit tricky, so if you weren't sure of it, it's quite likely you didn't hut exactly there right way of don't it. Note that the resources list is inside parenthesis but separated by semicolons, which always seems wrong to me. – Jules May 15 '17 at 18:49
2

the comments say mainly what is going on in a general level.

Well here's yer problem.

It's good to have comments when they are needed. But it's not good to need comments.

What you've shown us in option 1 is procedural code. You say it's easier to read. Well it is straight forward, flat, and about as flexible as a tax collector. But readable? Well I read FileSender. So as my eyes gloss over I expect it to send files. But I quickly get lost in details. Details I'm not ready to think about. Now I can power through that. Or I can rely on the comments. But doing that isn't a sign of easy to read code. So really I think this is easier to write. Not easier to read.

You hold up option 2 as the counter example. Well yes this is hard to read as well. But It isn't the use of abstraction that I find distracting here. It's the extra noise.

  • Why are their comments that say thing same thing that the method names do?

  • Why do I have to scroll up and down to follow this? Either put the most abstract method definitions at the top or the bottom. When they're at the same level define them in order of appearance. Don't shuffle them.

  • Why are you catching exceptions just to re-throw them when you never re-catch them? Why are they all RuntimeException? Is this doing you any good? Because it's fairly distracting and has nothing to do with being OOP vs procedural.

  • Why are you returning things that need to be closed? Look up the hole in the middle pattern. OO can be functional.

In short option 2 is hard to read because it wasn't well written. Now I'll admit, writing good looking OO code can be hard. Especially when you aren't used to it. However, reading good OO isn't that hard.

The real question then is now that you know what you want to do with good OO which one would you rather work from? Option 1 or Option 2? Well I'll admit option 2 wouldn't be my pick. Does this mean OO is wrong and procedural is cleaner? No. It means OO is more expressive. The more expressive one is the easier one to make a mess in. It's also where the power is. Use that power wisely.

  • Good point about the order of the methods. The RuntimeException is something I read in Java Programming good practices. There's no point on throwing or catching a checked exception if the client has nothing to do with it. Checked Exceptions should only be used when the client of the code can actually do something useful when the exception occurs. This is kind of a polemic Java practice but I've find that makes Exception handling simpler. – AFP_555 May 11 '17 at 19:18
  • Also, great point about returning things that must be closed. Something that I also found smelly, but decided to ignore because I wasn't sure it was a bad practice, but now it is clear. – AFP_555 May 11 '17 at 19:25
  • Something else (sorry, it's just that you pointed some very cool flaws) how would you apply this pattern in Java. For C# is clear, as lambdas are very easy. – AFP_555 May 11 '17 at 19:35
  • 1) if you're going to turn every exception into an unchecked exception do that in one place. That decision doesn't need to be scattered everywhere. 2) Java 8 has lambdas. If you don't have java 8 you can still use anonymous classes, though it's hard to live with the boiler plate code you have to use. It's still better than leaving an open resource flapping in the wind. – candied_orange May 11 '17 at 19:53
1

It depends on two factors:

  1. Size of the program
  2. Whether it is a whole program or a part of a bigger program.

When would option 1 be better?

For a program of that size or that will not be part of a bigger project, I could say option 1 is OK. It having a main method means that that is the entry point and that's it. It probably a whole program in itself.

When would option 2 be better?

However, should this code be part of a bigger thing, the second option is better:

  1. The constructor tells you what it needs to do its work
  2. The only public method is all you need to know to use it from anywhere.

If the code was considerabily larger or complex, then the second option would also be better:

  1. Easier to test the individual parts (sending the file, creating the hash etc.)
  2. Easier to make corrections that would not create new bugs in other parts of the program because of smaller scopes of methods
  3. With the more modular approach you don't need to read the whole program to understand it. So you have less to read and less to have in mind at the same time.
  • That's actually the whole program, but I like pretending I'm working on something bigger haha. Good points, thanks. – AFP_555 May 11 '17 at 19:21

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.