Average calculator with filters

Question

I am a beginner in Java and I have a hobby project to create a tool for statistical calculations. Users can upload csv with results of a survey. The responses stored in a Response object which has two maps: Map<Integer, String> questionMap, Map<Integer, String> answerMap, where the integer(questionID) links the question and answer.

I would like implement an extra feature: to filter the responses to certain values. Therefore the Filter class stores a questionID (int) and a filterValue (String). Now I would like to calculate the averages of the fields. I have a working implementation but I hope you can help me to make it better:

public List<AverageResult> calculateAverage(List<Response> responses,     List<Filter> filters){
    List<Response> responsesCopy = new ArrayList<>(responses);
    List<AverageResult> averages = new LinkedList<>();
    Set<Integer> questionIDSet = responses.get(0).getAnswerMap().keySet();
    for(Integer questionID :questionIDSet) {
        double sum = 0;
        int count=0;
        for(Iterator<Response> iter = responsesCopy.iterator(); iter.hasNext();) {
            Response actualResponse = iter.next();
            if(filterMatch(actualResponse,filters)) {
                if(isNumber(actualResponse, questionID)) {
                    sum += Double.valueOf(actualResponse.getAnswerMap().get(questionID));
                    count += 1;
                }                   
            }else {
                iter.remove();
            }
        }
        Double average = null;
        if(count != 0) {
            average = sum/count;
            AverageResult avRes = new AverageResult();
            avRes.setAverage(average);
            avRes.setQuestionID(questionID);
            avRes.setCount(count);
            averages.add(avRes);
        }
    }
    return averages;
}

private boolean isNumber(Response response, int questionID) {
    Pattern p = Pattern.compile("[0-9]*\\.?[0-9]+");
    return (p.matcher(response.getAnswerMap().get(questionID)).matches());
}

private boolean filterMatch(Response response, List<Filter> filters) {
    for(Filter filter: filters) {
        if(!response.getAnswerMap().get(filter.getQuestionID()).equals(filter.getFilterValue())) {
            return false;
        }
    }
    return true;
}

Answer 1

LinkedList?

    List<AverageResult> averages = new LinkedList<>();

If there is a reason why this should be a LinkedList rather than an ArrayList, you should comment it. An ArrayList will normally use less memory if it fits exactly and allows easy random access. So is often preferable for containers that are built all at once as this one seems to be.

That's not to say that it is wrong to use a LinkedList here. It's to say that I don't see any reason in this segment of code to use a LinkedList. With more context, that might make perfect sense. So there should be a comment here letting us know that you thought about it and the basis for your decision.

Be careful

    Set<Integer> questionIDSet = responses.get(0).getAnswerMap().keySet();

This assumes that the first response has an answerMap with answers to all the possible questions. But that is a contract that you will actively have to work to maintain. It may be true now, but a future change could easily make it not true. Consider instead

    Set<Integer> questionIDSet = responses.get(0).getQuestionIDs();

Yes, this means defining a new method on responses.

or even better something like

    Set<Integer> questionIDSet = sample.getQuestionIDs();
    List<Response> responses = sample.getResponses();

So you can pass one sample object rather than a container of responses.

Yes, this requires an entire new class.

Keep it simple

        for(Iterator<Response> iter = responsesCopy.iterator(); iter.hasNext();) {
            Response actualResponse = iter.next();
            if(filterMatch(actualResponse,filters)) {
                if(isNumber(actualResponse, questionID)) {
                    sum += Double.valueOf(actualResponse.getAnswerMap().get(questionID));
                    count += 1;
                }                   
            }else {
                iter.remove();
            }
        }

So you create a copy of the responses so that you can iterate over them and remove those that do not match the filters. If they do match and are numeric, you calculate the sum and count. That's fine, but it seems more complicated than necessary. You never use responseCopy after this section. So why do you have to remove the entries that don't match? Why not just

        for (Response actualResponse : responses) {
            if (filterMatch(actualResponse, filters)) {
                if (isNumber(actualResponse, questionID)) {
                    sum += Double.valueOf(actualResponse.getAnswerMap().get(questionID));
                    count++;
                }
            }
        }

That will return the same sum and count values. But it doesn't require making a copy of responses or modifying the iterator. Or consider

        for (Response actualResponse : responses) {
            Map<Integer, String> answers = actualResponse.getAnswerMap();
            if (filterMatch(answers, filters)) {
                String answer = answers.get(questionID);
                if (isNumber(answer)) {
                    sum += Double.valueOf(answer);
                    count++;
                }
            }
        }

Rather than passing the response to filterMatch and isNumber, this narrows down to the information that they actually want. This makes isNumber general purpose.

Yes, this requires changes to the other methods.

The ++ is more idiomatic than += 1.

Declare at the smallest scope that covers

        Double average = null;
        if(count != 0) {
            average = sum/count;

Consider

        if (count > 0) {
            Double average = sum / count;

Now if the count is not greater than 0, we don't even bother allocating a place to store the average. We only do that if we're going to calculate it. Since we don't use average outside the if, this changes no functionality.

As a functional matter, there wouldn't seem to be a difference between not equal and greater than. But I find it to be more readable. This makes it clear that negative values would not be handled. The original just implicitly assumes that there will never be negative values. But I had to think about that. Now I don't. If we counted something, we calculate the average and return it as a result.

Right after writing the code is when you know the most about it. It's the best time to make the intentions of the code clear. Because when you look at it in six months, you won't necessarily remember all the clever things you did today. So make the code clear or comment why you can't.

And it will really help if someone else is the next person to edit the code. Since that person never did know all the decisions you made while developing the code.