2
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ When posting here, try to specify details that are not clear from your exact code snippet. For instance, I have the following questions: 1. What logger are you using? 2. What platform is this for (make sure to add tags too)? 3. We have no idea about what almost all of those classes are. I did a quick Google for FileStatements for example and got no results. We can't evaluate your code without knowing what your code does. \$\endgroup\$ – Michael Ziluck Aug 5 at 22:27
  • \$\begingroup\$ I was hopping that the code is readable anough and that it is not important to reviewers which particular logger, platform (do you mean OS?) I use. You cannot see implementation details of FileStatements and other classes, but I was hoping that it is clear what it does by looking at the methods it has and what they return. I could upload the entire project to github with all the classes but I think no one would be interested in such extensive code review. I might be wrong, let me know. \$\endgroup\$ – OSGI Java Aug 5 at 22:44
  • \$\begingroup\$ The main concern is that when posting on here, people like to be able to load it into an IDE as no one codes in a text editor anymore. Most auto-formatters tend to fail when they aren't able to compile things. I'm working on putting a Gist together that would have some dummy classes that makes it so I can load this locally. I'll add that in a comment when I'm done. When I first started posting here I didn't really understand what that entailed so my follow-up should help you understand what people want in the future :) \$\endgroup\$ – Michael Ziluck Aug 5 at 22:54
  • \$\begingroup\$ I see your point now. Thank you, I will do better next time. I very much appreciate your effort! \$\endgroup\$ – OSGI Java Aug 6 at 1:35
  • \$\begingroup\$ I added a description what the code does. I know it is too late... \$\endgroup\$ – OSGI Java Aug 6 at 1:52
2
\$\begingroup\$

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!

\$\endgroup\$

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.