2
\$\begingroup\$

I have simple class that I want to ask if is there any possible to improve it? I mean, for me it looks poor. Is there any way to use here try-with-resources, stream or optional?

package bookstore.scraper;

import lombok.extern.slf4j.Slf4j;
import org.jsoup.Jsoup;
import org.jsoup.nodes.Document;

import java.io.IOException;

@Slf4j
public class JSoupConnector {

    public static Document prepareConnectionToURL(String URL) {
        Document document = null;
        try {
            document = Jsoup.connect(URL).get();
        } catch (IOException e) {
            log.warn("Cannot connect to URL!");
        }
        return document;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Do not fall into solution looking for a problem. \$\endgroup\$ – greybeard Jul 24 '19 at 2:47
1
\$\begingroup\$

This error handling looks pretty useless to me:

  • The log message doesn't record which URL was inaccessible.
  • If an error occurs, then the code will continue to execute. That means that whoever calls this function will have to handle the possibility of a null result. If that isn't done, then it will cause a NullPointerException.

A better idea would be to simply propagate the exception, so that the caller will stop trying to process a Document that isn't there.

Furthermore, what does it mean to "prepare a connection"? It looks like this code goes way beyond preparing a connection — it actually makes the connection and fetches the content as a Document!

So, the class should look more like this:

public class JSoupConnector {
    public static Document connect(String url) throws IOException {
        try {
            return Jsoup.connect(url).get();
        } catch (IOException e) {
            log.warn("Cannot connect to URL " + url);
            throw e;
        }
    }
}

At that point, this wrapper is hardly doing anything at all, and you might consider eliminating this helper functional altogether.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I have a lot of methods that use it I wanted to eliminate error handling with IOException in every method (I read that throws ... in every method is bad practice). Is it good idea to change throw e to System.exit(1) ? \$\endgroup\$ – mara122 Jul 23 '19 at 20:56
  • \$\begingroup\$ ahh... I see that you deleted Document document=null, so I need to return something in catch statement. As I said is there any way to handle the exception here so that I do not have to do it in the future (methods that use connect method)? \$\endgroup\$ – mara122 Jul 23 '19 at 20:59
  • \$\begingroup\$ The whole point of the IOException is to force the caller to deal with the abnormal condition, and also to make it convenient to handle the abnormal condition. Use the exception mechanism to your advantage. Just keep propagating it by declaring a throws clause, until you reach a caller that can handle it in a meaningful way. (If you showed more of your code, I could illustrate it better.) \$\endgroup\$ – 200_success Jul 23 '19 at 21:17
  • \$\begingroup\$ As I said, I'm using it in a lot of Services, so it is hard to handle in a lot of places. Here is a gist: gist.github.com/must1/0124f5b294160d8ad5824fe9f52903fe I just added 2 to illustrate how I am using it, but I have 2 more. Thanks a lot. \$\endgroup\$ – mara122 Jul 23 '19 at 21:29
  • \$\begingroup\$ If you will have free time, please illustrate it, stil waiting :D .Thanks in advance. \$\endgroup\$ – mara122 Jul 25 '19 at 7:42
0
\$\begingroup\$

You can fail safe and explicitly handle connection issues by changing the intention of the method:

package bookstore.scraper;

import lombok.extern.slf4j.Slf4j;
import org.jsoup.Jsoup;
import org.jsoup.nodes.Document;

import java.io.IOException;

@Slf4j
public class JSoupConnector {

    public static Optional<Document> attemptConnectionTo(String url) {
        try {
           return Optional.of(Jsoup.connect(url).get());
        } catch (IOException e) {
            log.warn("Failed to connect to URL {}: {}", url, e.getMessage());
        }
        return Optional.empty();
    }
}

This way you emphasize that clients of your utility class should explicitly handle the absence of a document as part of their logic. If your clients would be in an invalid state or there responsibility does not stretch to handling missing documents, then throw an application exception instead to fail fast and delegate to an exception handler. My conclusion is don't return null but clarify your intention as this class is tightly integrated with your app's domain.

| improve this answer | |
\$\endgroup\$

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.