Filtering a log file and extracts by different properties

Question

I have implemented a program to extract a log file which contains a header line, followed by zero or more data lines, in comma-separated value format. The file consists of 3 columns. The first column is the Unix time stamp, the second column is the country code, and the third column is the time in milliseconds. The data lines are not guaranteed to be in any particular order.

There are three types of sample log files:

1. Empty file
2. Single lined file (file containing a header line and one data line)
3. Multi lined file (file containing a header line and multi data lines)

The code must work for all three input log file types.

Sample file:

 TIMESTAMP,COUNTRY_CODE,RESPONSE_TIME
 1511190458,US,500
 1756118933,GB,137

DataFiltererTest.java

public class DataFiltererTest {

@Test
public void shouldReturnEmptyCollection_WhenLogFileIsEmpty() throws FileNotFoundException {
    assertTrue(DataFilterer.filterByCountry(openFile("src/test/resources/empty"), "GB").isEmpty());
}

@Test
public void shouldReturnfilteredrows_ByCountry_WhenLogFileIsMultiLines() throws FileNotFoundException {
    String[] myArray = { "1433190845", "US", "539", "1433666287", "US", "789", "1432484176", "US", "850" };
    Collection<?> expected = new ArrayList<String>(Arrays.asList(myArray));
    assertEquals(expected, DataFilterer.filterByCountry(openFile("src/test/resources/multi-lines"), "US"));

}

@Test
public void shouldReturnfilteredrows_ByCountry_WhenLogFileIsSingleLine() throws FileNotFoundException {
    String[] myArray = { "1431592497", "GB", "200" };
    Collection<?> expected = new ArrayList<String>(Arrays.asList(myArray));
    assertEquals(expected, DataFilterer.filterByCountry(openFile("src/test/resources/single-line"), "GB"));
}

@Test
public void shouldReturnEmptyCollection_ByCountry_WhenLogFileIsSingleLine() throws FileNotFoundException {
    assertTrue(DataFilterer.filterByCountry(openFile("src/test/resources/single-line"), "US").isEmpty());
}

@Test
public void shouldReturnEmptyCollection_ByCountryWithResponseTimeAboveLimit_WhenLogFileIsEmpty()
        throws FileNotFoundException {
    assertTrue(DataFilterer
            .filterByCountryWithResponseTimeAboveLimit(openFile("src/test/resources/empty"), "GB", 10).isEmpty());
}

@Test
public void shouldReturnFilteredRows_ByCountryWithResponseTimeAboveLimit_WhenLogFileIsMultiLines()
        throws FileNotFoundException {
    String[] myArray = { "1433666287", "US", "789", "1432484176", "US", "850" };
    Collection<?> expected = new ArrayList<String>(Arrays.asList(myArray));
    assertEquals(expected, DataFilterer
            .filterByCountryWithResponseTimeAboveLimit(openFile("src/test/resources/multi-lines"), "US", 550));

}

@Test
public void shouldReturnFilteredRows_ByCountryWithResponseTimeAboveLimit_WhenLogFileIsSingleLine()
        throws FileNotFoundException {
    String[] myArray = { "1431592497", "GB", "200" };
    Collection<?> expected = new ArrayList<String>(Arrays.asList(myArray));
    assertEquals(expected, DataFilterer
            .filterByCountryWithResponseTimeAboveLimit(openFile("src/test/resources/single-line"), "GB", 150));
}

@Test
public void shouldReturnEmptyCollection_ByCountryWithResponseTimeAboveLimit_WhenLogFileIsSingleLine()
        throws FileNotFoundException {
    assertTrue(DataFilterer
            .filterByCountryWithResponseTimeAboveLimit(openFile("src/test/resources/single-line"), "GB", 200)
            .isEmpty());
}

@Test
public void shouldReturnEmptyCollection_ByCountryWithResponseTimeAboveAverage_WhenLogFileIsEmpty()
        throws FileNotFoundException {
    assertTrue(DataFilterer.filterByResponseTimeAboveAverage(openFile("src/test/resources/empty")).isEmpty());
}

@Test
public void shouldReturnFilteredRows_ByCountryWithResponseTimeAboveAverage_WhenLogFileIsMultiLines()
        throws FileNotFoundException {
    String[] myArray = { "1433190845", "US", "539", "1433666287", "US", "789", "1432484176", "US", "850" };
    Collection<?> expected = new ArrayList<String>(Arrays.asList(myArray));
    double average = getAverageForFilter("src/test/resources/multi-lines");
    DataFilterer.setAverage(average);
    assertEquals(expected,
            DataFilterer.filterByResponseTimeAboveAverage(openFile("src/test/resources/multi-lines")));

}

@Test
public void shouldReturnEmptyCollection_ByCountryWithResponseTimeAboveAverage_WhenLogFileIsSingleLine()
        throws FileNotFoundException {
    double average = getAverageForFilter("src/test/resources/single-line");
    DataFilterer.setAverage(average);
    assertTrue(DataFilterer.filterByResponseTimeAboveAverage(openFile("src/test/resources/single-line")).isEmpty());
}

private double getAverageForFilter(String filename) throws FileNotFoundException {
    return new Average().findAverage(openFile(filename));
}

private FileReader openFile(String filename) throws FileNotFoundException {
    return new FileReader(new File(filename));
}

}

DataFilterer.java

public class DataFilterer {

private static double average;

// Method to set the average value of RESPONSETIME
    public static void setAverage(double avg) {

        average = avg;
    }

    // Method to get the average value of RESPONSETIME
    public static double getAverage() {

        return average;
    }

public static Collection<?> filterByCountry(Reader source, String country) {
    BufferedReader br = new BufferedReader(source);
    String line = null;
    Collection<String> additionalList = new ArrayList<String>();
    int iteration = 0;
    try {
        while ((line = br.readLine()) != null) {
            // Logic to remove header from the input data.
            if (iteration == 0) {
                iteration++;
                continue;
            }
            String[] myArray = line.split(",");

            List<String> myList = new ArrayList<String>(Arrays.asList(myArray));

            if (myList.contains(country)) {
                additionalList.addAll(myList);
            }
            else
            {
                return Collections.emptyList();
            }

        }


    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    } finally {
        try {
            br.close();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

    return additionalList;
}



public static Collection<?> filterByCountryWithResponseTimeAboveLimit(Reader source, String country, long limit) {

        BufferedReader br = new BufferedReader(source);
        String line = null;
        Collection<String> additionalList = new ArrayList<String>();
        int iteration = 0;
        long count = 0;
        long responseTime = 0;
        try {
            while ((line = br.readLine()) != null) {
                // Logic to remove header from the input data
                if (iteration == 0) {
                    iteration++;
                    continue;
                }
                String[] myArray = line.split(",");

                List<String> myList = new ArrayList<String>(Arrays.asList(myArray));
                for (String eachval : myArray) {
                    // Finding the RESPONSE TIME from the input line
                    boolean isNumeric = eachval.chars().allMatch(x -> Character.isDigit(x));
                    if (isNumeric) {
                        count = eachval.chars().count();
                        // Identifying between RESPONSETIME and
                        // REQUEST_TIMESTAMP.Unix Timestamp will be always 10
                        // digits or 13 digits
                        if (count < 10) {
                            responseTime = Integer.parseInt(eachval);
                            if (myList.contains(country)) {
                                if (responseTime > limit) {
                                    additionalList.addAll(myList);
                                }
                            }
                            else
                            {
                                return Collections.emptyList();
                            }

                        }
                    }
                }
            }

        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } finally {
            try {
                br.close();
            } catch (IOException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }

        return additionalList;

}

public static Collection<?> filterByResponseTimeAboveAverage(Reader source) {
    BufferedReader br = new BufferedReader(source);
    String line = null;
    double average = 0.0;
    Collection<String> additionalList = new ArrayList<String>();
    average = getAverage();

    long responseTime = 0;
    int iteration = 0;
    long count = 0;
    String[] myArray = null;
    try {
        while ((line = br.readLine()) != null) {
            // Logic to remove header from the input data.
            if (iteration == 0) {
                iteration++;
                continue;
            }

            myArray = line.split(",");
            List<String> myList = new ArrayList<String>(Arrays.asList(myArray));
            for (String eachval : myArray) {
                // Finding the RESPONSE TIME from the input line
                boolean isNumeric = eachval.chars().allMatch(x -> Character.isDigit(x));
                if (isNumeric) {
                    count = eachval.chars().count();
                    // Identifying between RESPONSETIME and
                    // REQUEST_TIMESTAMP.Unix Timestamp will be always 10
                    // digits or 13 digits
                    if (count < 10) {
                        responseTime = Integer.parseInt(eachval);
                        if (responseTime > average) {

                            additionalList.addAll(myList);

                        }
                        else
                        {
                            return Collections.emptyList();
                        }

                    }
                }
            }

        }


    } catch (Exception e) {
        e.printStackTrace();
    } finally {
        try {
            br.close();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

    return additionalList;


}
}

Average.java

 //This class is to find average of RESPONSETIME Field of input file
 public class Average {

// Method to calculate the average of RESPONSETIME
 public double findAverage(Reader source) {

    BufferedReader br = new BufferedReader(source);
    String line = null;
    int iteration = 0;
    String[] myArray;
    Collection<Long> responseTimeList = new ArrayList<Long>();
    long responseTime = 0;
    long count = 0;
    double average = 0.0;
    try {
        while ((line = br.readLine()) != null) {
            // Logic to remove header from the input file
            if (iteration == 0) {
                iteration++;
                continue;
            }
            myArray = line.split(",");
            for (String eachval : myArray) {
                // Finding the RESPONSE TIME field from the input line
                boolean isNumeric = eachval.chars().allMatch(x -> Character.isDigit(x));
                if (isNumeric) {
                    count = eachval.chars().count();
                    // Identifying between RESPONSETIME and
                    // REQUEST_TIMESTAMP. Unix Timestamp will be always 10
                    // digits or 13 digits
                    if (count < 10) {
                        responseTime = Integer.parseInt(eachval);
                        responseTimeList.add(responseTime);
                    }
                }
            }

        }
        // Calculating the average
        average = responseTimeList.stream().mapToInt(Long::intValue).average().getAsDouble();

    } catch (Exception e) {
        e.printStackTrace();
    } finally {
        try {
            br.close();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

    return average;

}

}

The code works as expected for all the input file types. Please suggest improvements for this code. Also, please check and suggest whether I need to write this many number of test cases in TDD methodology.

Answer 1

Quick skim results:

• The indentation is off. This might be an artefact of how you pasted the code into the question, though.
• Inconsistent use of newlines at the start and end of blocks.
  Sometimes you start a block and put an empty line at its start, sometimes you just don't put a newline there. The same goes for the end of blocks.
  You should be able to configure your formatting preferences to take care of that for you :)
• There's some leftover auto-generated TODO comments. If they are fixed, drop them :)

Bugs!

• When any item in your logfile does not match its filter criteria, all the filter methods return an empty list. I don't think that's desired:

  Consider the following logfile:

  TIMESTAMP,COUNTRY_CODE,RESPONSE_TIME
  1425859632,US,500
  1452145245,GB,137
  

  If you now call filterByCountry(logfile, "US"), the result is an empty list. That's most definitively not desired!

• There is a slim possibility that the caller of the method expects the Reader it passes to remain open. As of now you're closing the BufferedReader. Per the spec, closing a BufferedReader must also close its underlying Reader. This should not be much of an issue here, because you probably expected the method to consume the whole file anyways, but it's something to be aware of.
  It becomes significantly more relevant when you're handling Writers that follow the same mechanic, but are usually intended to be kept open.

General approach:

Your overall approach is most likely very slow. You're parsing possibly extreme quantities of files each time you want to filter by anything. If you run these "queries" a lot, you should strongly consider aggregating the log entries in a database.
Databases are explicitly designed for querying and aggregation of high volumes of data. This could give you an extreme boost to performance. All RDBMS I know of support importing from CSV.

You're also not making use of Object orientation at all. Currently log entries are stored contiguously in a List. This means that any code that consumes the log entries must know how many fields an entry has when iterating over that list. It also means that any change to the structure of your logging entries requires changes in the code using the results from the presented classes, that may not be obvious or even easy to find.

To avoid this, you should model a LogEntry in a class, somewhat like this:

@Value
public class LogEntry {
    private long timestamp;
    private String countryCode; // possibly Locale?
    private int responseTime;
}

Sidenote: I'm using Lombok's @Value annotation here to automatically generate getters and a constructor for me.

This allows you to make your filter methods return a significantly cleaner (and nicer) datatype than Collection<?> with contiguously stored data. Instead we get Collection<LogEntry>.

This also exposes an issue with how you parse the log entries for filtering on RESPONSE_TIME. Currently you just assume that the response time is always less than 1000000000. That is probably a reasonable assumption. You also assume that it's the only value in the log entry that only contains digits and is shorter than 10 characters. That may be a less reasonable assumption (especially if the log-entry format changes).
To find it in your entry you iterate all values in each entry and manually check each character for being numeric and then check the length of the entry as well. If the columns in your logfiles are always the same, that makes no sense. You know the columns, you know the index they have, make use of that knowledge.
If you can only guarantee that the log entry columns are consistent in a single file and the header indicates how they are arranged, you should be using the header to determine the indices of your columns.

Consider something like the following:

public static Collection<LogEntry> filterByCountry(Reader source, String country) throws IOException {
    BufferedReader br = new BufferedReader(source);
    int timestampCol = -1;
    int countryCol = -1;
    int responseTimeCol = -1;
    String line = br.readLine();
    // File completely empty, not even a header
    if (line == null) {
        return Collections.emptyList();
    }
    String[] headers = line.split(",");
    for (int i = 0; i < headers.length; i++) {
        String h = headers[i];
        if (h.equals("TIMESTAMP")) {
            timestampCol = i; continue;
        }
        if (h.equals("COUNTRY_CODE")) {
            countryCol = i; continue;
        }
        if (h.equals("RESPONSE_TIME")) {
            responseTimeCol = i; continue;
        }
    }
    Collection<LogEntry> result = new List<>();
    while ((line = br.readLine()) != null) {
        String[] logEntry = line.split(",");
        LogEntry entry = new LogEntry(Long.parseLong(logEntry[timestampCol])
            , logEntry[countryCol]
            , Integer.parseInt(logEntry[responsetimeCol]));
        if (entry.getCountry().equals(country)) {
            result.add(entry);
        }
    }
    return result;
}