8
\$\begingroup\$

So as a starter Java project I decided to web scrape some data (specifically all historically No. 1 ranked players for weeks starting from 1973) from the ATP website, and do something with it (IPR). I'm in the process of refactoring my working web scraper and wanted some feedback.

  • Currently my scraper retrieves the No.1s - or so it seems. I haven't tested it apart from just printing it to my console and verifying it that way. One thing I feel is that I can tighten some of the exception handling, but I wasn't sure how what test cases to develop in JUnit for that. Any tips?

  • More importantly, feedback on the code style would be really appreciated! The bulk of my code is in Scraper (duh), but I'm not sure I'm too comfortable with having various static methods. That being said, a sprawling main function is not ideal either, especially when there are separable pieces of the logic that the scraper performs. Does this indicate I need to somehow break the Scraper design into smaller objects? What be a good design practice?

  • Any other feedback, especially related to best practices and idioms in Java would be appreciated (I come from a primarily C & C++ background).

Here's my code:

Scraper:

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.util.ArrayList;
import java.util.Collections;

public class Scraper {
    public static void main() {
        final String ATP_URL_PREFIX = "https://www.atptour.com/en/rankings/singles?";
        final String ATP_URL_SUFFIX = "&rankRange=0-100";
        // get the list of historical ranking weeks - basically from 1973-present.
        ArrayList<String> weeks = new ArrayList<String>();
        weeks = getWeeksForRankings(ATP_URL_PREFIX, weeks);
        // weeks might be null if no valid HTML
        if (weeks.size() == 0) {
            System.out.println("Please provide a historical time range! Cannot rank otherwise!");
            return;
        }
        getPlayerNames(ATP_URL_PREFIX, ATP_URL_SUFFIX, weeks);
    }
    
    static ArrayList getWeeksForRankings(String url, ArrayList<String> weeks) {
        try {
            final Document document = Jsoup.connect(url).get();
            // extract the series of list items corresponding to the ranking weeks, from the dropdown menu
            Elements rankingWeeksList = document.getElementsByAttributeValue("data-value", "rankDate").select("ul li");
            for (Element li : rankingWeeksList) {
                // for accessing the relevant week's ranking page later, the rankDate= param in the URL takes '-'s
                // instead of dots so we replace the characters here and then add them to out list.
                String week = li.text().replaceAll("\\.", "-");
                weeks.add(week);
            }
        } catch (IOException e) {
            System.out.println("Error while connecting and parsing HTML: " + e);
            System.exit(1);
        } catch (Exception e) {
            System.out.println("Fatal Error: " + e);
            System.exit(1);
        }
        Collections.reverse(weeks); // start from 1973.
        return weeks;
    }

    static void getPlayerNames(String url_prefix, String url_suffix, ArrayList<String> weeks) {
        // dynamically update a player's ranking and animate his status
        for (String week : weeks) {
            String url = url_prefix+"rankDate="+week+url_suffix;
            try {
                final int SECONDS_TO_MILLISECONDS = 1000;
                // time out is an issue. ideally, try mutliple times to get the data??
                final Document document = Jsoup.connect(url).timeout(180 * SECONDS_TO_MILLISECONDS).get();
                Element player = document.getElementsByClass("player-cell").first();
                if (player == null) {
                    continue;
                } else {
                    System.out.println("Week: " + week + " No.1: "+ player.text());
                }
            } catch (IOException e) {
                System.out.println("Error while connecting and parsing HTML: " + e);
                System.exit(1);
            }
        }
    }
}

Main Driver:

package tennisProject;

import Scraper.Scraper;

public class TennisProject {
    public static void main(String[] args) {
        Scraper.main();
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Please consider posting a new question as follow-up instead, feel free to add links back-and-forth for bonus context. \$\endgroup\$ – Mast Jun 19 at 20:08
  • \$\begingroup\$ oh, sorry I wasn't aware of that! I'll post a new question as a follow up, thank you! \$\endgroup\$ – cloudy_eclispse Jun 19 at 20:12
7
\$\begingroup\$

Some style issues first:

  • Package names should be all lowercase ASCII letters. No camelCase, PascalCase, snake_case or kebab-case. So tennisproject and scanner.

  • Local variables should never be uppercase SNAKE_CASE, but camelCase. So atpUrlPrefix instead of ATP_URL_PREFIX and so on. You probably want those to be class constants anyways, which use uppercase SNAKE_CASE. These are fields that are private static final.

  • The same is true for parameters. Always camelCase. So urlPrefix url_prefix and so on.

  • Don't declare a method called main that isn't actually a Java style main method. It's confusing. You can get rid of the TennisProject class all together.


Some notes on code snippets before I present a "cleaned up" version

ArrayList<String> weeks = new ArrayList<>();
weeks = getWeeksForRankings(ATP_URL_PREFIX, weeks);

No need to create a list and pass it to the method here. Remove the list parameter and have the method create the list. Also change the return type of getWeeksForRankings from ArrayList to List<String>. Raw type usage is discouraged, and there is usually no need for the caller to know which list implementation is returned. The same is true for the parameter. Use the broadest type of Collection possible.


} catch (IOException e) {
    System.out.println("Error while connecting and parsing HTML: " + e);
    System.exit(1);
} catch (Exception e) {
    System.out.println("Fatal Error: " + e);
    System.exit(1);
}

(Re)throw the exception(s) after handling them (in your case, handling them is just printing out an error message) if the error is unrecoverable instead of using System.exit and let the caller handle the exception. In your case, it would just be the runtime terminating the application.


if (weeks.size() == 0) {

Use weeks.isEmpty() instead.


"Cleaned up" code

Now, I would make it so that Scanner is an instantiable class with instance methods. That way you can create multiple instances and pass different parameters if needed.

First, we add a Result POJO:

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;
    }
}

Now, the cleaned up Scraper class. The changes are substantial, so please read the explanation below.

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;
    }

    public List<WeeklyResult> scrape() throws IOException {
        final List<String> weeks = loadWeeks();

        return loadResults(weeks);
    }

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

        return notEmptyElseThrow(result);
    }

    private Document loadDocument(final String url) throws IOException {
        return Jsoup.connect(url).timeout((int) timeout.toMillis()).get();
    }

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

    private List<WeeklyResult> loadResults(final List<String> weeks) throws IOException {
        final List<WeeklyResult> result = new ArrayList<>();

        for (final String week : weeks) {
            loadWeeklyResult(week).ifPresent(result::add);
        }

        return result;
    }

    private Optional<WeeklyResult> loadWeeklyResult(final String week) throws IOException {
        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 String extractWeek(final Element li) {
        return li.text().replaceAll("\\.", "-");
    }

    private static Elements selectRankingWeeksElements(final Document document) {
        final Elements result = document.getElementsByAttributeValue("data-value", "rankDate")
                                        .select("ul li");

        Collections.reverse(result);
        return result;
    }

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

        return weeks;
    }

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

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

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

You will notice that there are a lot of methods, but all methods are very small. In fact they are so small that no method has more than four lines of actual code.

Nobody expects you to do this right of the bat as a novice, but it is something you can strive towards. Notice that the code got longer, which many people think is a bad thing. It isn't. The fact that every method is no longer than four lines makes each methods purpose blindingly obvious, especially if you use meaningful names.

As I said earlier, I made the Scraper an instantiable object that has the url prefix and suffix as constructor parameters, as well as the desired timeout as a Duration object.

I've made all the error handling a responsibility of the caller. Ideally, you might want to define your own exception and wrap the IOExceptions in them, for example you could have a ScraperException that is thrown when the Scraper encounters an error.

Note also that all the result handling is moved to the caller also. The caller receives a result object in form of a List<WeeklyResult> and can do with it whatever they please. If you want to handle results as soon as they are parsed but want to stay flexible, you migth want to consider using Callbacks.


Questions

  1. Collection vs Elements for the parameter of extractWeeks: does this again relate to “use the broadest type of collection possible”?

To be honest, it wasn't a conscious choice since I let the IDE perform Extract Method, but generally, yes. Elements is a type of Collection<Element>, but none of it's features are needed in extractWeeks so you might as well use Collection<Element> to make the method more broadly applicable (even though you might not need it).

  1. static member functions vs non-static: I’m definitely going to look into this more myself but I couldn’t help getting confused over why certain functions (like extractWeeks) were static, but others (like weeklyResultUrl) are not static. In both cases, the object doesn’t directly call it, so wouldn’t it make sense to declare all such functions as static?

Methods can't be static if they use members of their class. Since weeklyResultUrl uses the fields urlPrefix and urlSuffix, it cannot be static. I could declare all methods none-static, but declaring a method static has a few advantages to the reader and to the programmer:

When calling a static method, you can be sure that it does not modify the instance state. Likewise, when inside a static method, you are not able to modify the instance state. Both of these decrease the mental load when reading and writing code.

Also, since a static clearly doesn't require an instance to function, you are able to call a public static method without an instance from outside the class.

  1. The noEmptyElseThrow strictly isn’t an IOException, is it? Can I throw other exceptions instead (IllegalArgumentExcpetion or NullPointerException, and I’m not sure which is the more suited of the two?), and if so would the caller have to rethrow them?

Yes, technically you're right. I don't think either of the Exceptions you suggested are quite what you'd want. I would only ever throw IllegalArgumentExcpetion if you pass an invalid argument to a method. I would assume that you could extract the numbers from &rankRange=0-100 and add them as an argument to the method. Then IAE might be more applicable.

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

But NPE definitely doesn't fit. Only ever throw an NPE if something is null when it shouldn't be.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I realize this might be a little advanced, so if you have any further questions, please let me know and I will try to answer them. \$\endgroup\$ – Marv Jun 18 at 2:31
  • \$\begingroup\$ First of all, thanks a lot for the review, there's a LOT of valuable insights in your comment! I'm still going through a bit of them, so I'll probably ask more questions over the course of the coming day but I've got a few questions for the moment, see my post below. \$\endgroup\$ – cloudy_eclispse Jun 18 at 23:24
  • \$\begingroup\$ You're welcome! See my edited answer for answers to your questions. Additionally, I would recommend checking out Robert C. Martins excellent Clean Code: A Handbook of Agile Software Craftsmanship. \$\endgroup\$ – Marv Jun 18 at 23:43
  • \$\begingroup\$ I moved my comments to my questions so everyone referring to this will have an easier time. I also added a bunch of questions to clarify a bit about cleaner exception handling - it'd be great if you could get to them whenever you have time! I think for the most part I have a much better idea of how to strengthen my code, so thanks again and I hope I'm not asking too much of you by asking too many questions :D \$\endgroup\$ – cloudy_eclispse Jun 19 at 19:32

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.