Java - Refactor two nearly identical methods

I have two methods, one which counts the number of objects which are considered to have a lower value than a given object, and another which counts the number of objects which have a higher value than a given object. As you can probably tell, the two methods are practically identical:

public int countHigher(SomeObject a){
    if (a == null){
           throw etc...
    }
    int numberHigher = 0;
    for (SomeObeject b : this.listOfSomeObjects) {
        if (b.compareTo(a) == 1) {
            numberHigher++;
        }
    }
    return numberHigher;
}

public int countLower(SomeObject a){
    if (a == null){
           throw etc...
    }
    int numberLower = 0;
    for (SomeObeject b : this.listOfSomeObjects){
        if (b.compareTo(a) == -1){
            numberLower++;
        }
    }
    return numberLower;
}

I've refactored t开发者_Python百科he methods to call a private method:

private int coun(SomeObject a, int comparison){
    if (a == null){
           throw etc...
    }
    int number = 0;
    for (SomeObeject b : this.listOfSomeObjects){
        if (b.compareTo(a) == comparison){
            number++;
        }
    }
    return number;
}

But I do not find this solution satisfying. It would be possible to call the private method with an invalid integer (i.e. 10), and the additional error checking for this case is fairly ugly:

if (comparison < -1 || comparison > 1)
{
    throw blah
}

Using a boolean is also unsatisfactory, since in the future it is reasonably possible that I may want to count the number of objects of equal value.

Do you have an alternative refactoring solution?

Cheers,

Pete

What I would do:

• Implement a Comparator for the type.
• Pass instances of that Comparator instead of the int comparison parameter.
• One of the counters will wrap the Comparator in Collections.reverseOrder.

That will give you properly separated concerns.

I agree - comparing directly against an integer is fragile, since the comparisons are only guaranteed to return e.g. less than zero, and not specifically -1.

You can use a Predicate to perform the comparison. This encapsulates the evaluation of the condition that you want to count. For example,

   abstract class ComparisonPredicate
   {
       private SomeObject a;
       // set in constructor

       public abstract boolean evaluate(SomeObject b);
   }

   class LessThanPredicate extends ComparisonPredicate
   {

      public boolean evaluate(SomeObject b) {
          return a.compareTo(b)<0;
      }
   }

And then the count method becomes:

private int count(ComparisonPredicate comparison){
int number = 0;
for (SomeObeject b : this.listOfSomeObjects){
    if (comparison.evaluate(b)){
        number++;
    }
}
return number;
}

The method is then called like:

   SomeObject a = ...;
   int lessThanCount = count(new LessThanPredicate(a));    

This can be used to find the number of objects for any type of evaluation - not just less than/greater than comparison, but also equality and any other relations that are defined on your object.

Alternatively, you can normalize the comparison values to that they are -1, 0, 1.

Use Enums

public class test {
    private int count(Object a, Comparison c){
        if (a == null){
               throw new RuntimeException();
        }
        int number = 0;
        for (Object b : new Object[] {null, null}){
            if (b.compareTo(a) == c.val()){
                number++;
            }
        }
        return number;
    }

    public void test() {
        System.out.println(count(null, Comparison.GT));
        System.out.println(count(null, Comparison.LT));
    }
}

class Object implements Comparable {
    public int compareTo(java.lang.Object object) {
        return -1;
    }
}

enum Comparison {
    GT { int val() { return 1; } },
    LT { int val() { return -1; } };

    abstract int val();
}

If you would like to extend this to check for equals as well, you would just add an extra value to the enum.

The argument should be Comparator ( http://java.sun.com/j2se/1.5.0/docs/api/java/util/Comparator.html ) You can then call the method with an anonymous class to avoid implementing the Comparator explicitly if you only need it in one place. If the method gets even more complicated you may want to define your own interface that encapsulates more complex logic for example the logic of the inside the if statement itself.

Also if you are asked vote for labmdas (closures) to be implemented in the Java language:)

You can use an enum instead of int. Also, generally the a.compareTo(b) can return an arbitrary negative number when a is less than b and an arbitrary positive number when a is greater than b.

Here's my solution:

private static enum CountMode {CountHigher, CountLower};

private int count(SomeObject anObject, CountMode mode) {
    if (a == null) {
        // handle
    }

    int count = 0;
    for (SomeObject o : this.listOfSomeObjects) {
        int compare = o.compareTo(anObject);
        if (compare > 0 && mode == CountMode.CountHigher) {
            count++;
        } else if (compare < 0 && mode == CountMode.CountLower) {
            count++;
        }
    }

    return count;
}

And here's the modified code to handle the equal case as well:

private static enum CountMode {CountHigher, CountLower, CountEqual};

private int count(SomeObject anObject, CountMode mode) {
    if (a == null) {
        // handle
    }

    int count = 0;
    for (SomeObject o : this.listOfSomeObjects) {
        int compare = o.compareTo(anObject);
        if (compare > 0 && mode == CountMode.CountHigher) {
            count++;
        } else if (compare < 0 && mode == CountMode.CountLower) {
            count++;
        } else if (compare == 0 && mode == CountMode.CountEqual) {
            count++;
        }
    }

    return count;
}

I agree with many others; a Predicate-type object is correct here, but specifically want to call out mdma's point that assuming relying on the int result of Comparable.compareTo() is incorrect (or at least fragile) -- it's only guaranteed that Comparable returns a positive, 0, or negative number, the specific values of 1 and -1 aren't dictated (and many implementations don't retrun these). Again, I think a Predicate is the go, but as a workaround to this problem, and the risk of people passing in a non-standard int, you might do:

/**
 * ...
 * @param int the sign of the comparison; will count objects whose .compareTo(a)
 *    returns an int with the same sign as this value 
 */
private int count(SomeObject a, int comparisonSign){
    ...
    for (SomeObject b : this.listOfSomeObjects){
        if (Math.signum(b.compareTo(a)) == Math.signum(comparison)){
            number++;
        }
    }
    return number;
}

the use of signum means you're always comparing -1.0, 0, or 1.0 values.

Again, I wouldn't recommend this approach (think it makes the API very non-obvious, for a start), but thought I'd put it out here for curiosity's sake.

Most of the answers here have covered your options, but I'm going to add one more: a better data structure.

Depending on the nature of your data, a SortedSet<E>, or the richer NavigableSet<E> may serve your purpose better. Specifically, these 3 methods are of interest:

headSet(E toElement, boolean inclusive): Returns a view of the portion of this set whose elements are less than (or equal to, if inclusive is true) toElement.

tailSet(E fromElement, boolean inclusive): Returns a view of the portion of this set whose elements are greater than (or equal to, if inclusive is true) fromElement.

subSet(E fromElement, boolean fromInclusive, E toElement, boolean toInclusive): Returns a view of the portion of this set whose elements range from fromElement to toElement.

Here's an example usage (see also on ideone.com):

import java.util.*;
public class NavigableSetExample {
    public static void main(String[] args) {
        NavigableSet<String> names = new TreeSet<String>(
            Arrays.asList(
                "Bob", "Carol", "Alice", "Jill",  "Jack", "Harry", "Sally"
            )
        );
        
        System.out.println(names);
        // [Alice, Bob, Carol, Harry, Jack, Jill, Sally]

        // "Carol" and up
        System.out.println(names.tailSet("Carol", true));
        // [Carol, Harry, Jack, Jill, Sally]

        // below "Elizabeth"
        System.out.println(names.headSet("Elizabeth", false));
        // [Alice, Bob, Carol]

        // who stands between "Harry" and "Sally"?
        System.out.println(names.subSet("Harry", false, "Sally", false));
        // [Jack, Jill]     
    }
}

There's of course NavigableMap<K,V> if a map is more suitable than a set.

Related questions

• How to do inclusive range queries when only half-open range is supported (ala SortedMap.subMap)
  • Explains how SortedSet/Map is different from NavigableSet/Map

I would propose to wrap a private method and use function object pattern:

Interface for function object:

interface Filter<T>{
    boolean eligible(T t);
}

Count higher wrapper method to show the idea:

public int countHigher(final SomeObject a) 
{ 
 return coun(a, new Filter<SomeObject>()
           {   
                 public boolean eligible(SomeObject b){
                      return a.compareTo(b) == -1;
                 }
           }); 
} 

private helper method that counts eligible objects

private int coun(SomeObject a, Filter<SomeObject> filter){ 
     //...
}