10
\$\begingroup\$

Write a method range that accepts an ArrayList of integers as a parameter and that returns the range of values contained in the list, which is defined as 1 more than the difference between the largest and smallest elements.

I feel like I'm beginning to understand conditionals, but for some reason this just makes me cringe. Is it good practice to have these conditionals as catch-alls in the beginning? Should I be using else ifs instead?

public static int range(ArrayList<Integer> list){
    if(list.size() == 0){
        return 0;
    }
    if(list.size() == 1){
        return 1;
    }

    int firstElement = list.get(1);
    int max = firstElement;
    int min = firstElement;

    for(int i = 0; i < list.size(); i++){
        int elementValue = list.get(i);
        if(max < elementValue){
            max = elementValue;
        }
        if(elementValue < min){
        min = elementValue;
        }
    }
return (max - min) + 1;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ if(list.size() == 1){ return 1; } is a nice optimisation \$\endgroup\$ – KarlM Dec 17 '15 at 2:29
  • 3
    \$\begingroup\$ @KarlM I disagree. Short inputs aren't going to be a performance problem anyway. The extra complexity of the special case is not worthwhile, when it can be handled just fine by the general case (once list.get(1) gets fixed as list.get(0)). \$\endgroup\$ – 200_success Dec 17 '15 at 3:16
10
\$\begingroup\$

To answer your question, yes, early returns are very good. Often, you will check all error conditions at the beginning of the method, then do your work knowing the input data is OK. You can read more about this here.


if(list.size() == 0){
    return 0;
}
if(list.size() == 1){
    return 1;
}

You can combine these into a single statement. Look at what you are returning - you are returning the value of list.size() in both of these conditions.

if(list.size() == 0 || list.size() == 1){
    return list.size();
}

if(elementValue < min){
min = elementValue;
}

For the most part, your indentation is very good. Just try to be a bit more consistent.

Other than these little things, this looks like very good code.

\$\endgroup\$
  • 3
    \$\begingroup\$ Actually you can even melt down that if condition. .size() will never return a negative integer, so if (list.size() <= 1) is perfectly valid instead \$\endgroup\$ – Vogel612 Dec 16 '15 at 22:26
  • \$\begingroup\$ @Vogel612 I thought of that, but I thought this condition expresses what is happening better. Anyway, Simon mentioned that in his answer. \$\endgroup\$ – Hosch250 Dec 16 '15 at 22:28
  • 2
    \$\begingroup\$ "you are returning the value of list.size() in both of these conditions" - that seems more like happenstance than something that should actually be used in the code. Particularly, it seems much more reasonable to raise an exception in the list.size() == 0 case, since there are no minimum or maximum values to subtract, and the list.size() == 1 case is only necessary because the questioner has mistakenly used list.get(1) as the "first" value. \$\endgroup\$ – user2357112 Dec 16 '15 at 22:33
  • \$\begingroup\$ In addition to this I would suggest to assign list.size() to a local variable instead of using an identical expression multiple times. As in int size = list.size();. \$\endgroup\$ – Tobold Dec 17 '15 at 13:00
11
\$\begingroup\$

Use this:

if (list.size() <= 1) {
    return list.size();
}

list.get(1) refers to the second element in the list, not the first. Either rename the variable or use list.get(0). With that change, you don't need to use a list size of 1 as a special case.


Only being able to do the job on ArrayList is quite useless. Your method can take an argument of type List instead.


No, it is no need to use else if in this case.

Rewriting with Iterable

You currently check the first item in the list twice. If your code would accept an Iterable<Integer>, you would support even more data types (You still support ArrayList also).

The only thing you need is the Iterator from the iterable. When you have that, you can first see if it contains at least one item. If it doesn't, return 0. Otherwise, you do similar to what you are already doing, but with different code.

public static int range(Iterable<Integer> numbers) {
    Iterator<Integer> iterator = numbers.iterator();
    if (!iterator.hasNext()) {
        return 0;
    }

    int firstElement = iterator.next();
    int max = firstElement;
    int min = firstElement;
    while (iterator.hasNext()) {
        int elementValue = iterator.next(i);
        if (max < elementValue) {
            max = elementValue;
        }
        if (elementValue < min) {
            min = elementValue;
        }
    }
    return (max - min) + 1;
}

Happy coding!

\$\endgroup\$
  • \$\begingroup\$ While I agree with you that it's not limited to ArrayList the description is : Write a method range that accepts an ArrayList of integers, so I can't blame OP for that. \$\endgroup\$ – Marc-Andre Dec 16 '15 at 20:40
  • 1
    \$\begingroup\$ @Marc-Andre The method will still accept ArrayLists, it's just that it will support a whole lot of other stuffs as well. \$\endgroup\$ – Simon Forsberg Dec 16 '15 at 20:59
10
\$\begingroup\$

Edge case

range(Integer.MIN_VALUE, Integer.MAX_VALUE) == 0; // true

This occurs due to overflowing. To handle this, you will need to update your method return type and temporary variables as long.

Java 8

It's trivial to get statistics from a LongStream via LongSummaryStatistics in order to retrieve the maximum and minimum values.

After adding an Optional and a healthy amount of using method references, a shorter Java 8 solution can be just one return statement:

private static long getRange(Collection<Integer> collection) {
    return Optional.of(collection)
                    .filter(c -> !c.isEmpty())
                    .map(Collection::stream)
                    .map(s -> s.mapToLong(Integer::longValue))
                    .map(LongStream::summaryStatistics)
                    .map(i -> i.getMax() - i.getMin() + 1)
                    .orElse(0L);
}

The use of Collection is in the same vein as @Simon's answer, to handle more kinds of 'ranges' instead of just ArrayList. On that note, the method should be taking in a List so that it can work with other implementations of that interface as well.

\$\endgroup\$
8
\$\begingroup\$

Foreach loop

When you don't need the index in a for loop, you can use a foreach loop.

for(int i = 0; i < list.size(); i++){
    int elementValue = list.get(i);
    if(max < elementValue){
        max = elementValue;
    }
    if(elementValue < min){
    min = elementValue;
    }
}

Could become :

    for (int elementValue : list) {
        if (max < elementValue) {
            max = elementValue;
        }
        if (elementValue < min) {
            min = elementValue;
        }
    }

list is a rather poor name for the argument, but this is a really small detail.

\$\endgroup\$
3
\$\begingroup\$

There is way too much code, and too many special cases within that code. Use Stack Overflow to write leaner code, like

import java.util.Collection;
import java.util.Collections;

…

public static int range(Collection<Integer> nums) {
    return nums.isEmpty() ?
           0 :
           1 + Collections.max(nums) - Collections.min(nums);
}

Even though the problem statement says that your function should accept an ArrayList<Integer>, you can satisfy that requirement more generically by accepting a List<Integer> or even a Collection<Integer>.

\$\endgroup\$
3
\$\begingroup\$
public static int range(ArrayList<Integer> list){

You do not need to limit this method to ArrayLists. With minor changes it could accept the more general List, Collection, or even Iterable, which makes your code more reusable (Simon covers Iterable, I will use Collection). You will still be able to call it with ArrayLists just like before.

I would prefer a more descriptive name than list (perhaps numbers), but the method is small and simple enough that it doesn't really matter. range is perhaps also not the best name, but it seems to be set in stone by the assignment.

Even if the purpose is obvious now, you should document your methods (especially public ones). Many development tools will be able to automatically fetch and render comments written on the form of Javadoc to help whoever is trying to use this method later on (which, in this case, will probably be you or your instructor, both of whom you want to keep happy).

    if(list.size() == 0){
        return 0;
    }

Checking your input early is a great idea. Returning 0 may or may not be the correct response to an empty list though - I would be inclined to throw an IllegalArgumentException, but this depends on how the method will be used. We can also use the more readable isEmpty method instead of explicitly comparing the size.

    if(list.size() == 1){
        return 1;
    }

You do not need to make this check (assuming you perform the correction noted below). Optimizing for single-element lists is pointless as handling them normally will be more than fast enough anyways.

    int firstElement = list.get(1);
    int max = firstElement;
    int min = firstElement;

The first element is accessed with get(0), not get(1). This doesn't cause any problems at the moment (you could have gotten any element from the list and it would still work), but fixing this allows us to get rid of the special case above. In fact, we do not need to get an element from the list at all, as long as we can be sure that there will be something in the list that is at least as large as max and something that is at least as small as min. We can guarantee this by using Integer.MIN_VALUE and Integer.MAX_VALUE. This lets us support the Collection interface more easily - as there is no direct way to get a particular element from a Collection.

    for(int i = 0; i < list.size(); i++){
        int elementValue = list.get(i);

Since you do not care about the index, and only want the value itself, you can use an "enhanced" for-loop instead. This gives you immediate access to the values instead of having to get them yourself via the index. Other benefits of the enhanced for-loop include better performance with some data structures (like LinkedLists) and compatibility with some data structures which do not use indexes. If you did need to use the index for something, we could not use Collections as they do not necessarily have indexes, and do not guarantee that elements are always in the same order when you iterate over them.

        if(max < elementValue){
            max = elementValue;
        }
        if(elementValue < min){
        min = elementValue;
        }

This is fine, except for the indentation. I would personally try to stay consistent with which side of the comparison I put elementValue on, but that's minor.

    }
return (max - min) + 1;
}

Again, fine except for indentation.

I've also gone ahead and inserted some spaces outside of parentheses (except those directly following a method name) because I find that easier to read, but you are free to disagree until you start working for/with someone with a strong opinion on the matter.

With all of that said, here is the code with my updates:

/**
 * Calculates the range of a collection of numbers.
 * The range is the largest number in the collection less the smallest number, plus one.
 * Empty collections have a range of 0.
 * @param numbers the numbers to scan through
 * @return the range of the collection
 */
public static int range(Collection<Integer> numbers) {
    if (numbers.isEmpty()) {
        return 0;
    }

    int max = Integer.MIN_VALUE;
    int min = Integer.MAX_VALUE;

    for (Integer number : numbers) {
        if (number > max) {
            max = number;
        }
        if (number < min) {
            min = number;
        }
    }

    return (max - min) + 1;
}
\$\endgroup\$
2
\$\begingroup\$

as you are interested only on max and min, you could use Math.min and Math.max methods and this:

        if (number > max) {
            max = number;
        }
        if (number < min) {
            min = number;
        }

could become this

min = Math.min(number,min);
max = Math.max(number,max);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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