Inner class calling outer class method

I created a console program that tests report results from JasperReport. It retrieves the list of reports, splits it, and passes each sublist to a new thread. Each thread executes its own reports and compares the results with reference files.

My colleague says multithreading is wrong, but as usual he doesn't explain why. Any hint on what is not correct? He just spouts off something about using an inner class but was not clear, and it's hard to get more details.

This is how the code looks. I've omitted some irrelevant functions (with no side effects anyway). Every local variable is final.

```
public class ReportTester {
private class ThreadTest implements Runnable {
final List reports;
final Configuration config;

public ThreadTest(final List reports, Configuration config)
this.reports = reports;
this.config = config;
}

@Override
public void run() {
runTest(this.reports, this.config);
}
}

private final String format = "xml";
private final String directoryReport = "\var\reports";
private final JasperRestfulClient restClient = new JasperRestfulClient();
private final List reportsToBeTested = restClient.getReports();
volatile private errors = false; // SIDE EFFECT HERE. Public getter omitted.

private void runTest(List reports, Configuration config) {
for (Report report : reports) {
try {
String fileName = getFilePath(directoryReport, report, config);
restClient.runReport(report.getPath(), format,config, fileName);
compareWithReference(fileName, report, config);
}
catch(Exception ex){
ex.printStack();
}
}
}

public void runTestMultithreading(Configuration config, int numThread){
ExecutorService es = Executors.newCachedThreadPool();
List> splitted = splitReports(reportsToBeTested, nu

I can only see one 'bug' problem with this code, but there are a few other 'style' and 'simplicity' issues.

reportsDaTestare

What is this? It appears out of nowhere:

for (Report report : reportsDaTestare) {
    ....
}

I am worried that this is a typo for reportsToBeTested as you tanslated the code.... If it is, then you will be repeating all the reports in each thread..... it should be:

for (Report report : reports) {
    ....
}

Style

-
Test and class and method names derived from it typically relate to unit testing, like jUnit, etc. By common convention you should not use these method names for anything other than test code.

Use words like Validate, or Check instead.

-
Error handling.... I presume you have removed the actual error handling from this method, because this code will not compile... it is ex.printStackTrace() and not ex.printStack(). I hope the error-handling code you removed is 'better'.

catch(Exception ex){
        ex.printStack();
    }

-
ThreadTest is not a Thread, it is a Runnable. Call it something else.

Simplifications.

There are some tricks you can play that will simplify your code a bit.

Firstly, the ThreadTest class (which is a Runnable), does not need to exist at all. It is just a very light-weight container.

Consider the following multi-threaded loop method:

public void runTestMultithreading(final Configuration config, int numThread){
    ExecutorService es = Executors.newCachedThreadPool();
    List> splitted = splitReports(reportsToBeTested, numThread);

    for (final List reportsOfThread : splitted) {
        es.execute(new Runnable(){
            public void run() {
                runTests(reportsOfThread, config);
            }
        });
    }

    es.shutdown();
    es.awaitTermination(8, TimeUnit.HOURS);
}

Note how we use an anonymous class in here, and we can do it by making the config parameter final, as well as the relatively unknown final inside the for-each loop for (final List ....)

StackExchange Code Review Q#44749, answer score: 7