Interview Code Test: Webservice Checker

Question

I recently applied for a job as a C# developer. Based on the first phone call, they wanted to check my skills with a code interview in Java. Since I haven't programmed Java for years, I tried my best, and I would like to submit my solution for review (for personal improvement) because I believe a programming language is just a tool a programmer uses to solve the problem. Please also tell me what you think about the documentation, since it is my first time using one of these documentation tools.

The problem was defined as the following (translated from German):

Goal

Implement a Web service Checker.

Functional Requirements

The application is required to be capable of checking the availability of an URL. For that, the user starts the program and inserts a URL of a WWW resource followed by the enter key. After that the program checks every 30 seconds whether the URL is accessible and logs the result continuously to a local log file.

Example Output

15-SEP-2011 14:00:30 Uhr : http://mynode/login.html -> available!
15-SEP-2011 14:01:00 Uhr : http://mynode/login.html -> not available!
15-SEP-2011 14:01:30 Uhr : http://mynode/login.html -> available!

After the insertion of the URL the program shows the following message:

http://mynode/login.html is checked, press any key to terminate the program...

Technical Hints

• Console Application
• Comments if meaningful
• Choice of classes: feel free.

---

Code:

public class Main {

    public static void main(String[] args) {
        try (InputReader reader = new InputReader()) {
            URL url = reader.readUrl();
            WebserviceObserver observer = new WebserviceObserver(url);
            observer.observe();
            System.out.println(String.format("%s is checked, press any key to terminate the program...", url.toString()));

            reader.waitForKeypress();
            observer.stopObserving();
        } catch (IOException exception) {
            System.out.println("It was not possible to start the program.");
        }
    }
}

public class InputReader implements Closeable {

    /**
     * The raw terminal for single user keypass which do not require the <enter>-key.
     */
    final private Terminal terminal;

    InputReader() throws IOException {
        this.terminal = getRawTerminal();
    }

    /**
     * Creates a raw terminal which enables non-buffered key inputs.
     * This enables the detection of single character inputs without pressing the <code>enter</code>-key.
     * <p>
     * Hint: For the IntelliJ console, this method returns a {@link org.jline.terminal.impl.DumbTerminal}, which means
     * that the terminal has no advanced functionality.
     * Therefore, the {@link org.jline.terminal.impl.DumbTerminal} is still buffered and requires the <code>enter</code>-key after single character inputs.
     * <p>
     * Jansi is required for system compatibility, especially for windows.
     *
     * @return a {@link Terminal} object with raw mode enables, if terminal creation is successful, otherwise <code>null</code>
     */
    private static Terminal getRawTerminal() throws IOException {
        Terminal terminal = TerminalBuilder.builder()
                .jansi(true)
                .system(true)
                .build();
        terminal.enterRawMode();
        return terminal;
    }

    /**
     * Requests the user to input an URL until a valid URL is obtained.
     *
     * @return a valid URL.
     */
    public URL readUrl() {
        URL url = null;
        // Create a line reader for the raw terminal
        LineReader reader = LineReaderBuilder.builder().terminal(terminal).build();
        while (url == null) {
            System.out.println("Bitte geben sie eine gültige URL ein:");
            String urlInput = reader.readLine();
            url = validateUrl(urlInput);
        }

        return url;
    }

    /**
     * Converts the string-based URL into a URL object if the given URL is valid. Otherwise, returns null.
     *
     * @param urlString the URL to validate.
     * @return A URL if the input is a valid URL, otherwise null.
     */
    private URL validateUrl(String urlString) {
        URL url = null;

        try {
            url = new URL(urlString);

            // if the user types an url like:
            // http:example.com, then this will cause a connection error because the host
            // is an empty string, but will not cause a MalformedURLException
            String host = url.getHost();
            if (host.isEmpty()) {
                url = null;
            }
        }
        catch (MalformedURLException ignored) {
            // MalformedURLException is also throws if http:// or https:// is missing
            // in case of an exception, url stays null
        }

        if(url == null) {
            String errorMessage = String.format("The URL %s is invalid.", urlString);
            System.out.println(errorMessage);
        }

        return url;
    }

    /**
     * Blocks the program until the user presses any key.
     */
    public void waitForKeypress() {
        try {
            int notUsed = terminal.reader().read();
        } catch (IOException exception) {
            System.out.println("Because of an error it was not possible to wait for the user input.");
        }
    }

    @Override
    public void close() throws IOException {
        this.terminal.close();
    }
}

public enum WebserviceStatus {
    AVAILABLE("available"),
    NOT_AVAILABLE("not available");

    /**
     * Representation of the enum value as a string
     */
    private final String textRepresentation;

    WebserviceStatus(String textRepresentation) {
        this.textRepresentation = textRepresentation;
    }

    public String getTextRepresentation() {
        return textRepresentation;
    }
}

public class WebserviceStatusChecker {

    /**
     * This is the user agent string for the chrome web browser, required to handle HTTP requests with URLs containing "www".
     *
     * @see <a href="http://www.useragentstring.com/pages/useragentstring.php">User agents</a>
     */
    private static final String userAgent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 Safari/537.36";

    /**
     * The HTTP request method.
     * <p>
     * HEAD was chosen over GET because of efficiency.
     *
     * @see <a href="https://www.w3schools.com/tags/ref_httpmethods.asp">HTTP request methods</a>
     */
    private static final String requestMethod = "HEAD";

    /**
     * The URL to check.
     */
    private final URL url;

    /**
     * The timeout duration for the HTTP request.
     */
    private final Duration timeoutDuration;

    /**
     * Creates a new instance of {@link WebserviceStatusChecker}
     *
     * @param url the URL which is checked.
     */
    WebserviceStatusChecker(URL url) {
        this(url, Duration.ofMillis(2000));
    }

    /**
     * Creates a new instance of {@link WebserviceStatusChecker}
     *
     * @param url             the URL to check.
     * @param timeoutDuration the timeout duration for the HTTP request.
     */
    WebserviceStatusChecker(URL url, Duration timeoutDuration) {
        this.url = url;
        this.timeoutDuration = timeoutDuration;
    }

    /**
     * Sends a HTTP request to check whether the webservice is available or not.
     *
     * @return {@link WebserviceStatus#AVAILABLE} if the HTTP request is successful, otherwise {@link WebserviceStatus#NOT_AVAILABLE}
     * @see <a href="https://stackoverflow.com/questions/3584210/preferred-java-way-to-ping-an-http-url-for-availability">StackOverflow: Ping a HTTP URL for availability</a>
     */
    public WebserviceStatus checkWebserviceStatus() {
        WebserviceStatus status = WebserviceStatus.NOT_AVAILABLE;
        try {
            HttpURLConnection connection = (HttpURLConnection) this.url.openConnection();
            int timeoutInMilliseconds = (int) this.timeoutDuration.toMillis();

            // Required, otherwise some HTTP requests from URLs containing "www",
            // e.g., https://www.google.com/search?q=java are rejected.
            // see https://stackoverflow.com/questions/2793150/how-to-use-java-net-urlconnection-to-fire-and-handle-http-requests
            // for more information about user agents
            connection.setRequestProperty("User-Agent", userAgent);
            connection.setConnectTimeout(timeoutInMilliseconds);
            connection.setReadTimeout(timeoutInMilliseconds);
            connection.setRequestMethod(requestMethod);

            int responseCode = connection.getResponseCode();
            if (isCorrectHttpStatusCode(responseCode)) {
                status = WebserviceStatus.AVAILABLE;
            }
        } catch (IOException ignored) {
            // In the case of any exception the method returns WebserviceStatus.NOT_AVAILABLE
        }

        return status;
    }

    /**
     * Indicates whether the response code is correct.
     *
     * @param httResponseCode
     * @return <code>true</code> if the code is in the interval [200, 399], otherwise <code>false</code>
     * @see <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status">HTTP response status codes</a>
     */
    private boolean isCorrectHttpStatusCode(int httResponseCode) {
        return httResponseCode <= 399 && httResponseCode >= HttpURLConnection.HTTP_OK;
    }
}

public class WebserviceStatusLogger {

    /**
     * The path to the file which is written by the {@link WebserviceStatusLogger}.
     */
    final String filePath;

    /**
     * The URL which is logged to the file.
     */
    final URL url;

    /**
     * Creates a new instance of {@link WebserviceStatusLogger}, which writes the {@link WebserviceStatus} of the url.
     *
     * @param url  the url which was checked.
     * @param path the path to the log file. If the file exists, the logger appends to the file, otherwise a new file is created.
     */
    WebserviceStatusLogger(URL url, String path) {
        this.filePath = path;
        this.url = url;
    }

    WebserviceStatusLogger(URL url) {
        this(url, "Webservice Availability.log");
    }

    /**
     * Logs the status of the webservice with a timestamp.
     *
     * @param status Availability of the webservice.
     */
    public void writeWebserviceStatus(WebserviceStatus status) {
        Date now = Calendar.getInstance().getTime();
        DateFormat dateFormat = new SimpleDateFormat("dd-MMM-yyyy hh:mm:ss");
        String dateText = dateFormat.format(now).toUpperCase();
        String urlText = url.toString();
        String statusText = status.getTextRepresentation();
        String content = String.format("%s Uhr : %s -> %s!", dateText, urlText, statusText);
        this.write(content);
    }

    /**
     * Writes the content into the log file.
     *
     * @param content The content which is written to the file.
     * @see <a href=" https://stackoverflow.com/questions/1625234/how-to-append-text-to-an-existing-file-in-java">StackOverflow: Append to an existing file</a>
     */
    private void write(String content) {
        // This implementation closes the fileWriter with every write.
        // Therefore, this approach trades performance for the ability to log from multiple processes
        // to the same log file, since the file lock is released after every write.
        try (FileWriter fileWriter = new FileWriter(this.filePath, true);
             BufferedWriter bufferedWriter = new BufferedWriter(fileWriter);
             PrintWriter printWriter = new PrintWriter(bufferedWriter)) {
            printWriter.println(content);
        } catch (IOException ioException) {
            System.out.println(String.format("An error occurred: %s", ioException.toString()));
        }
    }
}

/**
 * Hint: First this was implemented with a {@link ScheduledExecutorService};
 * however, the solution was highly unreliable regarding the timespan between thread spawns.
 * This problem was only detected on Windows.
 */
public class PeriodicRunner implements Runnable {

    /**
     * Specifies the interval in which the {@link WebserviceStatus} is checked an logged.
     */
    final private Duration interval;

    /**
     * The method which is run periodically
     */
    final private Consumer<Void> runnerFunction;

    PeriodicRunner(Consumer<Void> runnerFunction, Duration interval) {
        this.runnerFunction = runnerFunction;
        this.interval = interval;
    }

    PeriodicRunner(Consumer<Void> runnerFunction) {
        this(runnerFunction, Duration.ofSeconds(30));
    }

    /**
     * Runs the function in the interval by measuring the time consumed to execute the function.
     * Thread execution stops when the thread is interrupted by the main thread.
     */
    @Override
    public void run() {
        boolean isInterrupted = false;
        while (!isInterrupted) {
            long startTime = System.nanoTime();
            this.runnerFunction.accept(null);
            long endTime = System.nanoTime();
            Duration executionDuration = Duration.ofNanos(endTime - startTime);
            Duration duration = this.interval.minus(executionDuration);
            isInterrupted = this.sleepUntilEndOfDurationOrInterrupted(duration);
        }
    }

    /**
     * Sets the thread sleeping until it is woken up by the scheduler or by an interrupt.
     *
     * @param duration the sleep duration.
     * @return <code>true</code> if the thread was interrupted while sleeping, otherwise <code>false</code>.
     */
    boolean sleepUntilEndOfDurationOrInterrupted(Duration duration) {
        boolean isInterrupted = false;
        try {
            Thread.sleep(duration.toMillis());
        } catch (InterruptedException interrupt) {
            // Thread was interrupted for termination.
            isInterrupted = true;
        }

        return isInterrupted;
    }
}

public class WebserviceObserver {

    /**
     * The logger which writes the {@link WebserviceStatus}.
     */
    final WebserviceStatusLogger logger;

    /**
     * The {@link WebserviceStatusChecker} which obtains the {@link WebserviceStatus} based on the URL.
     */
    final WebserviceStatusChecker checker;

    /**
     * The thread which is responsive for executing the periodic {@link WebserviceStatus} check and logging.
     */
    final Thread thread;

    WebserviceObserver(URL url) {
        this.logger = new WebserviceStatusLogger(url);
        this.checker = new WebserviceStatusChecker(url);
        Runnable periodicRunner = new PeriodicRunner((Void notUsed) -> this.logWebserviceStatus());
        this.thread = new Thread(periodicRunner);
    }

    /**
     * Starts the thread.
     */
    public void observe() {
        this.thread.start();
    }

    /**
     * Stops the thread.
     */
    public void stopObserving() {
        this.thread.interrupt();
    }

    /**
     * Obtains the webservice status and writes the measured state to the log file.
     */
    private void logWebserviceStatus() {
        WebserviceStatus status = this.checker.checkWebserviceStatus();

        // It is unlikely that the output in the log file will be the exact specified interval, because the execution of
        // the webservice status takes additional time.
        // Only after the check is finished, the result is logged.
        // The expectation is that the additional time cannot exceed the timeout duration of the Checker.
        this.logger.writeWebserviceStatus(status);
    }
}

Answer 1

First impression

Wow, there's a lot of code for a task that seems quite simple at the first glance. That's not necessarily a bad thing as I do not know what the interviewers are looking for.

Main class

You have seven different classes but still there is logic in the main method. I would have wanted to see the main method just contain a single setup call with the data input source as a parameter. That way all your logic would be contained in a single reusable component that can be called form any context, not just from command line. Something like this:

public static void main(String[] args) {
    WebServiceChecker checker = new WebServiceChecker(System.in);
    checker.start();
}

There are a couple of places where you fail to log the exception stack trace. This is close to non-hireable offense. We have literally lost money because of programmers doing this. Imagine having a problem in production and error logs just containing a basic message instead of exact error location. It's christmas shopping period so production is in freeze mode and no updates can be deployed to add more logging, so now we have to go through all kinds of bureucracy to get stuff done.

} catch (IOException exception) {
    System.out.println("It was not possible to start the program.");
}

Since you are already depending on non-standard libraries, you should have used a logging framework (e.g. Log4j) and added the exception object as the last parameter:

LOG.error("It was not possible to start the program.", exception);

Also, if errors are not logged with a framework, they should be written to System.err.

InputReader

By using the Terminal here you have hard coded the program into a single input source and added a dependency to a quite specialized library. While you are still within the spec, I feel that you have made the functionality more restricted than it should be.

The InputReader has a very generic name but it peforms two very specific functions: read a line from from terminal and meke sure it is an URL. Should you keep the dependency to Terminal instead of just using BufferedReader, it should be split into two classes and renamed to have nmes that describe their function.

A few minor issues are mixing german and english in the messages (I'm hoping you just forgot it when translating it to english) and returning null as an error in the validateURL method. The validator method is now responsible for messaging the user. Instead it should throw an exception on an error and the readURL should catch and process it. Or if you refactor the main class as mentioned above, the readURL should not contain a loop. The readURL should just throw the errors to the caller and the caller shoul be responsible for looping and error handling. This way the readURL would become more generic and reusable.

WebserviceStatus

It is pretty common to mix human readable messages into enumerations. I dislike this practise as it adds responsibilities to the enum, breaking the single responsibility principle. It makes the enums heavy and hard to maintain. This practise locks the implementation into single language. It is better just have the enums as plain as possible and mnage the user messages in a class that is responsible for the user interfacel.

WebserviceStatusChecker

You're not following Java naming standards here. Constants are supposed to be named with UPPER_SNAKE_CASE. Unlike other constants, the default duration is a magic value in the constructor. The IOException should not be inored. It should be logged on info or warn level. It would important to know if a website is unreachable because of network issues.

WebserviceStatusLogger

The fields should be private. The output file name is something that in my opinion should not have a default value as it has permanent effect on the running environment.

The write method shows you're only ever used Windows systems. Opening a file does not lock it in Unix-based systems. You've gone out of your way to implement a feature that was not requested and not written it correctly.

PeriodicRunner

You've documented that you tried the builtin executors, failed to get them working and decided to reinvent them. The only thing worse you could have done was to explicitly claim that there was a bug in the most used parts of the standard library instead of looking at your own code. And after writing your own implmentation you have added a comment that it does not fix wht you were set to fix: the unreliable timing interval.

WebserviceObserver

I had to stop for a while to figure out the purpose of this class. An observer would be a component that receives information, but this one has control over the whole application. The class needs to be renamed so that the name implies it is a controller.

General

Programming lanuages may be "just tools" but they are complicated tools that require specific knowledge to be used correctly. Your code shows that you understand that code should be modular but there's a bit of a gap into how it should be done. The responsibilities placed on classes and methods is not very clear. E.g. interfacing with the user is spread throughout the code. Studying the SOLID principles would help in this. Your classes are often responsible for acquiring their dependencies themselves making them hard to reuse. Instead they should receive dependencies in the constructor. The D in SOLID explains this in more depth.

Your documenting is pretty good. You've commented the actual reason and source material for the special user agent header. That was something I don't see every day. Class and method naming could use improvement as they are often quite generic. Names should describe the function and purpose of the item.

Answer 2

In addition to @TorbenPutkonen's review here are my comments:

In general, the code seems well structured, readable and understandable.

Tests

While there is a complex logic in your code, the tests are not implemented. Without tests most interviewers would consider the task not completed (as long as it is not an Intern or a Junior position).

Logs

Although the tasks stays that the program should interact via console it is a bad idea to rely on System.out.println in all places. Instead, you could use any kind of logger or create your own abstraction on top. You would understand that it is an issue if you tried to unit test this code because stdout communications are hard to mock.

Exceptions for code flow

It is a bad practice to use exceptions for code flow. Read more here. You have ignored exceptions in several places which are not ignored in fact. You have an additional logic (returning null, or printing a message) if an exception occurs.

Logic in constructors

Constructor of class InputReader calls an instance method in it's body. Although in you case it doesn't harm, it is considered a bad practice. Read why here. Instead you can create a factory method to create an instance of Termainl and pass it to a private constructor of InputReader.

Single responsibility

If I come across a method called validateUrl I expect that it validates urls. Instead, the method converts a String to a URL object. In some cases it prints some information to stdout. This is an example of a violation of the single responsibility principle. Instead, the method should do only one thing: it either validates or converts, but not both.

Self-documenting code

I see a lot of comments and javdocs in your code which help understand the logic. At the same time, if you constantly face a need to explain your code it is a sign of a bed design. In other words, if a reader is required to read a documentation o a method body in order to understand what it does it is a bad design. Read more on the subject here. As an alternative, you can carefully name your variables and methods, keep them simple and you won't need to explain your code. As a rule if you want to comment a peace of code, extract it to a method and give it a good name (even if these are just 1-2 lines of code).

Complexity

It is hard to judge without implementing myself or knowing all preconditions, but the code seems to be a way more complex that it could be. I would suggest you to go to GitHub and search by tag specifying the company name and I am pretty sure you'll find another solutions of the same task implemented by other folks. Juts compare them with yours.

Ich wünsche Ihnen viel Spaß!