Webscraping tennis data 1.2: Optionals, Streams, Callbacks(?)

Question

Through my previous questions, I developed a web-scraper that gets tennis ranking data.

• Webscraping tennis data: major refactor of code style and program modularity
• Webscraping tennis data 1.1: improving exception handling

In this version, I'm back with some modifications for my scraper based on my goals for the overarching project I have in mind. I understand this is a longer post, so I've divided my question into thematic sections - hopefully this makes it easier for readers to follow and provide feedback where they can!

---

What's changed in the code?

1. Rather than scrape ALL the weeks, and return a list of WeeklyResults, the scraper now returns a result for a given week. This enables the scraper to scrape a week, pass it on to another function that utilizes the scraped result. Note that it is not truly "asynchronous" yet - more on that later.

   • To facilitate this, Scraper and MyProject have been modified accordingly.

2. Scraper bug fix #1: certain weeks did not have actual ranking data for the No.1 on the website. Previously, each weekly result was loaded as an Optional in case the player-cell element was empty. However, I had overlooked a case where the first available player-cell was non-empty, but didn't actually belong to the No.1 player.

   • selectNumberOneRankCell in scrapeWeekly resolves this.

3. Scraper bug fix #2: Further inspection showed that the empty WeeklyResults would be between stretches of the reign of a given player. With that trend in mind, plus the general likelihood that the current week's No.1 has a good chance to remain No.1 for the next week (generally), I changed the code to retain the No.1 player from the past week, in the case of an empty scraped result.

   • Added a new field latestResult and modified scrape.

4. WeeklyResult & ScraperException remain unchanged.

---

Code:

scraper Package:

WeeklyResult.java

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.java

package scraper;

public class ScraperException extends Exception {
    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;
    }
}

Scraper.java

package scraper;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
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.time.LocalDate;
import java.util.*;
import java.util.stream.Collectors;

public class Scraper {
    private static final Logger logger = LogManager.getLogger(Scraper.class);
    private final String urlPrefix;
    private final String urlSuffix;
    private final Duration timeout;
    private final int totalTries;
    private WeeklyResult latestResult;

    public Scraper(final String urlPrefix, final String urlSuffix, final Duration timeout, final int totalTries) {
        this.urlPrefix = urlPrefix;
        this.urlSuffix = urlSuffix;
        this.timeout = timeout;
        this.totalTries = totalTries;
        this.latestResult = new WeeklyResult("1973-08-16","N/A");
    }

    public WeeklyResult scrape(final String week) throws ScraperException {
        // in the case the latest scraped data returns an "empty" weekly result, simply retain the latest No.1
        // since it is likely he wouldn't have changed. A weekly result is deemed empty if no player or week info
        // can be found on the ATP page.
        this.latestResult = scrapeWeekly(week)
                .orElse(new WeeklyResult(updateLatestWeekByOne(), this.latestResult.getPlayerName()));
        return this.latestResult;
    }

    private Optional<WeeklyResult> scrapeWeekly(final String week) throws ScraperException {
        final Document document = loadDocument(weeklyResultUrl(week));
        final boolean numberOneDataExists = selectNumberOneRankCell(document).isPresent();
        final Element playerCell = numberOneDataExists ? selectPlayerCellElement(document) : null;

        return Optional.ofNullable(playerCell)
                .map(element -> new WeeklyResult(week, element.text()));
    }

    public 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 {
        Document document = null;
        for (int tries = 0; tries < this.totalTries; tries++) {
            try {
                document = Jsoup.connect(url).timeout((int) timeout.toMillis()).get();
                break;
            } catch (IOException e) {
                if (tries == this.totalTries) {
                    throw new ScraperException("Error loading ATP website: ", e);
                }
            }
        }
        return document;
    }

    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) {
        // refer to https://winterbe.com/posts/2014/07/31/java8-stream-tutorial-examples/
        // and https://www.baeldung.com/java-maps-streams.
        return elements.stream()
                .map(Scraper::extractWeek)
                .filter(week -> Optional.ofNullable(week).isPresent())
                .collect(Collectors.toList());
    }

    private static List<String> noEmptyElseThrow(final List<String> weeks) throws ScraperException {
        if (weeks.isEmpty()) {
            throw new ScraperException("Cannot process empty data from the weeks calendar!");
        } else {
            return weeks;
        }
    }

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

    private static Optional<Element> selectNumberOneRankCell(final Document document) {
        final Element rankCell = selectPlayerRankCell(document);
        return Optional.ofNullable(rankCell).filter(element -> numberOneRankCellExists(element));
    }

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

    private static boolean numberOneRankCellExists(final Element rankCell) {
        return rankCell.text().equals("1");
    }

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

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

    private String updateLatestWeekByOne() {
        return LocalDate.parse(this.latestResult.getWeek()).plusWeeks(1).toString();
    }
}

myproject Package:

MyProject.java

package myproject;

import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.core.config.Configurator;
import scraper.Scraper;
import scraper.ScraperException;
import scraper.WeeklyResult;

import java.time.Duration;
import java.util.List;

// Main class to manage the visualization of player's legacy rankings
public class MyProject {
    private static final Logger logger = LogManager.getRootLogger();

    private static void utilizeScrapedResult(WeeklyResult weeklyResult) {
        // pass the scraped result to the next stage of the visualization logic.
        logger.info("Week: " + weeklyResult.getWeek() + " No.1: " + weeklyResult.getPlayerName());
    }

    public static void main(String[] args) {

        Configurator.setRootLevel(Level.DEBUG);

        final Scraper scraper =
                new Scraper("https://www.atptour.com/en/rankings/singles?",
                        "&rankRange=0-100", Duration.ofSeconds(90), 3);

        // The flow is as follows: scrape the latest weekly results (starting from 1973),
        // then pass it to the ranking logic (IPR). Rinse and repeat
        try {
            final List<String> weeks = scraper.loadWeeks();
            for (String week : weeks) {
                WeeklyResult weeklyResult =  scraper.scrape(week);
                utilizeScrapedResult(weeklyResult);
            }
        } catch (ScraperException e) {
            System.out.println(e.toString());
        }
    }
}

---

Scraper Code: Optionals, Streams and Style Checks

1. I want to make sure I'm not abusing the concept of an Optional. I believe I'm not, since both the player-cell and rank-cell generally hold values relevant for us, but occasionally don't. One thing that was a bit sticky, though, was the fact that I didn't really have a neat way to relate rank-cell elements to player-cells. Logically, I wanted to say: "The rank-cell element is empty if the first available one on the given page is not that of the actual No.1's. Select the player-cell element if the rankCell is actually present." This is the best I could come up with:

   
   final boolean numberOneDataExists = selectNumberOneRankCell(document).isPresent();
   final Element playerCell = numberOneDataExists ? selectPlayerCellElement(document) : null;
   

   It'd be neat to know if there is a better way of achieving this.

2. Have I used Streams properly, specifically in the selectNumberOneRankCell & extractWeeks functions?

3. Any other style concerns would be appreciated. I think the addition of latestResult should be good, please let me know if I'm overlooking something!

---

MyProject Code - Optimizing the Scraper design, Asynchronicity & Callbacks.

NOTE: Since this involves looking at my design, which could be off-topic, I will keep it short. If it is off-topic, please let me know and I'll remove it and repost to a more appropriate site.

In general, the code in MyProject involves chaining separate pieces of logic. Eg. scrape a WeeklyResult, pass it on to utilizeScrapedResult, which does its work and constructs something, say a WeeklyRanking, that is passed to the next logical section and so on. Would my current code structure be efficient to handle this as the number of separate pieces of logic increases, or should I switch to using callbacks as suggested?

• In this context, a given piece of logic would only be dependent on its output in the preceding timestamp. Eg. the WeeklyRanking for week B would have to be preceded by the WeeklyRanking for week A, but the WeeklyResult for week B could be scraped (and stored somewhere) before the WeeklyRanking of week A is computed. On the flip side, a WeeklyResult for week A cannot be constructed after the WeeklyResult of week B. (I forget the mathematical term used to describe this relation...)

---

Feedback on any other aspects of the code that need to be addressed are welcome. If you made it this far, thank you for your time!

Answer 1

Reviewing this myself for the sake of completion.

---

Nits

• Utilize the logger instead of System.out.println. Thus, use logger.error(e.toString()); instead of System.out.println(e.toString());
• loadDocument has been updated with a try-catch to facilitate multiple connection tries. The try catch is thus a necessary evil, but upon revieiwng the code it would be slightly preferable to write it this way:

    private Document loadDocument(final String url) throws ScraperException {
        for (int tries = 0; tries < this.totalTries; tries++) {
            try {
                return Jsoup.connect(url).timeout((int) timeout.toMillis()).get();
            } catch (IOException e) {
                if (tries == this.totalTries) {
                    throw new ScraperException("Error loading ATP website: ", e);
                }
            }
        }
        return null;
    }

(Note that the final return null should never actually execute; it's only there to provide a compile error. A bit ugly but I prefer it over the anti-pattern of setting document null and then modifying it anyways).

---

Optionals & Streams

• Double checking the code, the rationale between having Optional<> type for rankCell elements is reasonable - we filter rankCell elements based on whether the rank value is No.1, and if not, the element should be considered empty for our purposes. Similarly, the boolean logic check for the playerCell element seems ok as well. We only want to consider playerCell if rankCell is non-empty, and even in that case, playerCell could return a null element, so the final return value of an Optional seems ok.

• I have an issue with extractWeeks, specifically of the filter:

        return elements.stream()
                .map(Scraper::extractWeek)
                .filter(week -> Optional.ofNullable(week).isPresent())
                .collect(Collectors.toList());

This code is implying that you want to filter out weeks that are null. This doesn't make sense in light of your logic in scrape, where you seek to either scrape a week's result if it exists, or re-construct week data by using updateLatestWeekByOne(). If you are already handling the case for a week being null, it's pointless to filter out null weeks.

• Building on this, you end up hiding a serious functionality bug in your code. Note that the main loop in MyProject is passing each week in weeks to scraper.scrape. Some of these weeks could be null, OR the data we want for these could be non-existent on the ATP site. What ends up happening in this case is that you "de-synchronize" from weeks in the latter case, as a result you run the risk of your loop prematurely ending. Luckily, your loop "re-synchronizes" since the value of forthcoming weeks are non-empty, but this introduces a new bug: you still have jumps between your timeline! So your logic to get the latestResult is not quite right. You may wish to address this later or immediately, depending on how severe you feel this bug is (although it doesn't result in a fully correct program, 90+ % of the results are correct).

---

Unit Testing

Given such possible bugs, you may wish to look into unit testing the scraper. Moving forward, test-driven development (TDD) may be worth the initial time investment as it can help avoid wasting time on correcting subtle mistakes like this.

---

Scraper 'Chaining' Design

(Disclaimer: still not fully sure about this, and I am considering posting this question to another site: eg. Software Engineering StackExchange)

• The chaining design can be made to work, however it might make the driver in MyProject unwieldy if you scale the number of such tasks to be chained.
• Look into Futures in Java; it seems like you can wrap a partially asynchronous operation into a syncrhonous one, since (I believe) 'get's are blocking. Refer to this for more research.