4
\$\begingroup\$

I'd like comments on the following code, explanations, etc. are given in the javadoc:

package ocr.base;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.util.Arrays;
import java.util.Objects;
import java.util.Optional;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.function.Consumer;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
 * Handles the checking of a directory at an interval.
 * 
 * This interval is variable and is to be found in the config file.
 * The found files get processed via a file consumer.
 * The config file is only opened if the last modified date has changed.
 *
 * @author Frank van Heeswijk
 */
public abstract class BaseChecker implements Runnable {
    /** Scheduler to run the file checking on. **/
    private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);

    /** The directory to be checked. **/
    private final File directory;

    /** A consumer that accepts the files in the directory. **/
    private final Consumer<File> fileConsumer;

    /** The configuration file. **/
    private final File configFile;

    /** The runtime sub class of BaseChecker. **/
    private final Class<? extends BaseChecker> subClass;

    /** The current duration, changes only if the file has been modified. **/
    private Duration duration;

    /** The last modified date of the last readout of the config file. **/
    private long configLastModified;

    /**
     * Constructs this object.
     * 
     * @param directory The directory to be checked.
     * @param fileConsumer  The consumer that accepts the files.
     * @param configFile    The configuration file.
     */
    public BaseChecker(final File directory, final Consumer<File> fileConsumer, final File configFile) {
        this.directory = Objects.requireNonNull(directory);
        this.fileConsumer = Objects.requireNonNull(fileConsumer);
        this.configFile = Objects.requireNonNull(configFile);
        this.subClass = this.getClass();
    }

    /**
     * Checks the directory now and calls innerRun().
     */
    @Override
    public void run() {
        checkDirectory();
    }

    /**
     * Gets the duration from the config file and schedules one execution of checkDirectory() at that time.
     * 
     * It only actually opens the config file if it has been modified.
     */
    private void innerRun() {
        if (configFile.lastModified() != configLastModified) {
            duration = durationFromConfig();
        }
        scheduler.schedule(this::checkDirectory, duration.period, duration.unit);
    }

    /**
     * Checks the directory and calls the consumer for every file, and then calls innerRun().
     */
    private void checkDirectory() {
        Arrays.stream(directory.listFiles()).forEach(fileConsumer);
        innerRun();
    }

    /**
     * Returns the duration from the config file.
     * 
     * @return  The duration.
     */
    private Duration durationFromConfig() {
        try {
            return durationFromConfigInner();
        } catch (IOException ex) {
            throw new IllegalStateException("The config file (\"" + configFile.getPath() + "\") has not been found.");
        }
    }

    /**
     * Returns the duration from the config file.
     * 
     * Searches the log file for the first line indicating the config entry for this instance.
     * 
     * @return  The duration.
     * @throws FileNotFoundException If the config file has not been found.
     */
    private Duration durationFromConfigInner() throws IOException {
        String entry = subClass.getSimpleName() + "=";
        Optional<String> optional = Files.newBufferedReader(configFile.toPath(), Charset.forName("UTF-8")).lines()
                .filter(s -> s.startsWith(entry))
                .map(s -> s.replaceAll(" ", ""))
                .findFirst();
        configLastModified = configFile.lastModified();
        if (!optional.isPresent()) {
            throw new IllegalStateException("Entry (\"" + entry + "\") has not been found in the config file.");
        }
        return Duration.of(optional.get().replace(entry, ""));
    }

    /**
     * Record class to hold the duration.
     */
    private static class Duration {
        /** The period of the duration. **/
        public final int period;

        /** The time unit of the duration. **/
        public final TimeUnit unit;

        /**
         * Constructs the duration.
         * 
         * @param period    The period.
         * @param unit  The time unit.
         */
        public Duration(final int period, final TimeUnit unit) {
            this.period = period;
            this.unit = Objects.requireNonNull(unit);
        }

        /**
         * Returns a duration based on a string value.
         * 
         * The implementation accepts all strings starting with an integer number and ending on a 's', 'm', 'h' or 'd'.
         * These characters respectively denote seconds, minutes, hours and days.
         * 
         * @param value The string value to be converted
         * @return  The duration that the string value represented.
         */
        public static Duration of(final String value) {
            Objects.requireNonNull(value);
            if (value.length() < 2) {
                throw new IllegalArgumentException("Invalid duration (\"" + value + "\").");
            }

            //period
            int period = Integer.parseInt(value.substring(0, value.length() - 1));
            if (period <= 0) {
                throw new IllegalArgumentException("Invalid period in value (\"" + value + "\").");
            }

            //unit
            char unit = value.charAt(value.length() - 1);
            TimeUnit returnUnit;
            switch (unit) {
                case 's':
                    returnUnit = TimeUnit.SECONDS;
                    break;
                case 'm':
                    returnUnit = TimeUnit.MINUTES;
                    break;
                case 'h':
                    returnUnit = TimeUnit.HOURS;
                    break;
                case 'd':
                    returnUnit = TimeUnit.DAYS;
                    break;
                default:
                    throw new IllegalArgumentException("Invalid unit in value (\"" + value + "\").");
            }
            return new Duration(period, returnUnit);
        }

        @Override
        public String toString() {
            return "Duration(" + period + ", " + unit + ")";
        }
    } 
}
\$\endgroup\$
1
\$\begingroup\$

Duration.of

With switch statements I find it pleasing to return-early, and avoid multiple lines of code. With switch statements especially, the early-return removes the need for an ugly break as well. Consider this method:

   public static Duration of(final String value) {

        Objects.requireNonNull(value);
        if (value.length() < 2) {
            throw new IllegalArgumentException("Invalid duration (\"" + value + "\").");
        }

        //period
        int period = Integer.parseInt(value.substring(0, value.length() - 1));
        if (period <= 0) {
            throw new IllegalArgumentException("Invalid period in value (\"" + value + "\").");
        }

        switch (value.charAt(value.length() - 1)) {
            case 's':
                return new Duration(period, TimeUnit.SECONDS);
            case 'm':
                return new Duration(period, TimeUnit.MINUTES);
            case 'h':
                return new Duration(period, TimeUnit.HOURS);
            case 'd':
                return new Duration(period, TimeUnit.DAYS);
            default:
                throw new IllegalArgumentException("Invalid unit in value (\"" + value + "\").");
        }
    }

File vs. Path

You should try to stick with one, or the other, but changing between them is not cool.

Your BaseChecker class takes Files as inputs, but internally, it does, in places, convert them to Paths, and then processes the Path values.

If you are using Path, then stick with it. Actually, it is not that simple, but you should consider using other features of Path like the Watchable interface it implements.... this would remove the need to poll the state of the config file.

Charsets

You have:

Charset.forName("UTF-8")

This is most conveniently replaced with:

StandardCharsets.UTF_8

which avoids some lookup overhead.

But you can directly use Files.newBufferedReader(configFile.toPath()) in your durationFromConfigInner function

findFirst vs. findLast

You look for the config value the first time it is specified, but, standard java properties files, and other config systems, typically rely on the last occasion a value is set. Make sure your code is consistent with expectations

This will likely not matter for you, but it may.

listFiles()

You have:

Arrays.stream(directory.listFiles()).forEach(fileConsumer);

I am not particularly happy with this structure. It requires a lot of work to build the listFiles Array, and the intention of the method is to filter it before consuming it, if needed.

Perhaps your code is intended to return all files, but, I would expect it to be more sensible to supply a filter for the listFiles, or even better, to use the new stream API method call Files.list(path)

Files.list(path).forEach(pathConsumer);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I agree with most, but must add to the watcher services. Watcher services are not safe in a production environment, they will miss files. Moreover, it is JVM/OS-dependant which is quite a big no-no. In production you will end up making your own emergency poller (possibly 1 min loop on top of the watcher service) anyway, so I decided to just get rid of the watcher altogether. \$\endgroup\$ – skiwi Mar 14 '14 at 10:02
  • \$\begingroup\$ fair point about the watcher... \$\endgroup\$ – rolfl Mar 14 '14 at 12:01

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.