Collatz Sequence

Question

I'm a beginner and I only know limited amounts such as if, while, do while. So I'm here to check if I'm coding with best practise and the most effective methods to my current knowledge.

import java.util.Scanner;


public class SixtyTwo {

    public static void main(String[] args) {

            Scanner keyboard = new Scanner(System.in);

            System.out.print("Starting Number: ");
            int n = keyboard.nextInt();
            int counter = 0;
            int stepsTaken = 0;
            int largestNumber = 0;
            System.out.println();

            while ( n != 1 ){
                    if ( ( n & 1 ) == 0 ) {
                            System.out.print( (n = ( n / 2 )) + " " );
                            stepsTaken++;
                            counter++;
                    }       else {
                            System.out.print( (n = ( n * 3 ) + 1) + " " );
                            stepsTaken++;
                            counter++;
                    }

                    if ( n > largestNumber ){
                            largestNumber = n;
                    }

                    if (counter == 9){
                            counter = 0;
                            System.out.print("\n");
                    }
            }

            System.out.println();
            System.out.println("\nTerminated after " + stepsTaken + " steps.");
            System.out.println("The largest value was " + largestNumber + ".");
    }
}

Answer 1

That is some overall quite good code you have there, but I have a couple of comments.

• Scanner should be closed when you are done with the input. Simply call keyboard.close(); when you have acquired the input that you need.

• Your class is called SixtyTwo but I have no clue what that has to do with anything. A better name would be Collatz

• if ( ( n & 1 ) == 0 ) although it is a nice way of checking if a number is even, it is for Java programmers more readable to use the % (modulo) operator. I would use if (n % 2 == 0).

• System.out.print( (n = ( n / 2 )) + " " ); ...now this I'm not a big fan of. Sure, it works to both modify a value and outputting it on the same line, but I would separate the assignment to it's own line. Assign on one line, output on another, makes things much clearer to read.

• stepsTaken++; and counter++; don't need to be inside the if-else blocks, put them outside them as they are always done no matter if your condition is true or false.

• if (counter == 9) can instead be if (stepsTaken % 9 == 0), which would remove the need for the counter variable entirely.

Answer 2

Now since the whole point of this code is to check the Collatz conjecture (which says that whatever integer you start with, you'll eventually end up at 1, and you won't get a repeating cycle or numbers growing more and more), could I suggest that you add a check that the number isn't getting too big. Because if there is a number where the sequence grows and grows then you would get an overflow, and from then on get the wrong results in the sequence.

Typically you will get wrong results if n is odd and 3n + 1 > 2^31 - 1, or n > 715,827,882. I bet you don't need to search very far to find a number where that limit gets exceeded. Without such a check, the whole code is pointless.

Answer 3

You can greatly reduce the complexity of your code, by using the ternary operator:

while (n != 1){
   stepsTaken++;
   n = (n % 2 == 0) ? n / 2 : n * 3 + 1;
   System.out.print(n);

   //Your largest number and the linebreaks go here;
}

I removed the counter++ as you correctly mentioned, you can instead of counter == 9 use stepsTaken % 9 == 0

Small explanation:

this:

int i = {some value};
if(i % 2 == 0){
   i = i / 2;
}
else {
   i = i * 3 + 1;
}

can be shortened to the terary operator. It has the following syntax:

i = (condition) ? if-branch : else-branch;

That is the whole magic behind the shortened code.

Answer 4

Indenting and spacing
Java's indentation conventions are no more than four spaces in a block. Also don't overdo spacing inside your parentheses and between braces. See here for more conventions:

while (n != 1){
    if (( n & 1 ) == 0) {
        System.out.print((n = (n / 2)) + " ");
        stepsTaken++;
        counter++;
    } else {
        System.out.print((n = (n * 3) + 1) + " ");
        stepsTaken++;
        counter++;
    }

    if (n > largestNumber) {
        largestNumber = n;
    }

    if (counter == 9) {
        counter = 0;
        System.out.print("\n");
    }
}

Don't change where you print
You have lines like this in your code:

System.out.print( (n = ( n / 2 )) + " " );

This is a very problematic line of code, since it logs as well as changes the value of n. The change is also not as obvious as you think, and might easily be missed. This causes two problems:

1. Someone reading your code might not understand how it works, since it missed the point where you change the value.
2. More seriously - if you decide to remove the printing, you might forget that it changes something, and simply comment it out or delete it.

I suggest you re-write it into to separate lines - one for the change and one for the log:

n = n / 2;
System.out.print(n + " ");

Answer 5

Since Java 7 it is also possible to use try-with-resource statements which closes the Scanner object automatically:

try (Scanner keyboard = new Scanner(System.in)) {
   // doing stuff with keyboard
}

This prevents also exception handling, because the stream will be closed in any case (like a finally block).

As Marc-Andre pointed out, you should be aware that the System.in stream is also closed after the try-block. So you will not be able to scan more input from the user, because you can't reopen this stream!

Also see: https://stackoverflow.com/questions/8941298/system-out-closed-can-i-reopen-it