Commonmark migration

#Formatting

# USE PROPER INDENTATION - Please!

###Braces

###Close Scanner

### Close Scanner

###Unnecessary outer while

### Unnecessary outer while

###Use the length of the array

### Use the length of the array

###Slight simplification

###Variables

### Variables

###Extract Method

### Extract Method

###Speed Improvement

### Speed Improvement

Simon Forsberg
• 57.9k
• 9
• 149
• 304

###Speed Improvement

Currently you're saving the result of one calculation into your array, and while that's a good start you are missing out on some values you can use.

Let's take the example of 3. The sequence is: 3, 10, 5, 16, 8, 4, 2, 1

The only result you are storing from this operation is 3 --> 7. But you have also found out that 10 has the value 7 - 1, 5 has the value 7 - 2, 16 has the value 7 - 3 and so on... Your algorithm would increase significantly in speed if you were also storing these intermediate values.

Simon Forsberg
• 57.9k
• 9
• 149
• 304

#Formatting

If you're using Eclipse, press Ctrl + Shift + F.

If you're using Netbeans or IntelliJ, press Alt + Shift + F.

If you're not using any IDE, start doing so!

###Braces

Once your code is properly formatted it can be seen that you're not using braces here:

if (j < 50000000)
arr[(int) (j - 1)] = count;


It is especially important to use braces when you're code's not formatted properly, as it is easy to accidentally add an extra statement inside the if, which would cause unexpected results.

if (j < 50000000) {
arr[(int) (j - 1)] = count;
}


###Close Scanner

Scanners should be closed when they have been used. sc.close(); at the end of your program will take care of that.

###Unnecessary outer while

Your outermost while "loop" is only executed once because there is a break; at the end of it. This could be an if instead and the break; at the end removed.

###Use the length of the array

if (j < 50000000) {


and

if (n < 50000000) {


if (j < arr.length) {


and

if (n < arr.length) {


respectively.

###Slight simplification

if (n % 2 == 0) {
n = n / 2;
count++;
}
else {
n = (3 * n) + 1;
count++;
}


Can be written as:

n = (n % 2 == 0 ? n / 2 : (3 * n) + 1);
count++;


Or at least, if you don't like the conditional operator (... ? ... : ...), use the if-else for n but do count++ before or after the if/else.

###Variables

Your naming leaves some things to be desired. x and y for example should be named startTime and endTime, the current names makes it sound like it is related to coordinates, which it is not.

The only variable with any meaning is max and count. It is hard to tell what kind of max max is though, naming it maxCycles would make that clearer.

As for your other variables, long i, j, n, pos, I certainly cannot tell what they're used for without looking at your code for a while. These variable names tell me absolutely nothing!

To get a better overlook of the variables, it is better to declare the variables as close to their usage as possible. y (endTime) is declared at the top, but not used until the end of the method.

###Extract Method

Your main method is doing way too many things right now.

Extract parts of your code to different methods:

For example, your entire while (n != 1) loop can be extracted to a determineCycles method.