4
\$\begingroup\$

I incorporated the substantial changes suggested in my previous question that involved building a web-scraper for gathering tennis data.

The improved code is shown below:

Scraper class:

package scraper;

import org.jsoup.Jsoup;
import org.jsoup.nodes.Document;
import org.jsoup.nodes.Element;
import org.jsoup.select.Elements;

import java.io.IOException;
import java.time.Duration;
import java.util.*;
import java.util.stream.Collectors;

public class Scraper {
    private final String urlPrefix;
    private final String urlSuffix;
    private final Duration timeout;

    public Scraper(final String urlPrefix, final String urlSuffix, final Duration timeout) {
        this.urlPrefix = urlPrefix;
        this.urlSuffix = urlSuffix;
        this.timeout = timeout;
    }

    private List<WeeklyResult> scrape() throws ScraperException {
        final List<String> weeks = loadWeeks();

        return loadResults(weeks);
    }

    private List<String> loadWeeks() throws ScraperException {
        final Document document = loadDocument(urlPrefix);
        final Elements elements = selectRankingWeeksElements(document);
        final List<String> weeks = extractWeeks(elements);

        return noEmptyElseThrow(weeks);
    }

    private Document loadDocument(final String url) throws ScraperException {
        try {
            return Jsoup.connect(url).timeout((int) timeout.toMillis()).get();
        } catch (IOException e) {
            throw new ScraperException("Error loading ATP website: " + e.toString());
        }
    }

    private static Elements selectRankingWeeksElements(final Document document) {
        // extract ranking weeks from the dropdown menu
        final Elements result = document.getElementsByAttributeValue("data-value", "rankDate")
                .select("ul li");

        Collections.reverse(result);
        return result;
    }

    private static List<String> extractWeeks(final Collection<Element> elements) {
        return elements.stream()
                        .map(Scraper::extractWeek)
                        .collect(Collectors.toList());
    }

    private static List<String> noEmptyElseThrow(final List<String> weeks) throws ScraperException{
        if (weeks.isEmpty()) {
            throw new ScraperException("Please provide a historical time range! Cannot rank otherwise!");
        } else {
            return weeks;
        }
    }

    private List<WeeklyResult> loadResults(final List<String> weeks) throws ScraperException {
        final List<WeeklyResult> result = new ArrayList<>();
        for (String week : weeks) {
            loadWeeklyResult(week).ifPresent(result::add);
        }
        return result;
    }

    private Optional<WeeklyResult> loadWeeklyResult(final String week) throws ScraperException {
        final Document document = loadDocument(weeklyResultUrl(week));
        final Element playerCell = selectPlayerCellElement(document);
        return Optional.ofNullable(playerCell).map(element -> new WeeklyResult(week, element.text()));
    }

    private String weeklyResultUrl (final String week) {
        return urlPrefix+"rankDate="+week+urlSuffix;
    }

    private static Element selectPlayerCellElement(final Document document) {
        return document.getElementsByClass("player-cell").first();
    }

    private static String extractWeek(final Element li) {
        return li.text().replaceAll("\\.", "-");
    }

    public static void main() throws ScraperException {
        final Scraper scraper =
                new Scraper("https://www.atptour.com/en/rankings/singles?", "&rankRange=0-100", Duration.ofSeconds(90));

        List<WeeklyResult> weeklyResults = scraper.scrape();
        System.out.println(weeklyResults);
        for (final WeeklyResult weeklyResult : weeklyResults) {
            System.out.println("Week: " + weeklyResult.getWeek() + " No.1: " + weeklyResult.getPlayerName());
        }
    }
}

WeeklyResult class:

package scraper;

// A POJO that encapsulates a ranking week and the name of the corresponding No.1 player
public class WeeklyResult {
    private final String week;
    private final String playerName;

    public WeeklyResult(final String week, final String playerName) {
        this.week = week;
        this.playerName = playerName;
    }
    public String getWeek() {
        return week;
    }
    public String getPlayerName() {
        return playerName;
    }
}

ScraperException class:

package scraper;

public class ScraperException extends Exception {
    final String message;
    public ScraperException (String message) {
        this.message = message;
    }
    @Override
    public String toString() {
        return this.message;
    }
}

I had some follow up questions to ensure I've truly understood Marv's feedback from my previous question:

  1. I like the fact that ScraperException can now provide informative messages to handle exceptions, and importantly groups exceptions for all scraper-related logic. The only thing irking me is the try catch block in loadDocument where I wrap the IOException and re-throw it as a ScraperException. I find it to be kind of ugly, to be honest, and was wondering if there was a more efficient method.

  2. Expanding on this, and stemming from Marv's suggestion:

There's something to be said about throwing a checked exception, which might be some further reading points as well.

I did some further research; from what I understand, it seems like some degree of boilerplate try-catch code is a necessary evil while wrapping checked exceptions. Would that be correct?

\$\endgroup\$
2
\$\begingroup\$

I see you used your custom ScraperException exception in two different modes: the first is below:

private static List<String> noEmptyElseThrow(final List<String> weeks) throws ScraperException{
    if (weeks.isEmpty()) {
         throw new ScraperException("Please provide a historical time range! Cannot rank otherwise!");
    } else {
        return weeks;
    }
}

To handle this situation, it could be better use the core java IllegalArgumentException thrown to indicate that a method has been passed an illegal or inappropriate argument and because it is a subclass of RuntimeException and then an unchecked exception there is no need to declare it in the signature of the method. So you could write :

private static List<String> noEmptyElseThrow(final List<String> weeks) {
    if (weeks.isEmpty()) {
         throw new IllegalArgumentException("Please provide a historical time range! Cannot rank otherwise!");
    } else {
        return weeks;
}

Note: from comments' section you can check @Marv suggested that the illegal argument is not directly connected to the user's actions but to the site structure. An alternative to my solution could be to maintain the old ScraperException with a more significative message like "weeks calendar cannot be empty", hiding all other aspects to the user.

The second mode is this:

private Document loadDocument(final String url) throws ScraperException {
    try {
        return Jsoup.connect(url).timeout((int) timeout.toMillis()).get();
    } catch (IOException e) {
        throw new ScraperException("Error loading ATP website: " + e.toString());
    }
}

Because you are not hiding the true cause of the ScraperException adding the string representation of the IOException to your custom exception, you could use the fact that exceptions are chainable throwing a new IOException in this way :

private Document loadDocument(final String url) throws IOException {
    try {
        return Jsoup.connect(url).timeout((int) timeout.toMillis()).get();
    } catch (IOException e) {
        throw new IOException("Error loading ATP website: ", e);
    }
}

Note: from comments' section you can check @Marv suggested to apply chain of exceptions to the old ScraperException. In this case the old ScraperException class could be rewritten like below:

public class ScraperException extends Exception {
    private final String message;
    
    public ScraperException (String message) {
        this.message = message;
    }

    public ScraperException(String message, Throwable cause) {
        super(cause);
        this.message = message;
    }
    
    @Override
    public String toString() {
        return this.message;
    }
}

The method loadDocument consequently could be rewritten in this way:

private Document loadDocument(final String url) throws ScraperException {
    try {
        return Jsoup.connect(url).timeout((int) timeout.toMillis()).get();
    } catch (IOException e) {
            throw new ScraperException("Error loading ATP website: ", e);
    }
}
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ The first error is not directly linked to an illegal argument. It could also be empty if the website had an internal error and produced bad HTML. For your second point, I agree that chaining exceptions is the right thing to do, but you can simply add Exceptions (String, Throwable) constructor to ScraperException and then use that. \$\endgroup\$ – Marv Jun 20 at 11:44
  • \$\begingroup\$ @Marv You got an interesting point, because even if the first method takes an illegal argument (empty list) it is unknown if there is bad HTML or there is a correct HTML and the name of tags are simply changed so an empty list is obtained. Maybe in this case changing the error message like "weeks calendar cannot be empty" would work, For the second one I'm agree with you to have all methods throwing the same ScraperException. Later I add this conversation to my post as a possible alternative to my original code. \$\endgroup\$ – dariosicily Jun 20 at 12:22
  • \$\begingroup\$ one clarifying point @dariosicily, I'm a bit confused when you refer to the "modes" of ScraperException, are you saying that all occurences of ScraperException should be thrown in the same format? That is, it's recommended to use the (String, Throwable) constructor in the "weeks calendar cannot be empty" situation as well? \$\endgroup\$ – cloudy_eclispse Jun 20 at 19:53
  • 1
    \$\begingroup\$ @cloudy_eclispse My words were a bit misleading, I was referring to the two different situations you are handling using the same exception in two different contexts. I started from the idea of using the already existing core java exceptions to solve the two problems, but after comments by marv , I am convinced that your idea of all methods throwing the same custom exception is correct because the user doesn't need to know all the details. I decided to leave the first solution just for comparison with the second solution. \$\endgroup\$ – dariosicily Jun 20 at 20:37
  • \$\begingroup\$ @cloudy_eclispse if you catch an Exception to throw one of a different type, use an Exception constructor that accepts the previous Exception. This way, the Exception you throw will have the whole stack trace. \$\endgroup\$ – Marv Jun 23 at 11:51

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.