17
\$\begingroup\$

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 + "."); } }
\$\endgroup\$
1

5 Answers 5

Reset to default
11
\$\begingroup\$

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.

\$\endgroup\$
0
8
\$\begingroup\$

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.

\$\endgroup\$
8
\$\begingroup\$

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.

\$\endgroup\$
9
  • \$\begingroup\$ 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? \$\endgroup\$ Commented Apr 2, 2014 at 12:30
  • 2
    \$\begingroup\$ @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. \$\endgroup\$ Commented Apr 2, 2014 at 12:33
  • \$\begingroup\$ Brilliant explanation so it can only be used when assigning Ints/Strings etc, thank you I have learnt alot today! \$\endgroup\$ Commented Apr 2, 2014 at 12:36
  • \$\begingroup\$ @NiallSzalkai keep in mind, the condition needs to have a boolean result, so no strings and ints there ;) \$\endgroup\$ Commented Apr 2, 2014 at 12:39
  • 1
    \$\begingroup\$ You mean 3 * n + 1, not 3 * n + n. Although it might be educational to see what happens with 3 * n + n. \$\endgroup\$ Commented Apr 3, 2014 at 9:36
7
\$\begingroup\$

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 + " ");
\$\endgroup\$
6
\$\begingroup\$

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

\$\endgroup\$
2
  • 3
    \$\begingroup\$ 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) \$\endgroup\$ Commented Apr 2, 2014 at 16:49
  • \$\begingroup\$ 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! \$\endgroup\$ Commented Apr 2, 2014 at 17:01

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.