CR Stack Exchange crawler

Question

I am writing a program which automatically crawls codes from this site!

Would you please review my code?

The required .jars: jsoup, org.apache.commons.io.

Main.java:

public class Main { 
    public static void main(String[] args) throws Exception { 
        final String site1 = "http://codereview.stackexchange.com/questions/"; 
        String site2[] = {"69", "109", "131"}; 
        String site3[] = {"is-this-implementation-of-shamos-hoey-algorithm-ok",
                "are-there-any-ways-to-improve-my-http-request-path-parser",
                "law-of-demeter-and-data-models"}; 
        HtmlGetter htmlGetter = new HtmlGetter();

        for(int i = 0 ; i < site2.length ; ++i) {
            htmlGetter.setUrl(site1, site2[i], site3[i]);
            htmlGetter.download(); 
            htmlGetter.parse(i+1);
        } 
    } 
} 

HtmlGetter.java:

import java.io.*;
import java.net.URL; 
import java.util.Scanner; 

import org.apache.commons.lang3.StringEscapeUtils;  
import org.apache.commons.io.IOUtils; 
import org.jsoup.Jsoup;
import org.jsoup.nodes.Document;
import org.jsoup.nodes.Element;
import org.jsoup.select.Elements;

public class HtmlGetter { 
    private URL url;
    private String fileName;

    public void setUrl(String site1, String site2, String site3) throws Exception {
        this.url = new URL(site1 + site2 + "/" + site3);
        this.fileName = "[" + site2 + "]" + site3 + ".html";
    }

    public void download() throws Exception { 
        final InputStream in = url.openStream(); 
        final OutputStream out = new FileOutputStream(fileName); 
        IOUtils.copy(in, out); 
        in.close(); 
        out.close();
        divideFile();
    }

    private void divideFile() throws Exception {
        Scanner scanner = new Scanner(new File(fileName)); 
        String oneLine; 
        final BufferedWriter out1 = new BufferedWriter(new FileWriter("question.html"));
        final BufferedWriter out2 = new BufferedWriter(new FileWriter("answer.html"));

        while(scanner.hasNextLine()) { 
            oneLine = scanner.nextLine(); 
            if(oneLine.matches(".*<div class=\"post-text\" itemprop=\"description\">.*")) { 
                while(scanner.hasNextLine()&&(!oneLine.matches(".*<div class=\"post-menu\">.*"))) { 
                    out1.write(oneLine);
                    out1.newLine();
                    oneLine = scanner.nextLine(); 
                } 
            }
            if(oneLine.matches("<td class=\"answercell\">")) {
                while(scanner.hasNextLine()&&(!oneLine.matches(".*<div class=\"post-menu\">.*"))) {
                    out2.write(oneLine);
                    out2.newLine();
                    oneLine = scanner.nextLine();
                }
                break;
            }
        }
        out1.close();
        out2.close();
     }

    public void parse(int articleNumber) throws Exception {
        parse2("question.html", articleNumber);
        parse2("answer.html", articleNumber);
    }

    public void parse2(String strFileName, int articleNumber) throws Exception {
        File input = new File(strFileName);
        Document doc = Jsoup.parse(input, "UTF-8");
        Elements codes = doc.select("code");
        BufferedWriter out;
        int subNumber = 0;
        String qa = "x";

        for(Element code: codes) 
        {
            if(strFileName.equals("question.html")) { qa = "q"; }
            else if(strFileName.equals("answer.html")) { qa = "a"; }
            else { System.out.println("Error!"); }
            out = new BufferedWriter(new FileWriter(qa + articleNumber + "-" + (++subNumber) + ".txt"));
            out.write(code.text().replaceAll("[\r\n]", "\r\n"));
            out.close();
        } 
    }
}

Answer 1

At least at first sight I couldn't tell you were new to Java, so that's pretty good. :) Not many issues there, but a some:

First, your divideFile method has some duplicate code. You should refactor that into a new method. The method also never closes the BufferedWriters if some exception occurs, which is why we have finally block in the try-catch-structure. This I'll leave for you as an exercise, though.

Next, in your parse2 method, move that if-block that's deciding the value of the variable qa outside of that for each -loop. There's no need to do costly string comparisons on each iteration. Then also notice that the strFileName could be null so your if-block could crash due to a NullPointerException when you do strFileName.equals("question.html"), so you should do a null-check prior to that. Then change the visibility of the method to private, if you only intend it to be called from parse.

Then, you've got those same question.html and answer.html strings in the code many times. That's always bad, because when you one day decide to change the file name to something else, you need to replace many occurrences. Rather, you should turn them into constants: private static final String QUESTION_FILE = "question.html";. Then just use the QUESTION_FILE constant in place of the string.

Furthermore, you could turn those constants into an enum. An enum is like a collection of allowed values for something. You can define an enum that only holds the values question.html and answer.html. It's more handy when you have a little more options, but you get the point.

Finally, I find that int variable = 0; ++variable; syntax rather confusing and would rather use int variable = 1; variable++;, but I guess that's mostly just a matter of personal preference. Although you should be careful when changing the value of a variable in the middle of a sentence in any way, because eventually you might want to add some code into that block and to use that variable again. Then it's confusing if the variable number does not stay the same for the entire iteration.

So all in all I might write something like this in the HtmlGetter class:

public class HtmlGetter {

private enum FILE_TYPE {
    question, answer;
    public String getFileName() {
        switch (this) {
            case question: return "question.html";
            case answer:   return "answer.html";
            default:
                System.out.println("Error!");
                return "error.html";
        }
    }
};
private URL url;
private String fileName;
private static final String POST_MENU = ".*<div class=\"post-menu\">.*";

public void setUrl(String site1, String site2, String site3) throws Exception {
    this.url = new URL(site1 + site2 + "/" + site3);
    this.fileName = "[" + site2 + "]" + site3 + ".html";
}

public void download() throws Exception {
    final InputStream in = url.openStream();
    final OutputStream out = new FileOutputStream(fileName);
    IOUtils.copy(in, out);
    in.close();
    out.close();
    divideFile();
}

private void divideFile() throws Exception {
    Scanner scanner = new Scanner(new File(fileName));
    String oneLine;
    final BufferedWriter out1 = new BufferedWriter(new FileWriter(FILE_TYPE.question.getFileName()));
    final BufferedWriter out2 = new BufferedWriter(new FileWriter(FILE_TYPE.answer.getFileName()));

    while (scanner.hasNextLine()) {
        oneLine = scanner.nextLine();
        if (oneLine.matches(".*<div class=\"post-text\" itemprop=\"description\">.*")) {
            while (scanner.hasNextLine() && (!oneLine.matches(POST_MENU))) {
                oneLine = writeAndReadLine(scanner, out1, oneLine);
            }
        }
        if (oneLine.matches("<td class=\"answercell\">")) {
            while (scanner.hasNextLine() && (!oneLine.matches(POST_MENU))) {
                oneLine = writeAndReadLine(scanner, out2, oneLine);
            }
            break;
        }
    }
    out1.close();
    out2.close();
}

private String writeAndReadLine(Scanner scanner, BufferedWriter out, String line) throws Exception {
    out.write(line);
    out.newLine();
    return scanner.nextLine();
}

public void parse(int articleNumber) throws Exception {
    parse2(FILE_TYPE.question, articleNumber);
    parse2(FILE_TYPE.answer, articleNumber);
}

private void parse2(FILE_TYPE file, int articleNumber) throws Exception {
    File input = new File(file.getFileName()); // This throws a NullPointerException
    Document doc = Jsoup.parse(input, "UTF-8");
    Elements codes = doc.select("code");
    BufferedWriter out;
    int subNumber = 1;
    for (Element code : codes) {
        out = new BufferedWriter(new FileWriter("codereview-questions/"
                + file.getFileName().charAt(0) + articleNumber + "-" + subNumber + ".txt"));
        out.write(code.text().replaceAll("[\r\n]", "\r\n"));
        out.close();
        subNumber++;
    }
}
}

You Main class seems to be for testing purposes only since it doesn't parse the questions from the main page of the site, so I won't comment on it except by saying that it seems to do its job. And even though this message may look longish, I found your original code quite good. :)

Answer 2

While ZeroOne's answer has the good idea to use an enum, the enum itself is not implemented very well. Java enums are a powerful system, and using a switch statement inside the getFileName() method is very unnecessary. Furthermore, the naming conventions are a bit off. Here's a better implementation of the same enum:

private enum FileType {
    QUESTION("question.html"),
    ANSWER("answer.html");

    private final String fileName;

    FileType(String fileName) {
        this.fileName = fileName;
    }

    public String getFileName() {
        return fileName;
    }
}

This uses the unique capabilities of Java enums to store individual parameters. This also changes the names of the enum and its values. Java enum names should be upper camel-case (same as class names), while enum values should be all caps.