4
\$\begingroup\$

My code works; I am just seeing if there's an optimized way to write these methods or even better perhaps combine them (since majority of their contents look similar).

public class IntegerUtils {

    public static int getMaxValue(int[] array) {
        int value = Integer.MIN_VALUE;

        if (array.length <= 0) {
            throw new IllegalArgumentException("Array is empty.");
        }

        for (int i = 0; i < array.length; i++) {
            if (array[i] > value) {
                value = array[i];
            }
        }
        return value;
    }

    public static int getMinValue(int[] array) {
        int value = Integer.MAX_VALUE;

        if (array.length <= 0) {
            throw new IllegalArgumentException("Array is empty.");
        }

        for (int i=0; i < array.length; i++) {
            if (array[i] < value) {
                value = array[i];
            }
        }
        return value;
    }   
}

Here's my unit test (which works as well):

import org.junit.Test;

public class IntegerUtilsTest {

    int[] array = new int[] {1, 2, 3, 58, 47, 229, 40};

    @Test
    public void shouldBeMaxValue() {
        int maxValue = IntegerUtils.getMaxValue(array);
        assert(maxValue == 229);
    }

    @Test
    public void shouldBeMinValue() {
        int minValue = IntegerUtils.getMinValue(array);
        assert(minValue == 1);
    }

    @Test(expected = IllegalArgumentException.class)
    public void shouldBeIllegalArgumentException() {
        int[] emptyArray = new int[] {};
        int maxValue = IntegerUtils.getMaxValue(emptyArray);
        int minValue = IntegerUtils.getMinValue(emptyArray);
    }
}

Just seeking feedback regarding optimization and / or code elegance.

\$\endgroup\$

4 Answers 4

8
\$\begingroup\$

Real skill means knowing your libraries. Instead of 20 lines of handcrafted code, I'd rather go for:

int max = Arrays.stream(array).max().orElseThrow(() -> new IllegalArgumentException("Array is empty"));

... the same for min.

As a general advice: Java is a very mature language, and chances are good that every "simple everyday task" is already solved in the library. (Also note that most books on Java are from the pre-java-8 era and may be considered somewhat outdated today.)

\$\endgroup\$
7
  • 1
    \$\begingroup\$ The author didn't mention it, but perhaps they are writing this for learning. Do you have anything else to review about the code, other than offering an alternative from the Java libraries? \$\endgroup\$
    – Phrancis
    May 6, 2017 at 5:42
  • \$\begingroup\$ I knew there's an easier way using Streams in Java 1.8. I am just practicing for interview questions. No whiteboard will allow you to quote or specify a 3rd party library. \$\endgroup\$ May 6, 2017 at 7:50
  • 3
    \$\begingroup\$ @PacificNW_Lover This is not a 3rd party libary, this is the base lib. And frequently being on "the other side" of an interview, I would rather hire you for vague knowledge of the standard lib than for the ability to write rather simple low-level code. (Apart from that, the only thing I have to say is that I'd test for array.length == 0 as being less than zero is impossible.) \$\endgroup\$
    – mtj
    May 6, 2017 at 11:09
  • 2
    \$\begingroup\$ If I had an interviewee making a class IntegerUtils I would terminate the interview then and there. \$\endgroup\$
    – Bex
    May 6, 2017 at 15:30
  • 1
    \$\begingroup\$ @PacificNW_Lover It's wrong on so many levels :). It is called Integer (so it's specific to a type, when there may be ways to make it generic), and it's called Utils, that is a generic name that is prone to grow wild when as the code evolves (and your colleagues will be tempted to create a DoubleUtils, copy pasted from IntegerUtils, and put all kind of methods related to Integers and Doubles in the two classes). \$\endgroup\$
    – baol
    May 7, 2017 at 6:37
3
\$\begingroup\$

I suggest the use of JUnit's assert methods, i.e. assertTrue, assertEquals, etc.

@Test
public void shouldBeMaxValue() {
    int maxValue = IntegerUtils.getMaxValue(array);
    Assert.assertEquals(229, maxValue);

    // or Assert.assertTrue(maxValue == 229);
}

I prefer assertEquals over assertTrue due to the better message in case of failure:

Assert.assertEquals(229, maxValue); 
// java.lang.AssertionError: expected:<229> but was:<100>

Assert.assertTrue(229 == maxValue); 
// java.lang.AssertionError

Assert.assertThat() is another (more readable) solution (and my favorite):

Assert.assertThat(maxValue, is(229));

\$\endgroup\$
2
\$\begingroup\$

The main code looks great, except for the inconsistent formatting. Let your IDE format your code for you.

To make the functions easier to use, you could replace int[] array with int... array, so that they can be called like getMinValue(1, -5).

You should add some tests for negative numbers, and for MIN_VALUE and MAX_VALUE. There should always be tests for some "normal" values, as well as tests for extreme values or special values.

Your test for the empty array must be split into two separate tests since the last line is currently not executed. When you run your tests "with coverage" (something your IDE should provide), you can see those lines of code that are not tested.

Instead of assert, you should call the method Assert.assertEquals. This way, when one of the assertions fails, you get a nice error message that includes the expected and the actual value.

If you didn't know how getMinValue is implemented, you would need additional tests in which the minimum value is at the beginning of the array, or in the middle of the array, or at the end of the array. This is because a typical implementation will initialize value with array[0] instead of MAX_VALUE and your tests have to make sure that these special cases are handled correctly.

\$\endgroup\$
4
  • \$\begingroup\$ Can you give me an example of where the formatting is inconsistent? I've setup Eclipse to give me 4 spaces for indentation and am using Sun Microsystem's conventions. If you can copy/paste a better format or describe it in a comment, I would be very grateful. Thanks for the rest of the suggestions! \$\endgroup\$ May 6, 2017 at 7:52
  • \$\begingroup\$ There is only one very small issue: i=0 without spaces. The rest already looks perfect. :) \$\endgroup\$ May 6, 2017 at 10:05
  • \$\begingroup\$ Am confused regarding your suggestion of varargs (e.g. int... array). Can you provide an example, please? \$\endgroup\$ May 7, 2017 at 1:25
  • \$\begingroup\$ What exactly did you not understand? I already provided a calling example, and you know that this is called varargs, so where is the problem? \$\endgroup\$ May 7, 2017 at 5:57
2
\$\begingroup\$

I actually combined 2 methods to become one to reduce the repetition. I would recommend to use java's stream for cleaner and elegance code.

Not very elegance solution, especially the boolean part, but i tried, just provide an alternative solution or idea to the OP on how to combine both of the methods, maybe the usage of getMinOrMax(array, "min") is better than a boolean.

 public static int getMinOrMax(int[] array, boolean isMin) {
     if (array.length <= 0) {
         throw new IllegalArgumentException("Array is empty.");
     }

     int comparedValue = array[0];
     for (int i = 1; i < array.length; i++) {

         if (isMin && array[i] < comparedValue) {
             comparedValue = array[i];
         }
         if (!isMin && array[i] > comparedValue) {
             comparedValue = array[i];
         }
     }
     return comparedValue;
 }
\$\endgroup\$
5
  • \$\begingroup\$ Is there a way to do this using Streams in Java 8? Good job, dude! \$\endgroup\$ May 6, 2017 at 9:13
  • \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. \$\endgroup\$ May 6, 2017 at 20:19
  • \$\begingroup\$ The boolean flag min/max is code smell in my opinion: MIN and MAX are two different functions. \$\endgroup\$
    – user131519
    May 6, 2017 at 22:01
  • \$\begingroup\$ Simon, I think he meant it to be an alternative solution because he combined both sets of logic it into one method... \$\endgroup\$ May 6, 2017 at 22:08
  • \$\begingroup\$ i just updated some explanation above \$\endgroup\$
    – hades
    May 7, 2017 at 5:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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