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

Array whose values are the product of every other integer

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

Problem

I was asked the following interview question over the phone:


Given an array of integers, produce an array whose values are the
product of every other integer excluding the current index.


Example:

[4, 3, 2, 8] -> [3*2*8, 4*2*8, 4*3*8, 4*3*2] -> [48, 64, 96, 24]


and I gave the following answer

```
import java.math.BigInteger;
import java.util.Arrays;

public class ProductOfAnArray {

public static void main(String[] args) {

try {
System.out.println(Arrays.toString(ProductOfAnArray
.calcArray(new int[] { 4, 3, 2, 8 })));
System.out.println(Arrays.toString(ProductOfAnArray
.calcArray(new int[] { 4, 0, 2, 8 })));
System.out.println(Arrays.toString(ProductOfAnArray
.calcArray(new int[] { 4, 0, 2, 0 })));
System.out.println(Arrays.toString(ProductOfAnArray
.calcArray(new int[] {})));
System.out
.println(Arrays.toString(ProductOfAnArray
.calcArray(new int[] { 4, 3, 2, 8, 3, 2, 4, 6, 7,
3, 2, 4 })));
System.out
.println(Arrays.toString(ProductOfAnArray
.calcArray(new int[] { 4, 3, 2, 8, 3, 2, 4, 6, 7,
3, 2, 4 })));
System.out.println(Arrays.toString(ProductOfAnArray
.calcArray(new int[] { 4432432, 23423423, 34234, 23423428,
4324243, 24232, 2342344, 64234234, 4324247,
4234233, 234422, 234244 })));
} catch (Exception e) {
// debug exception here and log.
}
}

/*
* Problem: Given an array of integers, produce an array whose values are
* the product of every other integer excluding the current index.
*
* Assumptions. Input array cannot be modified. input is an integer array
* "prod

Solution

For starters, you have very weird line breaks:

System.out
        .println(Arrays.toString(ProductOfAnArray
                .calcArray(new int[] { 4, 3, 2, 8, 3, 2, 4, 6, 7,
                        3, 2, 4 })));


That's not layout, that's obfuscation! When dealing with long method chains, it may be preferable to break before each method invocation:

someObject
  .thisIs()
  .aFluid()
  .interface();


but that's not the case here, and this layout does not increase readability. Instead, break after the opening paren of the argument list:

System.out.println(
    Arrays.toString(ProductOfAnArray.calcArray(new int[] {
        4, 3, 2, 8, 3, 2, 4, 6, 7, 3, 2, 4
    }))
);


or something like that. Note that you could have written this as

System.out.println(
    Arrays.toString(ProductOfAnArray.calcArray(
        4, 3, 2, 8, 3, 2, 4, 6, 7, 3, 2, 4
    ))
);


(i.e. without the explicit array) when declaring your function with varargs: calcArray(int... inp).

The next question is why you are returning an array – a List would be more flexible – arrays have little use in modern Java (the usage of varargs implies arrays, and high-performance code may need them. But in all other cases, they are too inflexible – e.g. arrays and generics don't work together).

But why BigInteger? Yes, you don't want to overflow, but we can assume that your interviewers would be content with an int. If you want to allow larger numbers, the first step would be long. Big Integers are truly awkward to use and necessarily less performant that native types, so I would avoid them wherever possible.

Your code inside calcArray is unnecessarily clever. Let's look at this loop:

for (int i : inp) {
    if (i == 0) {
        cnt++;
        foundZero = true;
        if (cnt < 2)
            continue;
        else
            break;
    }
    multiple = multiple.multiply(BigInteger.valueOf(i));
}


Notice that foundZero is equivalent to cnt > 0, and that you aren't using braces around the bodies of the inner if/else. Your continue is rather confusing, and the following expresses the control flow much better:

for (int i : inp) {
    if (i != 0) {
        multiple = multiple.multiply(BigInteger.valueOf(i));
    }
    else {
        cnt++;
        if (cnt >= 2) {
            break;
        }
    }
}


This conditional in the next loop is also too complex, and could be simplified to

if (cnt < 2 && inp[i] == 0) {
    ans[i] = multiple;
} else {
    ans[i] = new BigInteger("0");
}


When applying these criticisms, the Simplest Thing That Could Work might be this:

public static List specialProduct(int... numbers) {
    long product = 1;
    int zeroesCount = 0;
    for (int n : numbers) {
        if (n != 0) {
            product *= n;
        }
        else {
            zeroesCount++;
        }
    }

    List output = new ArrayList<>(numbers.length);

    switch (zeroesCount) {
    case 0:
        // with no zeroes, we don't have to perform any checks
        for (int n : numbers) {
            output.add(product / n);
        }
        break;
    case 1:
        // with one zero, the all others will become zero too
        for (int n : numbers) {
            if (n == 0) {
                output.add(product);
            }
            else {
                output.add(0L);
            }
        }
        break;
    default:
        // two or more zeroes make everything zero
        for (int i = 0; i < numbers.length; i++) {
            output.add(0L);
        }
        break;
    }

    return output;
}


Oh, and naming! Have vocals become so precious that we must write cnt instead of count? Must we write ans instead of answer if you can actually write a long word like multiple? And what the heck is inp?

Your code is not bad, but it isn't good, and could certainly have been written better. The next time, maybe :)

Code Snippets

System.out
        .println(Arrays.toString(ProductOfAnArray
                .calcArray(new int[] { 4, 3, 2, 8, 3, 2, 4, 6, 7,
                        3, 2, 4 })));
someObject
  .thisIs()
  .aFluid()
  .interface();
System.out.println(
    Arrays.toString(ProductOfAnArray.calcArray(new int[] {
        4, 3, 2, 8, 3, 2, 4, 6, 7, 3, 2, 4
    }))
);
System.out.println(
    Arrays.toString(ProductOfAnArray.calcArray(
        4, 3, 2, 8, 3, 2, 4, 6, 7, 3, 2, 4
    ))
);
for (int i : inp) {
    if (i == 0) {
        cnt++;
        foundZero = true;
        if (cnt < 2)
            continue;
        else
            break;
    }
    multiple = multiple.multiply(BigInteger.valueOf(i));
}

Context

StackExchange Code Review Q#45498, answer score: 17

Revisions (0)

No revisions yet.