HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Find out the 2 numbers which gives the highest product

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thenumbersproductgivesfindwhichhighestout

Problem

Input given is an int array, which may or may not contain positive, negative or zero values. Write a
program to find out the two numbers which gives the highest product.
(Both numbers should be positive or negative.)

Below is my code whose time complexity is O(n2), I believe. Is there any improvement I can make to the code?

public class FindLargestProduct{

    public static void main(String[] args) {
        int[] arr = { 52, 12, 34, 10, 6, 40, 0, 12, 40, 52, -56, -78, -99, 68 };
        int firstMax = 0, secondMax = 0;
        int firstNegMax = 0, secondNegMax = 0;

        for (int i = 0; i  0) {
                if (arr[i] > firstMax) {
                    secondMax = firstMax;
                    firstMax = arr[i];
                } else if (arr[i] > secondMax) {
                    secondMax = arr[i];
                }
            }

            if (arr[i]  firstNegMax) {
                    secondNegMax = firstNegMax;
                    firstMax = arr[i];
                } else if (arr[i] > secondNegMax) {
                    secondNegMax = arr[i];
                }

            }
        }

        int proPositive = firstMax * secondMax;
        int proNegative = firstNegMax * secondNegMax;

        if (proPositive > proNegative) {
            System.out.println(proPositive);
        } else {
            System.out.println(proNegative);
        }
    }
}

Solution

Bugs

Your negative cases are wrong.

if (arr[i] < 0) {
            secondNegMax = firstNegMax;
            firstNegMax = arr[i];


Here you always overwrite the current maximums...

if (arr[i] > firstNegMax) {
                secondNegMax = firstNegMax;
                firstMax = arr[i];
            } else if (arr[i] > secondNegMax) {
                secondNegMax = arr[i];
            }


And here you check if a certain value is HIGHER than the max (which is negative, so 0 is higher than all negatives!). Neither of this is going to result in a good end result.

Aside from that, there seems to be no explicit check for verifying that you have at least an array of length 2 and that the output consists of either "two positive numbers" or "two negative numbers". Other than that, it's working fine.

Algorithm

I've got only one algorithmic improvement, and that's this one...

if (arr[i] > 0) {
            if (arr[i] > firstMax) {
                secondMax = firstMax;
                firstMax = arr[i];
            } else if (arr[i] > secondMax) {
                secondMax = arr[i];
            }
        }

        if (arr[i]  firstNegMax) {
                secondNegMax = firstNegMax;
                firstMax = arr[i];
            } else if (arr[i] > secondNegMax) {
                secondNegMax = arr[i];
            }

        }


Here, you first check if the current element is positive... and then, later, you check if it's negative. Now, there's nothing wrong with that, but if you know that this element is positive, then there is no need for the negative check anymore. Use else-if here.

Naming

Your shorthand should be expanded, firstNegativeMax reads better than firstNegMax. max is well understood as "maximum", neg isn't as well understood. Aside from that, assigning arr[i] to a local variable with a name like "currentNumber" or "currentElement" would help readability. (Better yet, follow Martin R's advice and make it a for-each loop.)

Code Snippets

if (arr[i] < 0) {
            secondNegMax = firstNegMax;
            firstNegMax = arr[i];
if (arr[i] > firstNegMax) {
                secondNegMax = firstNegMax;
                firstMax = arr[i];
            } else if (arr[i] > secondNegMax) {
                secondNegMax = arr[i];
            }
if (arr[i] > 0) {
            if (arr[i] > firstMax) {
                secondMax = firstMax;
                firstMax = arr[i];
            } else if (arr[i] > secondMax) {
                secondMax = arr[i];
            }
        }

        if (arr[i] < 0) {
            secondNegMax = firstNegMax;
            firstNegMax = arr[i];
            if (arr[i] > firstNegMax) {
                secondNegMax = firstNegMax;
                firstMax = arr[i];
            } else if (arr[i] > secondNegMax) {
                secondNegMax = arr[i];
            }

        }

Context

StackExchange Code Review Q#138217, answer score: 7

Revisions (0)

No revisions yet.