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\$
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\$
  • 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 '17 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\$ – PacificNW_Lover May 6 '17 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 '17 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 '17 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 '17 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\$
  • \$\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\$ – PacificNW_Lover May 6 '17 at 7:52
  • \$\begingroup\$ There is only one very small issue: i=0 without spaces. The rest already looks perfect. :) \$\endgroup\$ – Roland Illig May 6 '17 at 10:05
  • \$\begingroup\$ Am confused regarding your suggestion of varargs (e.g. int... array). Can you provide an example, please? \$\endgroup\$ – PacificNW_Lover May 7 '17 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\$ – Roland Illig May 7 '17 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\$
  • \$\begingroup\$ Is there a way to do this using Streams in Java 8? Good job, dude! \$\endgroup\$ – PacificNW_Lover May 6 '17 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\$ – Simon Forsberg May 6 '17 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 '17 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\$ – PacificNW_Lover May 6 '17 at 22:08
  • \$\begingroup\$ i just updated some explanation above \$\endgroup\$ – hades May 7 '17 at 5:09

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.