Class for scraping images with JSoup

Question

I refactored this class as far as I'm capable of at the time but I wonder if it can't be better. One thing that I'm not sure of, is that I take parameters from a method, which is not a constructor, and put them in fields. I wonder if it's bad practice.

Also, I decided to pull parameters away to fields to reduce the passing around of arguments. It made the method calls cleaner but initializing a field in a for loop looks awkward.

I suspect further improvements are possible so if you can see any, feel free to chop away at the code.

Some context: JsoupParser is just a wrapper around Jsoup with some retry functionality and to make it testable. SelectorContainer is a container that holds Selector objects that are used to define what has to be scraped. Response is a returned value from Jsoup when you connect to a URL, in this case the URL of an img src attribute.

@Component
public class ImageScraper {
    private JsoupParser jsoupParser;
    private GenericArticleScraper genericArticleScraper;
    private SelectorContainer selector;
    private Document document;
    private Elements elements;
    private Element element;

    @Autowired
    public ImageScraper(JsoupParser jsoupParser, GenericArticleScraper genericArticleScraper) {
        this.jsoupParser = jsoupParser;
        this.genericArticleScraper = genericArticleScraper;
    }

    public List<Image> scrape(Document document, SelectorContainer selector) {
        intializeFields(document, selector, scrapeImageElements());
        if (hasNoImageElements()) {
            return new ArrayList<>();
        }
        return scrapeImages(elements);
    }

    private void intializeFields(Document document, SelectorContainer selector, Elements elements) {
        this.document = document;
        this.selector = selector;
        this.elements = elements;
    }

    private Elements scrapeImageElements() {
        return genericArticleScraper.scrapeElementsUsingSelector(document, selector.forImage());
    }

    private boolean hasNoImageElements() {
        return elements == null || elements.isEmpty();
    }

    private List<Image> scrapeImages(Elements elements) {
        int orderOfAppearance = 0;
        List<Image> images = new ArrayList<>();
        for (Element element : elements) {
            try {
                ++orderOfAppearance;
                this.element = element;
                images.add(
                        createImage(
                                getResponse(scrapeUrl(this.element)),
                                orderOfAppearance,
                                createName(createBaseName(), orderOfAppearance)));
            } catch (IOException e) {
                --orderOfAppearance;
                e.printStackTrace();
            }
        }
        return images;
    }

    private Image createImage(Response response, int orderOfAppearance, String name) throws IOException {
        return new Image(
                scrapeUrl(element),
                name,
                orderOfAppearance,
                scrapeImageCaption(element),
                scrapeImageCredit(element),
                createExtension(response),
                responseBodyAsBytes(response));
    }

    private String scrapeUrl(Element element) {
        return genericArticleScraper.scrapeTextUsingSelector(element, selector.forImageUrl());
    }

    private String scrapeImageCaption(Element element) {
        return genericArticleScraper.scrapeTextUsingSelector(element, selector.forImageCaption());
    }

    private String scrapeImageCredit(Element element) {
        return genericArticleScraper.scrapeTextUsingSelector(element, selector.forImageCredit());
    }

    private Extension createExtension(Response response) {
        return Extension.from(response.contentType().replace("image/", "").replace(";charset=UTF-8", ""));
    }

    private byte[] responseBodyAsBytes(Response response) {
        return response.bodyAsBytes();
    }

    private Response getResponse(String url) throws IOException {
        return jsoupParser.connect(url);
    }

    private String createName(String name, int count) {
        return name + "_" + count;
    }

    private String createBaseName() {
        return replaceIllegalFileNameCharacters(scrapeHeadline());
    }

    private String replaceIllegalFileNameCharacters(String text) {
        return text.replaceAll("[^a-zA-Z0-9.-]", "_");
    }

    private String scrapeHeadline() {
        return genericArticleScraper.scrapeTextUsingSelector(document, selector.forHeadline());
    }
}

Refactored after ndm13's answer:

package org.observer.media.scrape.article;

import org.jsoup.Connection;
import org.jsoup.Connection.Response;
import org.jsoup.nodes.Document;
import org.jsoup.nodes.Element;
import org.jsoup.select.Elements;
import org.observer.media.model.Image;
import org.observer.media.model.Image.Extension;
import org.observer.media.parse.JsoupParser;
import org.observer.media.scrape.SelectorContainer;
import org.observer.media.scrape.component.GenericArticleScraper;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

@Component
public class ImageScraper {
    private JsoupParser jsoupParser;
    private GenericArticleScraper scraper;

    @Autowired
    public ImageScraper(JsoupParser jsoupParser, GenericArticleScraper scraper) {
        this.jsoupParser = jsoupParser;
        this.scraper = scraper;
    }

    public List<Image> scrape(Document document, SelectorContainer selector) {
        List<Image> images = new ArrayList<>();
        Elements elements = scraper.scrapeElements(document, selector.forImage());
        if (elements.isEmpty()) {
            return images;
        }
        int index = 0;
        for (Element element : elements) {
            try {
                images.add(createImage(++index, selector, element, createBaseName(document, selector)));
            } catch (IOException e) {
                --index;
                e.printStackTrace();
            }
        }
        return images;
    }

    private Image createImage(int index, SelectorContainer selector, Element element, String baseName) throws IOException {
        Connection.Response response = getResponse(scraper.scrape(element, selector.forImageUrl()));
        return new Image(
                scraper.scrape(element, selector.forImageUrl()),
                createName(baseName, index),
                index,
                scraper.scrape(element, selector.forImageCaption()),
                scraper.scrape(element, selector.forImageCredit()),
                createExtension(response),
                response.bodyAsBytes());
    }

    private Response getResponse(String url) throws IOException {
        return jsoupParser.connect(url);
    }

    private String createName(String name, int count) {
        return name + "_" + count;
    }

    private String createBaseName(Document document, SelectorContainer selector) {
        return replaceIllegalFileNameCharacters(scraper.scrape(document, selector.forHeadline()));
    }

    private String replaceIllegalFileNameCharacters(String text) {
        return text.replaceAll("[^a-zA-Z0-9.-]", "_");
    }

    private Extension createExtension(Response response) {
        return Extension.from(response.contentType().replace("image/", "").replace(";charset=UTF-8", ""));
    }
}

Answer 1

A few tips to keep in mind when writing code:

• Keep as many methods as you need, but only as many. Methods are a means to cut down on code reuse, so if you're only using them once, it's a bit of a waste. This class has one public method and fifteen private methods, each of which is only used once and many of which are aliases for one-liners.
• Fields are for information on the state of the object, not the computation. Things that are inherent to this instance of the object (e.g. I use this ImageScraper to scrape images acquired by this JsoupParser using selectors in this SelectorContainer), then it shouldn't be a field. Additionally, manipulating fields is typically done in a transparent way (think get/set/update methods).
• If a method needs something, pass it as a parameter, provided it's not a field.

That being said, there's some cool stuff going on here. Abstracting away the scraping behavior is a nice touch, as is abstracting away the Selectors used. I refactored the code to remove redundant elements and remove the fields that shouldn't be there:

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

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

public class ImageScraper {
    private GenericArticleScraper scraper;

    public ImageScraper(GenericArticleScraper scraper) {
        this.scraper = scraper;
    }

    public List<Image> scrape(Document document, SelectorContainer selector) {
        List<Image> images = new ArrayList<>();
        Elements elements = scraper.scrapeElementsUsingSelector(document, selector.forImage());
        if (elements.isEmpty()) {
            return images;
        }
        int index = 0;
        for (Element element : elements) {
            try {
                images.add(createImage(++index, selector, element));
            } catch (IOException e) {
                --index;
                e.printStackTrace();
            }
        }
        return images;
    }

    private Image createImage(int index, SelectorContainer selector, Element element) throws IOException {
        Connection.Response response = getResponse(scraper.scrapeTextUsingSelector(element, selector.forImageUrl()));
        return new Image(
                scraper.scrapeTextUsingSelector(element, selector.forImageUrl()),
                createName(createBaseName(element, selector), index),
                index,
                scraper.scrapeTextUsingSelector(element, selector.forImageCaption()),
                scraper.scrapeTextUsingSelector(element, selector.forImageCredit()),
                createExtension(response),
                response.bodyAsBytes());
    }

    private Extension createExtension(Connection.Response response) {
        return Extension.from(response.contentType().replace("image/", "").replace(";charset=UTF-8", ""));
    }

    private Connection.Response getResponse(String url) throws IOException{
        return Jsoup.connect(url).response();
    }

    private String createName(String name, int count) {
        return name + "_" + count;
    }

    private String createBaseName(Element element, SelectorContainer selector) {
        return replaceIllegalFileNameCharacters(scraper.scrapeTextUsingSelector(element, selector.forHeadline()));
    }

    private String replaceIllegalFileNameCharacters(String text) {
        return text.replaceAll("[^a-zA-Z0-9.-]", "_");
    }

    // Extrapolated
    private static class Image{
        public Image(String url, String name, int index, String caption, String credit,
                Extension extension, byte[] bytes){}
    }

    private interface GenericArticleScraper{
        String scrapeTextUsingSelector(Element element, Selector selector);
        Elements scrapeElementsUsingSelector(Element element, Selector selector);
    }
    private interface SelectorContainer{
        Selector forHeadline();
        Selector forImageCredit();
        Selector forImageCaption();
        Selector forImageUrl();
        Selector forImage();
    }

    private static class Extension{
        public static Extension from(String replace){
            return null;
        }
    }
}

I hope my assumptions on class type were correct, since you left out the import statements. I also mocked together some classes/interfaces at the end to get things to compile. The only thing I didn't mock was the JsoupParser, which I just unboxed.

Some suggestions for going beyond this would be to replace GenericArticleScraper with a supertype (assuming one exists), so you could scrape articles that don't fit that mold. Ditto for JsoupParser; what if someone wanted to use a different parsing framework?

Maybe you should also consider shorter method names. With refactoring, methods like GenericArticleScraper#getTextUsingSelector() eat up a fair amount of real estate. You can imply the Selector part (since it's an argument) and shorten it to getText().

I also question the necessity of an Extension class when a String would do. Watch out for image/svg+xml and image/x-icon when running that replacement function.