review comments. second round

gradle/wrapper/gradle-wrapper.properties

@@ -1,6 +1,6 @@
-#Tue May 19 15:07:28 EDT 2015
+#Wed Jun 17 12:57:36 EDT 2015
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-2.2.1-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-2.2.1-bin.zip

src/main/java/org/broadinstitute/hellbender/utils/MathUtils.java

@@ -137,19 +137,19 @@ public static double approximateLog10SumLog10(final double a, final double b, fi
         return approximateLog10SumLog10(a, approximateLog10SumLog10(b, c));
     }
 
-    public static double approximateLog10SumLog10(final double small, final double big) {
-        // make sure small is really the smaller value
-        if (small > big) {
-            return approximateLog10SumLog10(big, small);
+    public static double approximateLog10SumLog10(final double a, final double b) {
+        // this code works only when a <= b so we flip them if the order is opposite
+        if (a > b) {
+            return approximateLog10SumLog10(b, a);
         }
 
-        if (small == Double.NEGATIVE_INFINITY || big == Double.NEGATIVE_INFINITY) {
-            return big;
+        if (a == Double.NEGATIVE_INFINITY) {
+            return b;
         }
 
-        final double diff = big - small;
+        final double diff = b - a;
         if (diff >= JacobianLogTable.MAX_TOLERANCE) {
-            return big;
+            return b;
         }
 
         // OK, so |y-x| < tol: we use the following identity then:
@@ -158,7 +158,7 @@ public static double approximateLog10SumLog10(final double small, final double b
         // max(x,y) + log10(1+10^-abs(x-y))
         // we compute the second term as a table lookup with integer quantization
         // we have pre-stored correction for 0,0.1,0.2,... 10.0
-        return big + JacobianLogTable.get(diff);
+        return b + JacobianLogTable.get(diff);
     }
 
     public static double sum(final double[] values) {

src/main/java/org/broadinstitute/hellbender/utils/RandomDNA.java

@@ -24,7 +24,7 @@ public final class RandomDNA {
      * </p>
      */
     public RandomDNA() {
-        random = new Random();
+        this(new Random());
     }
 
 
@@ -35,10 +35,7 @@ public RandomDNA() {
      * @throws IllegalArgumentException if {@code rnd} is {@code null}.
      */
     public RandomDNA(final Random rnd) {
-        if (rnd == null) {
-            throw new IllegalArgumentException("the random number generator cannot be null");
-        }
-        random = rnd;
+        random = Utils.nonNull(rnd,"the random number generator cannot be null");
     }
 
     /**
@@ -47,7 +44,7 @@ public RandomDNA(final Random rnd) {
      * @param seed the random number generator seed.
      */
     public RandomDNA(final long seed) {
-        random = new Random(seed);
+        this(new Random(seed));
     }
 
     /**

src/main/java/org/broadinstitute/hellbender/utils/Utils.java

@@ -1,6 +1,7 @@
 package org.broadinstitute.hellbender.utils;
 
-import com.google.api.client.repackaged.com.google.common.base.Strings;
+import com.google.common.base.Strings;
+import org.apache.commons.lang3.ArrayUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -269,7 +270,10 @@ public static <T> List<T> append(final List<T> left, T ... elts) {
      * @param s the string to duplicate
      * @param nCopies how many copies?
      * @return a string
+     *
+     * @Deprecated use {@link Strings#repeat} directly. Left here temporarily for the ease of porting GATK3 code.
      */
+    @Deprecated
     public static String dupString(final String s, final int nCopies) {
         return Strings.repeat(s, nCopies);
     }
@@ -293,6 +297,8 @@ public static byte[] dupBytes(final byte b, final int nCopies) {
      * @return Parsed expressions.
      */
     public static String[] escapeExpressions(final String args) {
+        Utils.nonNull(args);
+
         // special case for ' and " so we can allow expressions
         if (args.indexOf('\'') != -1)
             return escapeExpressions(args, "'");
@@ -311,37 +317,20 @@ else if (args.indexOf('\"') != -1)
     private static String[] escapeExpressions(final String args, final String delimiter) {
         String[] command = {};
         final String[] split = args.split(delimiter);
-        String arg;
         for (int i = 0; i < split.length - 1; i += 2) {
-            arg = split[i].trim();
+            final String arg = split[i].trim();
             if (arg.length() > 0) { // if the unescaped arg has a size
-                command = Utils.concatArrays(command, arg.split(" +"));
+                command = ArrayUtils.addAll(command, arg.split(" +"));
             }
-            command = Utils.concatArrays(command, new String[]{split[i + 1]});
+            command = ArrayUtils.addAll(command, split[i + 1]);
         }
-        arg = split[split.length - 1].trim();
-        if (split.length % 2 == 1) { // if the command ends with a delimiter
-            if (arg.length() > 0) { // if the last unescaped arg has a size
-                command = Utils.concatArrays(command, arg.split(" +"));
-            }
+        final String arg = split[split.length - 1].trim();
+        if (split.length % 2 == 1 && arg.length() > 0) { // if the last unescaped arg has a size
+            command = ArrayUtils.addAll(command, arg.split(" +"));
         }
         return command;
     }
 
-    /**
-     * Concatenates two String arrays.
-     * @param A First array.
-     * @param B Second array.
-     * @return Concatenation of A then B.
-     */
-    public static String[] concatArrays(final String[] A, final String[] B) {
-        final String[] C = new String[A.length + B.length];
-        System.arraycopy(A, 0, C, 0, A.length);
-        System.arraycopy(B, 0, C, A.length, B.length);
-        return C;
-    }
-
-
     static public <T> List<T> reverse(final List<T> l) {
         final List<T> newL = new ArrayList<>(l);
         Collections.reverse(newL);
@@ -350,15 +339,10 @@ static public <T> List<T> reverse(final List<T> l) {
 
     public static byte [] arrayFromArrayWithLength(final byte[] array, final int length) {
         final byte [] output = new byte[length];
-        for (int j = 0; j < length; j++)
+        for (int j = 0; j < length; j++) {
             output[j] = array[(j % array.length)];
-        return output;
-    }
-
-    public static void fillArrayWithByte(final byte[] array, final byte value) {
-        for (int i=0; i<array.length; i++) {
-            array[i] = value;
         }
+        return output;
     }
 
     /**

src/main/java/org/broadinstitute/hellbender/utils/collections/IndexedSet.java

@@ -1,5 +1,7 @@
 package org.broadinstitute.hellbender.utils.collections;
 
+import org.broadinstitute.hellbender.utils.Utils;
+
 import java.util.*;
 
 /**
@@ -53,9 +55,7 @@ public IndexedSet(final int initialCapacity) {
      * if {@code values} array is {@code null} itself, or it contains {@code null}.
      */
     public IndexedSet(final Collection<E> values) {
-        if (values == null) {
-            throw new IllegalArgumentException("input values cannot be null");
-        }
+        Utils.nonNull(values, "input values cannot be null");
 
         final int initialCapacity = values.size();
         elements = new ArrayList<>(initialCapacity);
@@ -86,18 +86,15 @@ public IndexedSet(final Collection<E> values) {
      * if {@code values} collection is {@code null} itself, or it contains {@code null}.
      */
     public IndexedSet(final E... values) {
-        if (values == null) {
-            throw new IllegalArgumentException("input values cannot be null");
-        }
+        Utils.nonNull(values, "input values cannot be null");
 
         final int initialCapacity = values.length;
         elements = new ArrayList<>(initialCapacity);
         indexByElement = new HashMap<>(initialCapacity);
         int nextIndex = 0;
         for (final E value : values) {
-            if (value == null) {
-                throw new IllegalArgumentException("null element not allowed: index == " + nextIndex);
-            }
+            Utils.nonNull(values, "null element not allowed: index == " + nextIndex);
+
             if (indexByElement.containsKey(value)) {
                 continue;
             }
@@ -128,26 +125,6 @@ public List<E> asList() {
         return unmodifiableElementsListView;
     }
 
-    /**
-     * Throws an exception if an index is out of bounds.
-     *
-     * <p>
-     *     An element index is valid iff is within [0,{@link #size()}).
-     * </p>
-     *
-     * @param index the query index.
-     *
-     * @throws IllegalArgumentException {@code index} is out of bounds.
-     */
-    protected void checkIndex(final int index) {
-        if (index < 0) {
-            throw new IllegalArgumentException("the index cannot be negative: " + index);
-        }
-        if (index >= size()) {
-            throw new IllegalArgumentException("the index is equal or larger than the list length: " + index + " >= " + size());
-        }
-    }
-
     @Override
     public Iterator<E> iterator() {
         return asList().iterator();
@@ -190,9 +167,7 @@ public boolean contains(final Object o) {
      */
     @Override
     public boolean add(final E o) {
-        if (o == null) {
-            throw new IllegalArgumentException("the input argument cannot be null");
-        }
+        Utils.nonNull(o, "the input argument cannot be null");
         if (contains(o)) {
             return false;
         }
@@ -253,9 +228,6 @@ public boolean equals(final Object o) {
         if (o == this) {
             return true;
         }
-        if (o == null) {
-            return false;
-        }
         if (!(o instanceof IndexedSet<?>)) {
             return false;
         }
@@ -276,9 +248,7 @@ public boolean equals(final Object o) {
      * (as compared using {@link Object#equals} a this set with matching indices.
      */
     public boolean equals(final IndexedSet<?> other) {
-        if (other == null) {
-            throw new IllegalArgumentException("other cannot be null");
-        }
+        Utils.nonNull(other, "other cannot be null");
         final ArrayList<?> otherElements = other.elements;
 
         final int elementCount = elements.size();
@@ -312,7 +282,7 @@ public int hashCode() {
      * @return never {@code null}; as null is not a valid element.
      */
     public E get(final int index) {
-        checkIndex(index);
+        Utils.validIndex(index, size());
         return elements.get(index);
     }
 
@@ -326,9 +296,7 @@ public E get(final int index) {
      * values within [0,{@link #size()}).
      */
     public int indexOf(final Object o) {
-        if (o == null) {
-            throw new IllegalArgumentException("the query object cannot be null");
-        }
+        Utils.nonNull(o, "the query object cannot be null");
         return indexByElement.containsKey(o) ? indexByElement.get(o) : -1;
     }
 

src/main/java/org/broadinstitute/hellbender/utils/genotyper/AlleleListUtils.java

@@ -1,6 +1,7 @@
 package org.broadinstitute.hellbender.utils.genotyper;
 
 import htsjdk.variant.variantcontext.Allele;
+import org.broadinstitute.hellbender.utils.Utils;
 
 import java.util.AbstractList;
 import java.util.List;
@@ -51,13 +52,9 @@ public static <A extends Allele> boolean equals(final AlleleList<A> first, final
 
         for (int i = 0; i < alleleCount; i++) {
             final A firstSample = first.alleleAt(i);
-            if (firstSample == null) {
-                throw new IllegalStateException("no null samples allowed in sample-lists: first list at " + i);
-            }
+            Utils.nonNull(firstSample, "no null samples allowed in sample-lists: first list at " + i);
             final A secondSample = second.alleleAt(i);
-            if (secondSample == null) {
-                throw new IllegalArgumentException("no null samples allowed in sample-list: second list at " + i);
-            }
+            Utils.nonNull(secondSample,"no null samples allowed in sample-list: second list at " + i);
             if (!firstSample.equals(secondSample)) {
                 return false;
             }
@@ -82,9 +79,7 @@ public static <A extends Allele> boolean equals(final AlleleList<A> first, final
      * @return -1 if there is no reference allele, or a values in [0,{@code list.alleleCount()}).
      */
     public static <A extends Allele> int indexOfReference(final AlleleList<A> list) {
-        if (list == null) {
-            throw new IllegalArgumentException("the input list cannot be null");
-        }
+        Utils.nonNull(list, "the input list cannot be null");
         final int alleleCount = list.alleleCount();
         for (int i = 0; i < alleleCount; i++) {
             if (list.alleleAt(i).isReference()) {
@@ -104,9 +99,7 @@ public static <A extends Allele> int indexOfReference(final AlleleList<A> list)
      * @return never {@code null}.
      */
     public static <A extends Allele> List<A> asList(final AlleleList<A> list) {
-        if (list == null) {
-            throw new IllegalArgumentException("the list cannot be null");
-        }
+        Utils.nonNull(list, "the list cannot be null");
         return new AsList(list);
     }