Find all local maxima in a one-dimensional array I implemented the following class (it works) and I just want to hear your opinions about best practices and what can be improved. The idea was to do it using mostly arrays.

The class just finds the peak elements in an array with respect to their neighbors. The first element and the last element are ignored.

Example: {3, 15, 5, 20, 10}
Answer:  {15, 20}

package findpeak;

import java.util.ArrayList;
import java.util.List;


public class ArrayPeak
{
    private final int[] ARRAY;
    private final int LEN;
    private static final int IGNORE_FIRST_POSITION = 0;
    private final int IGNORE_LAST_POSITION;

    public ArrayPeak(int[] array)
    {
        this.ARRAY = array;
        this.LEN = this.ARRAY.length;
        this.IGNORE_LAST_POSITION = this.LEN - 1;
    }

    public List&lt;Integer&gt; findPeaks()
    {
        List&lt;Integer&gt; peaks = new ArrayList&lt;&gt;();
        for (int position = 0; position &lt; LEN; position++)
        {
            int output = 0;
            if (position != IGNORE_FIRST_POSITION &amp;&amp; position != IGNORE_LAST_POSITION)
            {
                int[] trippleArray =
                {
                    ARRAY[position - 1], ARRAY[position], ARRAY[position + 1]
                };
                output = checkPeak(trippleArray);
            }
            peaks.add(output);
        }

        return filterZeros(peaks);
    }

    private static int checkPeak(int[] inputArray)
    {
        int peak = 0;
        if (inputArray[1] &gt; inputArray[0] &amp;&amp; inputArray[1] &gt; inputArray[2])
        {
            peak = inputArray[1];
        }
        return peak;
    }

    private static List&lt;Integer&gt; filterZeros(List&lt;Integer&gt; list)
    {
        List&lt;Integer&gt; filteredList = new ArrayList&lt;&gt;();
        for (int i = 0; i &lt; list.size(); i++)
        {
            if (list.get(i) != 0)
            {
                filteredList.add(list.get(i));
            }
        }
        return filteredList;
    }
}
This shouldn't involve an instance of a class: you can (and should) implement this as a standalone static method, which takes the input array as a parameter and returns the peaks. Introducing a class to record the state is just introducing complexity which will make the code harder to maintain without providing any benefit.

I would also like some documentation explaining exactly what the method does: for example, it doesn't return non-positive peaks, and (as Toby Speight's comment indicates) some might thing it would return plateaus. Basically you need a better specification, and that needs to appear as documentation, so that the intention of the code is completely unambiguous (for the benefit of the implementer, maintainer, and consumer). I would not expect a method like this to return the heights of peaks, rather than their position.



IGNORE_FIRST_POSITION sounds like the name of a boolean, not an int. You'd be better off, I think, changing the for loop to start at 1, end at array.Length - 1, and include a comment explaining why this is the case.



I like that CheckPeak is its own method because it puts the corresponding logic 'all in one place', but I don't like that you are creating a new array every time you check for a peak: I would much prefer you pass the array and candidate position as parameters, and it checks 'in-place'. This is a simpler API (the current one lacks any specification that it takes an array of three parameters), and will reduce the overhead from calling the method.

I also don't like that it returns the height of the peak, rather than whether or not a peak appears: this too is lacking documentation (though granted it is private, so it isn't as important as the public method). Much better to return true or false if it is a peak, then add the peak to the list of peaks if it returns true, otherwise do not (then you can remote the confusing filterZeros at the end).

This can also remove the unpleased output variable, which is defined too early. It only has meaning once it is assigned, and if it isn't assigned it just has a default value you will filter anyway, so you might as well declare and assign it in one.



Really tiny thing that is almost completely subjective: I'd prefer to write

if (inputArray[1] &gt; inputArray[0] &amp;&amp; inputArray[1] &gt; inputArray[2])


as

if (inputArray[0] &lt; inputArray[1] &amp;&amp; inputArray[1] &gt; inputArray[2])


It keeps them 'in order, and makes it a little easier to feel what is going on.



Your code only works with Integers: if I wanted to find peaks in a Double[], I would need to write a new class. You might want to make it generic on a type T that implements Comparable&lt;T&gt;. This can create some confusion with floating point types (i.e. NaNs), but by depending on a basic interface, your code will be reusable and harder to 'get wrong'.

You might also consider taking an abstract collection as a parameter; it's good that you return List&lt;T&gt; rathern than ArrayList&lt;T&gt;.



Example rewrite (disclaimer: untested, and I don't know Java, and I'm running out of time...):

public class ArrayPeak
{
    public static &lt;T extends Comparable&lt;T&gt;&gt; List&lt;T&gt; findPeaks(T[] array)
    {
        List&lt;T&gt; peaks = new ArrayList&lt;&gt;();

        // ignore first and last elements
        for (int position = 1; position &lt; array.length - 1; position++)
        {
            if (checkPeak(array, position)
            {
                peaks.add(array[position]);
            }
        }

        return peaks;
    }

    private static &lt;T extends Comparable&lt;T&gt;&gt; int checkPeak(T[] array, int position)
    {
        if (inputArray[position - 1].compareTo(inputArray[position]) &lt; 0 &amp;&amp; inputArray[position].compareTo(inputArray[position + 1]) &gt; 0)
        {
            return true;
        }
        else
        {
            return false;
        }
    }
}