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 7 down vote favorite
2

I have written this method which calculates the range (max - min) of an ArrayList. I have two for loops each for a different ArrayList. The for loops do exactly the same thing except they each use a different list, so I was wondering if there was a way to just use one for loop (or any other way) that does the work but iterates through both ArrayLists so that I don't have nearly identical for loops.

public static void calcRange(ArrayList <Integer> list1, ArrayList<Integer> list2){
    int max = list1.get(0);
    int min = list1.get(0);
    int max2 = list2.get(0);
    int min2 = list2.get(0);

    for (int i : list1){
        if ( i > max){
            max = i;
        } else if (i < min){
            min = i;
        }
    }
    for (int i : list2){
        if ( i > max2){
            max2 = i;
        } else if (i < min2){
            min2 = i;
        }
    }
    System.out.println("List 1 Range: "+(max - min)+"\tList 2 Range: "+(max2 - min2));
}
share|improve this question

4 Answers 4

up vote 4 down vote accepted

One way you can remove duplicated code is to create a method:

public static int[] GetMaxMin (ArrayList <Integer> list)
{
    int max = list.get(0);
    int min = list.get(0);

    for (int i : list){
        if (i > max) {
            max = i;
        } else if (i < min) {
            min = i;
        }
    }
    int[] maxMin = {max, min};

    return maxMin;
}

This can then be called like this:

max = GetMaxMin(list)[0];
min = GetMaxMin(list)[1];

However, the above said, I would not recommend doing this because it has two purposes - to get the max value and the min value in the list. What I would recommend is to implement two methods - one to get the max value and one to get the min value:

public static int GetMax (ArrayList <Integer> list)
{
    int max = list.get(0);

    for (int i : list){
        if (i > max) {
            max = i;
        }
    }

    return max;
}

GetMin would be similar. Then, all you would need to do is this:

int max = GetMax(list1);
int min = GetMin(list1);
int max2 = GetMax(list2);
int min2 = GetMin(list2);

As you are calculating a range of values, though, you should just write a method to do that:

public static int CalculateRange (ArrayList <Integer> list)
{
    int max = list.get(0);
    int min = list.get(0);

    for (int i : list){
        if (i > max) {
            max = i;
        } else if (i < min) {
            min = i;
        }
    }

    return max - min;
}

As brought up in the comments, this will crash if there are 0 elements in the list. To prevent this, you should probably check this before you try to get the range, like this:

if (list.size() != 0) {
    System.out.printf("List 1 Range: %d\tList 2 Range: %d\n", CalculateRange(list1), CalculateRange(list2));
} else {
    System.out.println("There are no elements in this array");
}
share|improve this answer
    
I'm not a big java person, but would list.get(0) throw an error if the list had no entries? Perhaps this would be a desirable outcome though. –  DLeh 21 mins ago
    
@DLeh Yes. You cannot get the value at an index of an ArrayList (or any other collection/array for that matter) if there is no value there. –  Hosch250 19 mins ago
up vote 4 down vote

Hosch250 and h.j.k. have the right idea here, to reduce the code to a function that returns the range (max - min) for each list. The function, according to Hosch250, will have the signature:

public static int CalculateRange (ArrayList <Integer> list)

I would recommend changing that to a more Java-standard capitalization of having a lower-case C in CalculateRange. i.e. calculateRange. Additionally, I would change the input type from ArrayList, to just Collection. There is no need to restrict the function to just one concrete type.

Finally, because you have the option, I would strongly recommend that you investigate the new Java 8 features which, in this case, would help a lot.

The idea would be to convert your Collection<Integer> to an int stream, and to then compute the IntSummaryStatistics you need.... something like:

public static int calculateRange(Collection<Integer> data) {
    IntSummaryStatistics stats = data.stream()
            .filter(d -> d != null)
            .mapToInt(d-> d.intValue())
            .summaryStatistics();
    return stats.count() == 0 ? 0 : stats.max() - stats.min();
}

Then, in your main method, I would also recommend the use of 'formatted' print, using the Format syntax

System.out.printf("List 1 Range: %d\tList 2 Range: %d\n",
        calculateRange(list1),
        calculateRange(list2));
share|improve this answer
    
I'll try to remember about Java standards - I'm used to C# and C++ standards. –  Hosch250 15 hours ago
up vote 4 down vote

The following section should be a method itself:

for (int i : list1){
    if ( i > max){
        max = i;
    } else if (i < min){
        min = i;
    }
}
// return max - min here

E.g. with the method name getRangeForList(List<Integer> list). Use interfaces such as List instead of the implementation (ArrayList) so that your code is not too restrictive.

Your calcRange() method can then be (using varargs so that you are not limited to just two arguments) :

public static void calcRange(List<Integer>... lists) {
    if (lists == null) {
        return;
    }
    int index = 0;
    for (final List<Integer> current : lists) {
        System.out.printf("List %d Range: %d\t", ++index, getRangeForList(current));
    }
    System.out.println();
}
share|improve this answer
up vote 0 down vote

Analysis

The current running time is 0(m+n) where m = length(list1) and n = length(list2). Combining the loops reduces the running time to 0(n) where n is the length of the longer list.

Single Loop Example

Procedure in psuedo code:

calcRange(list1, list2)

  length1 = list1.length
  length2 = list2.length

  stop = maximum(length1, length2)

  min1 = list1[0]
  max1 = list1[0]
  min2 = list2[0]
  max2 = list2[0]

  loop from i = 1 to stop

    if i< length1
      min1 = minimum(min1, list1[i])
      max1 = maximum(max1, list1[i])

    if i< length2
      min2 = minimum(min2, list2[i])
      max2 = maximum(max2, list2[i])

  end loop

  return list(min1, max1, min2, max2)

Further refactoring is possible, but not shown to make the high level structure read more clearly.

share|improve this answer

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.