Copying messages with compression

Question

I have the following Main class:

public class Main {

    public static void main(String[] args) {
        try {
            MessageWriter writer = null;

            if (args.length < 1) {
                throw new IllegalArgumentException("There is no input file path");
            } else if (args.length == 1) {
                writer = new ConsoleMessageWriter();
            } else {
                writer = new FileMessageWriter(args[1]);
            }

            CompressingMessageWrite compressingMessageWrite = new CompressingMessageWrite(writer);

            FileMessageReader fileMessageReader = new FileMessageReader(args[0]);
            int numberOfMessages = fileMessageReader.getNumberOfMessages();
            int loopCounter = (numberOfMessages % 2 == 0) ? numberOfMessages : numberOfMessages - 1;

            if (numberOfMessages < 1) {
                return;
            }

            for (int i = 0; i < loopCounter / 2; i++) {
                Message message1 = fileMessageReader.readMessage();
                Message message2 = fileMessageReader.readMessage();
                compressingMessageWrite.writeMessages(message1, message2);
            }

            if (loopCounter != numberOfMessages) {
                Message message = fileMessageReader.readMessage();
                compressingMessageWrite.writeMessage(message);
            }
        } catch (IllegalMessageFormatException | IOException e){
            System.out.println(e.getMessage());
        }
    }
}

My professor says that I don't close any files in the Main class. I don't understand the files to which he's referring.

Note: Classes FileMessageWriter and FileMessageReader works with files using try-with-resources statement, like this:

try (FileReader file = new FileReader(messageFilePath);
     BufferedReader reader = new BufferedReader(file)) {

    String line;

    while ((line = reader.readLine()) != null) {
        //work with line
    }

}

FileMessageReader.java:

/**
 * Reading messages from file
 */
public class FileMessageReader {
    private static final int UndefinedNumberOfLines = -1;
    private int lastReadStringNumber = 0;
    private final String messageFilePath;

    /**
     * Constructs a new FileMessageReader with specific <code>filePath</code>.
     * @param filePath file with messages
     * @throws FileNotFoundException if given file does not exist
     */
    public FileMessageReader(String filePath) throws FileNotFoundException {
        if (filePath == null)
        {
            throw new IllegalArgumentException("filePath is null");
        }

        File f = new File(filePath);
        if (!f.exists() || f.isDirectory()) {
            throw new FileNotFoundException("file" + filePath + " not found.");
        }

        messageFilePath = filePath;
    }

    /**
     * Reading next message from a file
     * @return Read message
     * @throws IllegalMessageFormatException if message has illegal format
     * @throws java.io.IOException If an input or output exception occurred
     */
    public Message readMessage() throws IllegalMessageFormatException, IOException {
        int currentLine = 0;
        Message message = new Message();

        try (FileReader file = new FileReader(messageFilePath);
             BufferedReader reader = new BufferedReader(file)) {
            String line;
            int numberLinesInMessage = UndefinedNumberOfLines;

            for(int i = 0; i < lastReadStringNumber; i++) {
                reader.readLine();
                currentLine++;
            }

            while ((line = reader.readLine()) != null) {
                if (numberLinesInMessage == UndefinedNumberOfLines) {
                    if (isInteger(line)) {
                        numberLinesInMessage = Integer.parseInt(line);
                    } else {
                        throw new IllegalMessageFormatException("line is not a number");
                    }
                } else if (numberLinesInMessage > 0) {
                    message.append(line);
                    numberLinesInMessage--;
                }

                currentLine++;

                if (numberLinesInMessage == 0) {
                    lastReadStringNumber = currentLine;
                    return message;
                }
            }

            if (numberLinesInMessage > 0) {
                throw new IllegalMessageFormatException("Message has less lines than is specified.");
            }

        } catch (FileNotFoundException e) {
            throw new FileNotFoundException("file" + messageFilePath + " not found.");
        } catch (IOException e) {
            throw new IOException("Strange IOException happened. Message: " + e.getMessage());
        }

        lastReadStringNumber = currentLine;
        return message;
    }

    /**
     * @return number of messages in file
     * @throws IllegalMessageFormatException if message has illegal format
     * @throws java.io.IOException If an input or output exception occurred
     */
    public int getNumberOfMessages() throws IllegalMessageFormatException, IOException {
        int numberOfMessages = 0;

        try (FileReader file = new FileReader(messageFilePath);
             BufferedReader reader = new BufferedReader(file)) {

            String line;
            int numberLinesInMessage = UndefinedNumberOfLines;

            while ((line = reader.readLine()) != null) {
                if (numberLinesInMessage == UndefinedNumberOfLines) {
                    if (isInteger(line)) {
                        numberLinesInMessage = Integer.parseInt(line);
                    } else {
                        throw new IllegalMessageFormatException("line is not a number");
                    }
                } else if (numberLinesInMessage > 0) {
                    numberLinesInMessage--;
                }

                if (numberLinesInMessage == 0) {
                    numberOfMessages++;
                    numberLinesInMessage = UndefinedNumberOfLines;
                }
            }

            if (numberLinesInMessage > 0) {
                throw new IllegalMessageFormatException("Message has less lines than is specified.");
            }

        } catch (FileNotFoundException e) {
            throw new FileNotFoundException("file" + messageFilePath + " not found.");
        } catch (IOException e) {
            throw new IOException("Strange IOException happened. Message: " + e.getMessage());
        }

        return numberOfMessages;
    }

    private static boolean isInteger(String s) throws IllegalMessageFormatException {
        try {
            Integer.parseInt(s);
        } catch(NumberFormatException e) {
            throw new IllegalMessageFormatException("line is not a number");
        }

        return true;
    }
}

CompressingMessageWrite.java:

/**
 * Compress messages to input
 */
public class CompressingMessageWrite implements MessageWriter {
    private final MessageWriter writer;

    /**
     * Constructs a new CompressingMessageWrite with specific <code>writer</code>.
     * @param writer which is used for writing to some input
     */
    public CompressingMessageWrite(MessageWriter writer) {
        if (writer == null) {
            throw new IllegalArgumentException("writer is null");
        }

        this.writer = writer;
    }

    /**
     * Write message to input
     * @param message which will be wrote
     */
    @Override
    public void writeMessage(Message message) {
        if (message == null) {
            throw new IllegalArgumentException("message is null");
        }

        writer.writeMessage(message);
    }

    /**
     * Write messages to input
     * @param message1 which will be wrote
     * @param message2 which will be wrote
     */
    public void writeMessages(Message message1, Message message2) {
        if (message1 == null) {
            throw new IllegalArgumentException("message1 is null");
        }

        if (message2 == null) {
            throw new IllegalArgumentException("message2 is null");
        }

        Message resultMessage = new Message();
        resultMessage.append(message1);
        resultMessage.append(message2);

        writer.writeMessage(resultMessage);
    }
}

Message.java:

/**
 * Message class
 */
public class Message {
    private final List<String> lines = new ArrayList<>();
    private final StringBuilder stringBuilder = new StringBuilder();

    /**
     * Adding <code>line</code> to this message
     * @param line which adding to this message
     */
    public void append(String line) {
        if (line == null) {
            throw new IllegalArgumentException("line is null");
        }

        lines.add(line);
    }

    /**
     * Adding <code>message</code> to this message
     * @param message added message
     */
    public void append(Message message) {
        if (message == null) {
            throw new IllegalArgumentException("message is null");
        }

        message.getLines().forEach(this::append);
    }

    /**
     * @return list of message lines
     */
    public List<String> getLines() {
        return Collections.unmodifiableList(lines);
    }

    /**
     * @return String representation of Message
     */
    @Override
    public String toString() {
        stringBuilder.append(getLines().size());
        stringBuilder.append(System.lineSeparator());

        for (String msg : lines)
        {
            stringBuilder.append(msg);
            stringBuilder.append(System.lineSeparator());
        }
        stringBuilder.setLength(0);

        return stringBuilder.toString();
    }
}

MessageWritter.java:

/**
 * Interface for writing of messages
 */
public interface MessageWriter {
    /**
     * Write message in source
     * @param message which will be wrote
     */
    public void writeMessage(Message message);
}

FileMessageWritter.java:

/**
 * Write messages to file
 */
public class FileMessageWriter implements MessageWriter {
    private final String messageFilePath;

    /**
     * Constructs a new FileMessageWriter with specific <code>filePath</code>.
     * @param filePath file for messages
     * @throws FileNotFoundException if given file does not exist
     */
    public FileMessageWriter(String filePath) throws FileNotFoundException {
        if (filePath == null)
        {
            throw new IllegalArgumentException("filePath is null");
        }

        File f = new File(filePath);
        if (!f.exists() || f.isDirectory()) {
            throw new FileNotFoundException("file" + filePath + " not found.");
        }

        messageFilePath = filePath;
    }

    /**
     * Write message to console
     * @param message which will be wrote
     */
    @Override
    public void writeMessage(Message message) {
        if (message == null) {
            throw new IllegalArgumentException("message is null");
        }

        try (FileWriter fileWriter = new FileWriter(messageFilePath, true);
             BufferedWriter bufferedWriter = new BufferedWriter(fileWriter);
             PrintWriter out = new PrintWriter(bufferedWriter)) {
            out.print(message);
        } catch (IOException e) {
            throw new RuntimeException("Strange IOException happened. Message: " + e.getMessage());
        }
    }
}

Answer 1

Avoid too long try-catch blocks

There are some big problems with lengthy try-catch blocks:

• Harder to pinpoint the line that triggered the exception

  • Harder to write the code to handle it
  • Harder to understand the code: what could go wrong here and where exactly?

• If the large block calls other methods, then possibly one of those methods may throw as well, which can be overlooked, as the reader of the lengthy block might forget to verify all those calls, and catch something by accident that shouldn't be caught

Reduce variable span and live time

The span of a variable is the average lines between lines that use the variable. The live time of a variable is the number of lines between declaration and its last use. It's good to keep both of these metrics down.

Look at this code, for example:

int numberOfMessages = fileMessageReader.getNumberOfMessages();
int loopCounter = (numberOfMessages % 2 == 0) ? numberOfMessages : numberOfMessages - 1;

if (numberOfMessages < 1) {
    return;
}

The loopCounter is initialized in terms of numberOfMessages, but it might not be used at all, if numberOfMessages < 1. These statements should be rearranged:

int numberOfMessages = fileMessageReader.getNumberOfMessages();
if (numberOfMessages < 1) {
    return;
}

int loopCounter = (numberOfMessages % 2 == 0) ? numberOfMessages : numberOfMessages - 1;

This last statement can be simplified:

int loopCounter = numberOfMessages - (numberOfMessages % 2);

Check your real target, not a list of non-targets

This condition seem to check if the File f is something other than you want:

    if (!f.exists() || f.isDirectory()) {

The problem with this is, what if your list of invalid things is not exhaustive? And why phrase your requirements in such indirect way?

It would be better to write in a more direct way what you want exactly:

if (!f.isFile()) {

Naming

The convention for constant values is all-caps. So instead of:

private static final int UndefinedNumberOfLines = -1;

Name like this:

private static final int UNDEFINED_NUMBER_OF_LINES = -1;

Wasted operations

The FileMessageReader checks if the string path passed in is a file, by creating a File object from it. So far so good, but then it throws away the File and keeps only the path. It would be better to keep the File and forget about the path. Later when you use the path to create a FileReader, it will convert the path to a File. Again. Besides, it's called a FileReader for a reason. If you already have a File, it's better to use it.

Consider this code:

    if (isInteger(line)) {
        numberLinesInMessage = Integer.parseInt(line);
    } else {
        throw new IllegalMessageFormatException("line is not a number");
    }

// ...

private static boolean isInteger(String s) throws IllegalMessageFormatException {
    try {
        Integer.parseInt(s);
    } catch(NumberFormatException e) {
        throw new IllegalMessageFormatException("line is not a number");
    }

    return true;
}

This is just silly:

• Integer.parseInt gets called twice
• The else block will never get executed, so it's pointless
• This is confusing

You could replace the if-else with this single line:

numberLinesInMessage = parseInt(line);

And write a parseInt helper as:

private int parseInt(String s) throws IllegalMessageFormatException {
    try {
        return Integer.parseInt(s);
    } catch(NumberFormatException e) {
        throw new IllegalMessageFormatException("line is not a number");
    }
}

null checks willy-nilly

What's the point of all the null checks everywhere? For example here:

public void writeMessages(Message message1, Message message2) {
    if (message1 == null) {
        throw new IllegalArgumentException("message1 is null");
    }

    if (message2 == null) {
        throw new IllegalArgumentException("message2 is null");
    }

    Message resultMessage = new Message();
    resultMessage.append(message1);
    resultMessage.append(message2);

    writer.writeMessage(resultMessage);
}

Can the caller recover from them? No.

Should the method receive null parameters under normal circumstances? Hopefully not.

On closer inspection, it seems that these message objects can only come from fileMessageReader.readMessage(). And that method is guaranteed to return non-null values or throw. If it throws, then the writer should not be called with null parameters, as the caller should handle the situation better. If it doesn't throw, then the passed in Message objects are not null, so there's no need for these checks. These checks make the reader suspicious about the code.

Review all the null checks that you do. Try to rethink and refactor the code to make null values impossible. For example a common bad practice is when an interface is declared to return a List, but the implementation can return a null. This forces callers to check for null before iterating over the elements. The recommended practice is to make the implementation return an empty list instead, so that callers don't need the null checks, leading to a much more ergonomic usage. This is just an example. Suspect anything that might be null, and refactor to make such situations impossible by design.