1
\$\begingroup\$

I have made a skeleton part of the crawler and would like to ask you for a review. I'm not sure especially about the way how I divide app into classes.

What app does? It scraps through all the content written by users in the topic of the forum. The topic is chosen by client. In the end it returns Map with count for each word in the content of the topic.

Structure of classes:

Site - abstract class, navigates through Pages of the Topic and returns them.

Page - abstract class, navigates through Posts of the Page and returns them.

Post - abstract class, gather basic information like date of the Post, user of the Post, and the content of the Post.

Site4programmers - extends Site, works for the forum "4programmers".

Page4programmers - extends Page, works for the forum "4programmers".

Post4programmers - extends Post, works for the forum "4programmers".

NavigatePanel4programmers - holds elements for navigation through pages.

WordsCounter - class for counting the statics of the words

Splitter - interface with methods wchich supports WordsCounter

SplitterBlankSpaceWithoutPunctuationMarks - implementation of the interface Splitter.

Main questions

  • Do the classes divide the app in good OOP style?
  • Where should I put the variables? Into abstract class or into child class?

The code:

/**
 * 
 * The topic on the forum.
 * For navigating through pages of the topic and getting them.
 *
 */
public abstract class Site {


    /**
     * 
     * @return: maximum page number to don't overdose pagenumbers.
     */
    public abstract int getMaxPageNumber();

    /**
     * Download the chosen page by pageNumber.
     */
    public abstract Page getPage(int pageNumber);

}

import java.io.IOException;

import org.jsoup.Jsoup;
import org.jsoup.nodes.Document;

/*
 * to consider: singleton, flyweight

 * to consider: what if the view of the post will be changed from 10 to 50?

 */
public class Site4programmers extends Site {

    // to consider: put these variables into abstract class?
    private Document startPage;
    private Integer maxPageNumber;
    private String urlBase, pagePattern;
    private NavigatePanel4programmers navigatePanel;
    private final String BROWSER = "Mozilla/5.0";

    public Site4programmers(String url, String pagePattern) {
        urlBase = url;
        this.pagePattern = pagePattern;
    }

    public Site4programmers(String url) {
        urlBase = url;
        this.pagePattern = "?page=";
    }

    /**
     * Download the startPage from URL to set a navigatePanel.
     */
    private void initStartPage() {
        try {
            startPage = Jsoup.connect(urlBase).userAgent(BROWSER).get();
        } catch (IOException e) {
            System.err.println("Something went wrong with getting the url. Start page has not been initialized.");
            e.printStackTrace();
        }
    }

    private void initNavigatePanel() {
        if (startPage == null) {
            initStartPage();
        }
        navigatePanel = new NavigatePanel4programmers(startPage);
    }

    public void initMaxPageNumber() {
        if (navigatePanel == null) {
            initNavigatePanel();
        }
        maxPageNumber = navigatePanel.getMaxPageNumber();
    }

    public int getMaxPageNumber() {
        if (maxPageNumber == null) {
            initMaxPageNumber();
        }
        return maxPageNumber;
    }

    /* to consider: is it factory pattern? */
    public Page getPage(int pageNumber) {
        String newUrl = urlBase + pagePattern + String.valueOf(pageNumber);
        Document doc = null;
        try {
            doc = Jsoup.connect(newUrl).userAgent("Mozilla/5.0").get();
            //System.out.println(doc);
        } catch (IOException e) {
            System.out.println("something went wrong with downloading the page: " + newUrl);
            if(maxPageNumber == null){
                initMaxPageNumber();
            }
            if(maxPageNumber < pageNumber){
                System.out.println("The pageNumber you have chosen is to high. The highest is " + maxPageNumber);
            }else {
                System.out.println("Check the net connection or firewall.");
            }
            e.printStackTrace();
        }
        return new Page4programmers(doc, pageNumber);
    }

    public void setUrlBase(String html) {
        this.urlBase = html;
        // set to null cuz it must be initialize after changing the urlBase.
        startPage = null;
        navigatePanel = null;
        maxPageNumber = null;
    }

    public String getUrlBase() {
        return urlBase;
    }

    public void setPagePattern(String pattern) {
        pagePattern = pattern;
        // set to null cuz it must be initialize after changing the urlBase.
        maxPageNumber = null;
    }

    public String getPagePattern() {
        return pagePattern;
    }

}

import org.jsoup.nodes.Document;
/**
 * too consider: iterator pattern.
 * For navigating through posts of page and getting them.
 *
 */
public abstract class Page {

    int id;
    Document doc;

    public abstract Post getPost(int idx);

    public abstract int numberOfPosts();

}

import org.jsoup.nodes.Document;
import org.jsoup.select.Elements;

public class Page4programmers extends Page {

    private Elements allPosts;

    public Page4programmers(Document doc, int pageNumber) {
        id = pageNumber;
        this.doc = doc;
    }

    private void initAllPosts() {
        // cuz the first post occurs always at every page and the last is a
        // rubish.
        if(id == 1){
            allPosts = doc.select("div.post[id!=last-post]");
        }else {
            allPosts = doc.select("div.post[id!=first-post][id!=last-post]");
        }       
    }

    public Elements getAllPosts() {
        if (allPosts == null) {
            initAllPosts();
        }
        return allPosts;
    }

    /*
     * The first post at 4programmers is at every page, that's why here
     * is a possibility to get it from every page.
     */
    public Post getFirstPost() {
        return new Post4programmers(doc.select("div.post[id=first-post]").first());
    }

    @Override
    public Post getPost(int idx) {
        if (allPosts == null) {
            initAllPosts();
        }
        return new Post4programmers(allPosts.get(idx));
    }

    @Override
    public int numberOfPosts() {
        if (allPosts == null) {
            initAllPosts();
        }
        return allPosts.size();
    }

    public Document getDocument() {
        return doc;
    }

}

import java.util.Date;

import org.jsoup.nodes.Element;

public abstract class Post {

    Element elem; // the core of the Post class.
    long id;
    Date date;
    String authorNick;
    MessageForSallariesResearch message;

    public Post(Element elem){
        this.elem = elem;
    }

    /**   
     * to consider: generic? 
     * @return: id of the post as a number.
     */
    public abstract long getId();

    /**
     * 
     * @return: when the post was posted.
     */
    public abstract Date getDate();

    /* to consider: creating separate class for the author */
    /**
     * 
     * @return: nickname on the forum.
     */
    public abstract String getAuthorNick();

    /**
     * 
     * @return: what author has written.
     */     
    public abstract String getContent();

    public abstract Element getElement();
}

import java.util.Date;

import org.jsoup.nodes.Element;

public class Post4programmers extends Post {

    public Post4programmers(Element elem) {
        super(elem);
    }

    @Override
    public long getId() {
        // TODO Auto-generated method stub
        return 0;
    }

    @Override
    public Date getDate() {
        // TODO Auto-generated method stub
        return null;
    }

    @Override
    public String getAuthorNick() {
        // TODO Auto-generated method stub
        return null;
    }


    @Override
    public String getContent(){
        return elem.select("div.post-content").text();
    }

    @Override
    public Element getElement(){
        return elem;
    }

}

import java.util.Map;
import java.util.TreeMap;

public class WordsCounter {

    Map<String, Integer> wordsMap = new TreeMap<>();

    public void addToCount(String text, Splitter spl) {
        String[] words = spl.splitWords(text);
        calcWords(words);
    }

    public void addToCount(Post post, Splitter spl) {
        addToCount(post.getContent(), spl);
    }

    public void calcWords(String[] words) {
    int wordCount;
    for (String word : words) {
        wordCount = wordsMap.containsKey(word) ? wordsMap.get(word) : 0;         
        wordsMap.put(word, wordCount + 1);
    }
}

    public Map<String, Integer> getWordsCounted() {
        return wordsMap;
    }

}

public interface Splitter {

    public String[] splitWords(String text);
}

public class SplitterBlankSpaceWithoutPunctuationMarks implements Splitter{

    @Override
    public String[] splitWords(String text) {
        text = text.replaceAll("[^\\w]", "");
        return text.split(" ");
    }

}

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;

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

public class AppMain {

    public static void main(String[] args) throws IOException {

        WordsCounter wc = new WordsCounter();
        String url = "https://4programmers.net/Forum/Java/151279-Ksi%C4%85%C5%BCki";
        Site site = new Site4programmers(url);

        for (int j = 1; j <= site.getMaxPageNumber(); j++) { // for every page
            Page page = site.getPage(j);
            for (int i = 0; i < page.numberOfPosts(); i++) { // for every post in every page
                Post post = page.getPost(i);
                wc.addToCount(post.getContent(), new SplitterBlankSpaceWithoutPunctuationMarks());
            }
        }
        System.out.println(wc.getWordsCounted());

    }

}
\$\endgroup\$
1
\$\begingroup\$

The Site class has only behavior (abstract methods) and no data (fields). It would be good to make this an interface instead.

The Site class provides access to pages through two methods:

  • getMaxPageNumber to get the number of pages
  • getPage(int) to get a page by index

If the only use case is iterating over pages, and there is no need to access pages by index, then it will make more sense to provide one method that returns an Iterable<Page>.

The Site4programmers class initializes its elements using lazy-loading:

  • startPage will be initialized when initStartPage is called
  • initStartPage is called from initNavigatePanel
  • initNavigatePanel is called from initMaxPageNumber
  • initMaxPageNumber is called from getMaxPageNumber

Is this lazy-loading necessary? If you plan to have many Site4programmers instances without loading the documents, then lazy-loading could be useful, but I'm not sure that's your case here.

You forgot the @Override annotation from the getPage and getMaxPageNumber methods.

You have a bug in this splitWords implementation:

text = text.replaceAll("[^\\w]", "");
return text.split(" ");

If you replace all non-word characters with empty string, then there won't be any spaces left between words to split.

You can fix and simplify a bit like this:

return text.replaceAll("\\W", " ").split(" +");

In Post4programmers the methods where you didn't remove the TODO comments look like dummy implementations. You could have put these implementations in the parent class Post.

The Page class declares id and doc fields, but doesn't assign values to them or use them in any way. Every program element should have a good reason to be where it is, and I don't see any reason for these fields here. They should not be here.

\$\endgroup\$
  • \$\begingroup\$ What do ya think: is it better to move variables of Page4programmers to abstract Page? Or do Page as interface as ya said? \$\endgroup\$ – wBacz Sep 6 '17 at 21:40
  • \$\begingroup\$ @wBacz make Page an interface \$\endgroup\$ – janos Sep 7 '17 at 7:45

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.