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

Collatz conjecture using Java lambdas

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

Problem

Take any natural number n. If n is even, divide it by 2 to get n / 2. If n is odd, multiply it by 3 and add 1 to obtain 3n + 1. Repeat the process (which has been called "Half Or Triple Plus One", or HOTPO) indefinitely. The conjecture is that no matter what number you start with, you will always eventually reach 1.


(Wikipedia article)

  • Any suggestions to make it more Java-8-like?



  • Please review the Javadocs and the overall readability of code+test too.



One disclaimer to add first: I know AtomicLong c = ... variable name is not ideal, the only reason why I am using a single character here is to make the code formatting look slightly better in my IDE code formatting settings. Feel free to comment on everything else.

CollatzUtils

import java.util.concurrent.atomic.AtomicLong;
import java.util.stream.LongStream;

/**
 * Utilities class for deriving number of steps using the Collatz conjecture.
 * 
 * Given a number n:
 * 
 * If n is even, repeat for value n / 2.
 * If n is odd, repeat for value 3 * n + 1.
 * Repeat indefinitely, the conjecture is that n = 1 eventually.
 * 
 */
public final class CollatzUtils {

    /**
     * Private constructor for utility class.
     */
    private CollatzUtils() {
        // intentionally blank
    }

    /**
     * Applies the Collatz conjecture on value, and count the steps to reach 1.
     * 
     * Examples:
     * 
     * 1: 1 → 4 → 2 → 1. Therefore, the return value is 3.
     * 2: 2 → 1. Therefore, the return value is 1.
     * 
     *
     * @param value the value to start from.
     * @return the number of steps to end at 1.
     * @throws IllegalArgumentException if value is 0 or less.
     */
    public static long stepsFor(long value) {
        if (value  c.incrementAndGet() > 0 && v % 2 == 0 ? v / 2 : 3 * v + 1)
                .anyMatch(v -> c.longValue() > 1 && v == 1);
        return c.decrementAndGet();
    }

}


CollatzUtilsTest

```
import static org.hamcrest.MatcherAssert.assertThat;

Solution

I wouldn't obfuscate to →. Google Java Style says


Never make your code less readable simply out of fear that some programs might not handle non-ASCII characters properly. If that should happen, those programs are broken and they must be fixed.

and I can only agree.

I'm rather unfamiliar with Java 8, but

final AtomicLong c = new AtomicLong();
    LongStream.iterate(value, (v) -> c.incrementAndGet() > 0 && v % 2 == 0 ? v / 2 : 3 * v + 1)
            .anyMatch(v -> c.longValue() > 1 && v == 1);


looks pretty weird. It's not really functional, because of the side effects and it took me a while to grasp.

Using AtomicLong as a plain long holder is wrong. It's confusing (what do you need atomicity for?) and it's slow on a multicore CPU as it does quite some needles work. A plain new long[1] is way better.

While rolfl showed us, how to get the streams right, I'd argue that it should not be done at all. While functional programming is cool and this is a nice exercise, the lesson learned should be that there are problems which can be solved much simpler in a different way.

public static long stepsFor(long value) {
    checkArgument(value > 0, "Input must be greater than 0.");
    for (int i=1; ; ++i) {
        value = (value & 1) == 0 ? value >> 1 : 3*value + 1;
        if (value==1) return i;
    }
}


Untested, possibly off-by-one, but who cares... my points are:

  • use Preconditions or whatever simplifies the checks



  • always look for simpler alternatives



  • don't do the optimizations I did, as it may get unclear and error-prone ;)

Code Snippets

final AtomicLong c = new AtomicLong();
    LongStream.iterate(value, (v) -> c.incrementAndGet() > 0 && v % 2 == 0 ? v / 2 : 3 * v + 1)
            .anyMatch(v -> c.longValue() > 1 && v == 1);
public static long stepsFor(long value) {
    checkArgument(value > 0, "Input must be greater than 0.");
    for (int i=1; ; ++i) {
        value = (value & 1) == 0 ? value >> 1 : 3*value + 1;
        if (value==1) return i;
    }
}

Context

StackExchange Code Review Q#63766, answer score: 10

Revisions (0)

No revisions yet.