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