Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 + ".");
    }
}
share|improve this question

5 Answers 5

up vote 10 down vote accepted

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
    
Thank you this seems really efficient, this may seem a silly question but the n = (n % 2 == 0) ? n / 2 : n * 3 + n; line includes a question mark, what does that do? Although using this method seems to save alot of code, what is better for readability? If/Else or Ternary? - seen edit, the question mark separates the condition and the If/Else? –  NiallSzalkai Apr 2 at 12:30
2  
@Niall You can only use the Ternary operator for assignment operations (Afaik). In fact I personally love using it everywhere possible, but I think it depends on personal style, where you want it and where not. the ? is just the syntax requirement. I added a little explanation on that. –  Vogel612 Apr 2 at 12:33
    
Brilliant explanation so it can only be used when assigning Ints/Strings etc, thank you I have learnt alot today! –  NiallSzalkai Apr 2 at 12:36
    
@NiallSzalkai keep in mind, the condition needs to have a boolean result, so no strings and ints there ;) –  Vogel612 Apr 2 at 12:39
1  
You mean 3 * n + 1, not 3 * n + n. Although it might be educational to see what happens with 3 * n + n. –  gnasher729 Apr 3 at 9:36

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 + " ");
share|improve this answer

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: http://stackoverflow.com/questions/8941298/system-out-closed-can-i-reopen-it

share|improve this answer
    
Thank you very much sir, good information :) –  NiallSzalkai Apr 2 at 12:20
3  
Note that this will close System.in, so if you need it later it could cause some trouble. (This is just a warning, nothing wrong here) –  Marc-Andre Apr 2 at 16:49
    
Thanks for highlighting this! I didn't know, that you really can close the System.in stream, since you're also not opening it in any way! –  bobbel Apr 2 at 17:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.