current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

Let's say I have a List contains numbers:

1,2,3,4,5,6,7,8,9

and I would like to split this to two lists: - odd numbers - even numbers

So I can do this two ways:

Solution 1: Create two methods:

List<Integer> filterOdd(List<Integer> numbers){
    List<Integer> result  = new ArrayList<Integer>();
    for(Integer n : numbers){
        if(n % 2 != 0){
            result.add(n);
        }
    }
    return result;
}

List<Integer> filterEven(List<Integer> numbers){
    List<Integer> result  = new ArrayList<Integer>();
    for(Integer n : numbers){
        if(n % 2 == 0){
            result.add(n);
        }
    }
    return result;
 }

and In code I will call: 

List<Integer> numbers = Arrays.asList(new Integer[] { 1,2,3,4,5,6,7,8,9 });

List<Integer> oddNumbers = filterOdd(numbers);
List<Integer> evenNumbers = filterEven(numbers);

and do something with this lists.

Disadvantage: I do a two loops over one collection

Solution 2:

Create one method:

public void filter(List<Integer> numbers, List<Integer> oddNumbers, List<Integer> evenNumbers){
    for(Integer n : numbers){
        if(n % 2 != 0){
            oddNumbers.add(n);
        }else{
            evenNumbers.add(n);
        }
    }
}

and In code I will call: 

List<Integer> numbers = Arrays.asList(new Integer[] { 1,2,3,4,5,6,7,8,9 });
List<Integer> oddNumbers  = new ArrayList<Integer>();
List<Integer> evenNumbers = new ArrayList<Integer>();
filter(numbers, oddNumbers, evenNumbers);

Disadvantage: I hear that assign results for parameters is a bad practice.

Question is which solution is better?

share|improve this question
2  
"Disadvantage: I hear that assign results for parameters is a bad practice." <-- where did you read that? If it is the most practical for you, just use it... –  fge Jun 24 '13 at 12:51
    
I think the first one is better. Not only because it is most reusable, but because the second one can be created also by making calls to the 2 methods of the first solution –  py_script Jun 24 '13 at 15:43
    
Assigning to parameters is definitely an abomination. But it is not what the OP does here. So that principle, however true, just do not apply here. Calling a method on a parameter is not an assignment. –  abuzittin gillifirca Jun 25 '13 at 13:02
add comment

2 Answers

up vote 3 down vote

Here is a solution which replicates Guava's Predicate interface. Since this interface is really easy, here is how it is done:

public interface Predicate<T>
{
    // returns true if input obeys the predicate
    boolean apply(T input);
}

Given this interface, implement it for your types; and then do a method like this:

public static <T> List<List<T>> filteredLists(final List<T> list,
    final List<Predicate<T>> predicates)
{
    final int size = predicates.size();
    final List<List<T>> ret = new ArrayList<List<T>>(size);

    // Fill ret with initial lists
    for (int i = 0; i < size; i++)
        ret.add(new ArrayList<T>());

    // Now walk the predicates and add to the necessary lists

    for (final T element: list)
        for (int i = 0; i < size; i++)
            if (predicates.get(i).apply(element))
                ret.get(i).add(element);

    // Done! Return...
    return ret;
}

Now, if you had a utility class, say MyPredicates, with two methods even() and odd(), you could write:

final List<List<Integer>> filtered = filteredList(inputList,
    Arrays.asList(MyPredicates.even(), MyPredicates.odd()));

The "drawback" here is that it is up to the caller to remind what predicates where in what order in the calling lists, of course.

As to your MyPredicates hypothetical class:

public static final class MyPredicates
{
    // No instansiation
    private MyPredicates()
    {
    }

    public static Predicate<Integer> even()
    {
        return new Predicate<Integer>()
        {
            @Override
            public boolean apply(final Integer input)
            {
                return input.intValue() % 2 == 0;
            }
        }
    }

    public static Predicate<Integer> odd()
    {
        return new Predicate<Integer>()
        {
            @Override
            public boolean apply(final Integer input)
            {
                return input.intValue() % 2 == 1;
            }
        }
    }
}
share|improve this answer
add comment
up vote 3 down vote

Go for two methods :

  • the methods will lead to code that is more readable
  • the signatures make more sense, and you don't need to use parameters as return values
  • while this is probably slightly slower, we should remember that "Premature optimization is the root of all evil" (Knuth); unless you can show that having two loops is a performance bottle neck in your code : don't optimize.
  • this will be easier to refactor to using the Predicate approach that @fge suggests. Even though at this point that would, in my opinion, be overkill. Nevertheless, a valid option once lambdas are available in 8.
share|improve this answer
    
In fact, Java 8 will have Predicate... They have salvaged quite a few interfaces from Guava for that matter. Hurray! –  fge Jun 25 '13 at 10:18
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.