2
\$\begingroup\$

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", ""));
    }
}
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Upon revisiting this, a few methods could be made static, specifically replaceIllegalFileNameCharacters, createBaseName, createName, and createExtension. \$\endgroup\$ – ndm13 Jul 11 '17 at 16:08
  • 1
    \$\begingroup\$ I took your advice about fields and state in account. The private methods with one liners are intended to make the code easier to read but I might have went overboard so I cut down on it. GenericArticleScraper is common code which could be in a super class but I decided to favour composition over inheritance. Your assumptions where spot on, your tips made the class cleaner imo. I'm not too excited about making the methods from your comment static . I don't see the need or value, maybe you can clarify. Tnx either way! \$\endgroup\$ – progonkpa Jul 13 '17 at 21:05
  • \$\begingroup\$ There's no need to. Typically methods that don't use any instance fields should be static, but it's not a hard-and-fast rule. \$\endgroup\$ – ndm13 Jul 13 '17 at 21:09

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.