Java job responsible for processing file statements

Question

I am looking for any refactoring tips to improve code readability. Does it feel like procedural programming? If so, how can I improve it?

public class StatementJob implements Job {

    private static final Logger LOGGER = LoggerFactory.getLogger(StatementJob.class);
    private static final String JOB_IS_DISABLED = "Uploads statement job is disabled and will not run";
    private static final String UNABLE_TO_CONTINUE = "Unable to continue";

    private final Configuration jobConfiguration;
    private final Log log;

    public StatementJob(JobConfiguration jobConfiguration) {
        this.jobConfiguration = jobConfiguration;
        this.log = new JobLog();
    }

    @Override
    public Log execute() {

        LOGGER.info("Stating uploading statements");
        final JobProperties properties;
        try {
            properties = jobConfiguration.loadProperties();
        } catch (BadConfigurationException e) {
            return log.addError(e.getMessage());
        }

        if (!properties.isRunJob()) {
            return log.addSuccess(JOB_IS_DISABLED);
        }

        final Employee statementUploader = Employee.load(properties.getStatementUploader());
        final FileStatements fileStatements = new Statements(properties.getDirectories());

        fileStatements
                .getInboundStatements()
                .forEach(inboundStatement -> {

                    try {
                        fileStatements.moveToWipLocation(inboundStatement);
                        fileStatements
                                .getWipStatements()
                                .forEach(wipFileStatement -> {

                                    try {
                                        new UploadableDocument(statementUploader, wipFileStatement).upload();
                                        fileStatements.moveToArchiveLocation(wipFileStatement);
                                    } catch (FailedToUploadDocumentException e) {
                                        log.addError(e.getMessage());
                                        try {
                                            fileStatements.moveToErrorLocation(wipFileStatement);
                                        } catch (UnableToMoveFileToDirectoryException ex) {
                                            LOGGER.error(UNABLE_TO_CONTINUE, e);
                                            throw new FatalJobException(UNABLE_TO_CONTINUE, e);
                                        }
                                    } catch (UnableToReadFileException | UnableToMoveFileToDirectoryException e) {
                                        LOGGER.error(UNABLE_TO_CONTINUE, e);
                                        throw new FatalJobException("Unable to continue", e);
                                    }
                                });
                    } catch (UnableToMoveFileToDirectoryException e) {
                        LOGGER.error(UNABLE_TO_CONTINUE, e);
                        throw new FatalJobException("Unable to continue", e);
                    }
                });

        return log;
    }

}

What does the code do?

• There is FileStatements which represents files, called Statement, stored in the file system in the inbound directory.
• When StatementJob is executed it moves Statements from the inbound to work in progress directory.
• Then Statements from the work in progress directory are uploded to some storage.
• When the upload is succesfull, the Statements are moved to the archive statement folder. Statements which failed to upload are moved to the error statement folder.
• Log is used to collect information about the job that will be emailed to whomever needs to know the status. This is very primitive at this point.

Answer 1

Here's the best I think we can get it without actually knowing what your code is for.

public class StatementJob {
    public static final Logger LOGGER = LoggerFactory.getLogger(StatementJob.class);

    public static final String JOB_IS_DISABLED = "Uploads statement job is disabled and will not run";
    public static final String UNABLE_TO_CONTINUE = "Unable to continue";

    private final Configuration jobConfiguration;
    private final Log log;

    public StatementJob(JobConfiguration jobConfiguration) {
        this.jobConfiguration = jobConfiguration;
        this.log = new JobLog();
    }

    public Log execute() {
        LOGGER.debug("Stating uploading statements");

        final JobProperties properties;
        try {
            properties = jobConfiguration.loadProperties();
        } catch (BadConfigurationException e) {
            return log.addError(e.getMessage());
        }

        if (!properties.isRunJob()) {
            return log.addSuccess(JOB_IS_DISABLED);
        }

        Employee employee = Employee.load(properties.getStatementUploader());
        FileStatements fileStatements = new Statements(properties.getDirectories());

        FatalConsumer<WipStatement> wipConsumer = wip -> {
            try {
                new UploadableDocument(employee, wip).upload();
                fileStatements.moveToArchiveLocation(wip);
            } catch (FailedToUploadDocumentException e) {
                log.addError(e.getMessage());
                fileStatements.moveToErrorLocation(wip);
            }
        };
        FatalConsumer<InboundStatement> inboundConsumer = inbound -> {
            fileStatements.moveToWipLocation(inbound);
            fileStatements.getWipStatements().forEach(wipConsumer);
        };

        fileStatements.getInboundStatements().forEach(inboundConsumer);

        return log;
    }

    @FunctionalInterface
    public interface FatalConsumer<T> extends Consumer<T> {
        void fatalAccept(T t) throws UnableToMoveFileToDirectoryException, UnableToReadFileException;

        @Override
        default void accept(T t) {
            try {
                fatalAccept(t);
            } catch (UnableToMoveFileToDirectoryException | UnableToReadFileException e) {
                LOGGER.error(UNABLE_TO_CONTINUE, e);
            }
        }
    }
}

Changelog:

• Separate constants to be grouped by category. The logger typically is flush with the class declaration while other constants are grouped below it.
• No need to have the extra space after the start of the execute method
• Use more accurate log levels. Specifically, you overuse "info". Refer to this article about different things to avoid when logging effectively.
  • "DEBUG is intended for messages that could be useful in debugging an issue (ex: method execution started)"
• Use line breaks in fluent calls only when it becomes too long, and then only when you have created your stream/data-structure. Specifically, there's no need to have a line break after fileStatements
• You have a lot of excess indentation and repeated code. Specifically, all of times you catch an exception, log it, and then immediately rethrow it. My changes above resolve that.
• If you are going to statically define the document uploader to be an Employee, naming the variable documentUploader instead of employee is kind of pointless.

I would highly recommend moving FatalConsumer and each of the non-logging constants you have to their own files. Besides that, this is about as good as you're going to get imo. Hope this helps!