2
\$\begingroup\$

I've been implementing a little application that will read a text file that holds URL addresses (one address per line).

Example:

http://www.yahoo.com
http://www.cnn.com
http://news.google.com

Based on that file, I am supposed to read the parsed content of the main page for each URL and search for any text that includes a hashtag.

Once I found all the words that matches the hashtag pattern, I am supposed to write an output text file for each URL that I connected to with the results of the lookup.

External libraries being used: Apache Commons Validator (for URL stuff) and JSoup for HTML handling.

So far, this is what I've done:

An enum holding the supported patterns, I am using by now the hashtag:

public enum SearchPatternTypeEnum {

HASHTAG_PATTERN("#(\\S+)"),
TWITTER_ACCOUNT_PATTERN("@(\\S+)"),
PROPER_NAME_PATTERN("^[\\p{L} .'-]+$");

private String regexPattern;

private SearchPatternTypeEnum(String regexPattern) {
    this.regexPattern = regexPattern;
}

public String getRegexPattern() {
    return regexPattern;
}

public void setRegexPattern(String regexPattern) {
    this.regexPattern = regexPattern;
}

}

A class (singleton) to handle read/write operations on text files

public class TextFileManager {

private static class Loader {
    static TextFileManager INSTANCE = new TextFileManager();
}

private TextFileManager() {

}

public static TextFileManager getInstance() {
    return Loader.INSTANCE;
}

public List<String> readLinesAsList(String fullFileName) throws IOException {
    List<String> dataLines = new ArrayList<>();

    try (Stream<String> stream = Files.lines(Paths.get(fullFileName))) {
        dataLines = stream.collect(Collectors.toList());
    } catch (IOException e) {
        throw e;
    }

    return dataLines;
}

public void writeContentToFile(String folderName, String fileName, String content) throws IOException {
    File targetFile = new File(folderName, fileName);
    try (BufferedWriter bufferedWriter = new BufferedWriter(new FileWriter(targetFile))) {
        bufferedWriter.write(content);
    } catch (IOException e) {
        throw e;
    }
}

}

Since the application is supposed to support more than one thread processing, I am creating a "task" that will connect to the URL, then get the HTML using JSoup, then search for the hashtag pattern (regex) and then save all the matches in a String variable that is a property in an object that holds other information of the process. I am saving as String because that content must be output to a file.

public class WebPageAnalyzerTask implements Callable<WebPageAnalysisTaskResult>{
private static final int CONNECTION_TIMEOUT_IN_MILLIS = 10000;
private String targetUrl;
private String searchPattern;
private int matchesFound = 0;

public WebPageAnalyzerTask(String targetUrl, String searchPattern) {
    this.targetUrl = targetUrl;
    this.searchPattern = searchPattern;
}

@Override
public WebPageAnalysisTaskResult call() throws Exception {
    WebPageAnalysisTaskResult resultObj = new WebPageAnalysisTaskResult();
    resultObj.setAnalyzedUrl(this.targetUrl);
    long startTime = System.nanoTime();
    String htmlContent = this.getHtmlContentFromUrl();
    String resultContent = this.getAnalysisResults(htmlContent);
    resultObj.setOutputContent(resultContent);
    long endTime = System.nanoTime();
    resultObj.setElapsedTime(startTime, endTime);
    resultObj.setSuccess(true);
    return resultObj;
}

private String getAnalysisResults(String htmlContent) {
    StringBuilder contentResult = new StringBuilder();
    Pattern pattern = Pattern.compile(this.searchPattern);
    Matcher matcher = pattern.matcher(htmlContent);

    while (matcher.find()) {
        matchesFound++;
        contentResult.append(matcher.group(1)).append(System.lineSeparator());
    }
    return contentResult.toString();
}

private String getHtmlContentFromUrl() throws IOException {
    Connection httpConnection = Jsoup.connect(this.targetUrl);
    httpConnection.timeout(CONNECTION_TIMEOUT_IN_MILLIS);
    Document htmlDocument = httpConnection.get();
    return htmlDocument.html();
}

}

And the class that will use the tasks is:

public class WebAnalyzer {
private static final int THREAD_POOL_SIZE = 3;
private static final String[] ALLOWED_URL_SCHEMES = new String[]{"http"};
private static final String RESULT_OUTPUT_FOLDER = "output";
private String inputPath;
private final TextFileManager txtFileMgr = TextFileManager.getInstance();

public WebAnalyzer(String inputPath) {
    this.inputPath = inputPath;
}

public void startAnalysis(SearchPatternTypeEnum patternType) {
    try {
        if (this.inputPath == null || this.inputPath.isEmpty()) {
            throw new InvalidFilePathException("The input file path provided is not valid.");
        }
        List<String> urlsToBeProcessed = txtFileMgr.readLinesAsList(this.inputPath);

        if (urlsToBeProcessed != null && urlsToBeProcessed.size() > 0) {
            List<Callable<WebPageAnalysisTaskResult>> pageAnalysisTasks = this.buildPageAnalysisTasksList(urlsToBeProcessed, patternType.getRegexPattern());
            ExecutorService executor = Executors.newFixedThreadPool(THREAD_POOL_SIZE);
            List<Future<WebPageAnalysisTaskResult>> results = executor.invokeAll(pageAnalysisTasks);
            executor.shutdown();
            writeOutputFiles(results);
        } else {
            throw new NoContentToProcessException("The input file provided does not contain relevant information for analysis.");
        }

    } catch (Exception e) {
        e.printStackTrace();
    }
}



private void writeOutputFiles(List<Future<WebPageAnalysisTaskResult>> results) throws InterruptedException, ExecutionException, IOException {
    String outputFolder = File.listRoots()[0].getPath() + File.separator + RESULT_OUTPUT_FOLDER + File.separator;
    new File(outputFolder).mkdir();

    for (Future<WebPageAnalysisTaskResult> result : results) {
        WebPageAnalysisTaskResult taskResult = result.get();
        if (taskResult != null) {
            String fileName = this.generateFileName();
            txtFileMgr.writeContentToFile(outputFolder, fileName, taskResult.getOutputContent());
        }
    }
}

private List<Callable<WebPageAnalysisTaskResult>> buildPageAnalysisTasksList(List<String> urlsToBeProcessed, String searchPattern) {
    List<Callable<WebPageAnalysisTaskResult>> tasks = new ArrayList<>();
    UrlValidator urlValidator = new UrlValidator(ALLOWED_URL_SCHEMES);

    urlsToBeProcessed.forEach(urlAddress -> {
        if (urlValidator.isValid(urlAddress)) {
            tasks.add(new WebPageAnalyzerTask(urlAddress, searchPattern));
        }
    });

    return tasks;
}

private String generateFileName() {
    return String.format("%s.txt", UUID.randomUUID().toString());

}

}

Main class that will perform the call:

   public static void main(String[] args) {
    WebAnalyzer webAnalyzer = new WebAnalyzer("data/siteList.txt");
    webAnalyzer.startAnalysis(SearchPatternTypeEnum.HASHTAG_PATTERN);
}

So, how I am focusing this solution is to create a task for each URL being analized. Once the whole analysis is done for the URLS, I start to write the content (results of analysis) on a file for each URL.

I wonder to know what improvements could be made for the performance stuff (considering the multi-threading requirement).

Or if I can get any kindly suggestion to improve the implementation of this application.

I'll be grateful for any suggestion or feedback.

\$\endgroup\$
2
\$\begingroup\$

Some points in addition to ChrisWue's good answer:

  • It makes no sense to expose a mutator for the RegexPattern of SearchPatternTypeEnum

  • It also makes no sense to postfix a typename with it's typekind. You don't use TextFileManagerClass or WebAnalyzerClass. Why SearchPatternTypeEnum then?

  • readLinesAsList is ... useless. This method does nothing aside from materializing a nice lazy IO-Stream into memory, which is not a benefit. It doesn't provide exception handling, nor does it hide any complexity. That method is utterly useless.

  • writeContentToFile should use the java.nio API instead of the java.io API like this:

    try (BufferedWriter bw = Files.newBufferedWriter(Paths.get(folderName, fileName)
                                                    , StandardCharsets.UTF_8
                                                    , StandardOpenOption.WRITE)) {
    

    This way you get the benefit of cleaner and more differentiated Exceptions as well as support for a multitude of different filesystems.

  • Summarizingly these changes IMO make TextFileManager completely superfluous

  • You're conflating WebPageAnalyzerTask and WebPageAnalysisTaskResult's responsibilities. It's significantly cleaner to create a Result only when you actually do have a result instead of preemptively declaring a resultObj the first thing in your method. Instead I'd expose a static factory method on WebPageAnalysisTaskResult that takes the relevant information as parameters and constructs an immutable instance of Result. That way nobody will mess with what's inside a Result.

    This also allows for properly useful error handling in case of exceptions. Currently the Callable just silently terminates without generating a nice proper error when an Exception occurs.

  • Generally I don't follow with how you deal (or rather actually do not deal) with Exceptions. Instead of just passing every exception you ever encounter to the caller, you should limit the scopes of blocks where you expect to encounter Exceptions, and only in extremely rare cases throw a checked Exception as result. This makes for a cleaner and less noisy API to expose.

  • Disallowing "https", but allowing "http" as URL_SCHEME is bad and you should feel bad.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the response! For the mutator, I just thought it would be useful to limit the type of regex patterns supported, I followed the suggestions of ChrisWue related to that. I got rid of the TextFileManager since I noticed that it was not useful as you mentioned. I am refatoring now the Task and Result stuff for exception handling, thanks for the feedback since I was racking my brain trying to improve that. By the way, I know https should be enabled but was told that I should allow http and not https by now, it was a requirement. \$\endgroup\$ – Marcelo Tataje Aug 12 '17 at 15:45
  • \$\begingroup\$ I am changing some stuff so instead of abusing of enums, I am considering using a properties file. Also because I think that I would need to include some parameters to JSoup connection like user agent, accept encoding and also set the time out stuff. \$\endgroup\$ – Marcelo Tataje Aug 12 '17 at 15:47
2
\$\begingroup\$

I don't do a lot of Java, so here just a few things which caught my attention:

  1. You re-compile the regex pattern over and over again. Probably doesn't make a lot of difference compared to the time spent on downloading and parsing the web page but still not the most efficient use of CPU resources. You could simply compile the pattern once in SearchPatternTypeEnum and make getRegexPattern() return the compiled pattern.

  2. I don't see any reason why TextFileManager would need to be a singleton. Singletons are one of the most abused "design patterns". They have their uses occasionally but this isn't one of them as far as I can see.

  3. I don't quite see the point of simply re-throwing the caught IOExceptions in TextFileManager. Try-with-resources doesn't require a catch block as far as I know and since you're not doing anything useful in there you might as well omit it.

  4. WebAnalyzer gets given the inputPath but it's only sanity checked when you call startAnalysis. That check should happen right there in the constructor. You should probably also check that the file exists and present a slightly nicer error to the user in that case.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer, I will follow those recommendations for sure. I obviated the pattern stuff, my bad! So for just reading/writing a file is not singleton necessary? I mean, I will only use the process for read/write files. By the way, do you think is it ok to generate file names using UUID or would be better choice to use timestamp for that purpose? Thanks! \$\endgroup\$ – Marcelo Tataje Aug 12 '17 at 1:33
  • \$\begingroup\$ @MarceloTatajeL I think you're misunderstanding what the singleton does for you. Singleton means that there is only one instance of that class. It's not a means of ensuring that only one thread at the same time is accessing a file. The way it's written you might as well turn the two read/write methods into a couple of static helper methods. Principally UUID is fine for the filenames - it will ensure the names won't collide. Not the most user-friendly - you could try and incorporate an abbreviation of the webpage into the filename as well. \$\endgroup\$ – ChrisWue Aug 12 '17 at 2:31

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.