Return to Answer

• 1

• 25.3k
• 3
• 31
• 76

Bugs

BigInteger i2 = i;
i2.add(two);

This isn't right. If BigInteger were mutable, you would still change i as well. But since it isn't, you are changing neither. It should be:

BigInteger i2 = i.add(two);

The same problem here:

for (BigInteger i = five; m.compareTo(i) > -1; i.add(six)) {

For me, your program never terminates, and this is the reason why. See here for how to use BigInteger and loops. It should be:

for (BigInteger i = five; m.compareTo(i) > -1; i = i.add(six)) {

Other Improvements

What would be the best way to improve this?

Right now, I would focus on readability, and after that is taken care of, look at the performance.

Contradicting Exception Message

if (x < 2) throw new IllegalArgumentException("x must be greater then 2.");

The check and the message of this exception to not match. It should actually say that x must be greater than 1.

Static BigInteger

There is no need to create a new object every time you call a method. It makes the code slower and harder to read. Just declare all the constant BigInteger you need once:

private static BigInteger two = new BigInteger("2");
private static BigInteger three = new BigInteger("3");
private static BigInteger five = new BigInteger("5");
private static BigInteger six = new BigInteger("6");

BigInteger zero and one

BigInteger actually has a constant for zero, no need to create an object for it:

test = (lucas.compareTo(BigDecimal.ZERO) == 0);

It also has a constant for one, so you should also get rid of all the new BigInteger("1") (again, for readability and performance).

General Style

It will make your code a lot more readable if you follow general style guidelines:

• capitalize class names (calculate -> Calculate)
• function names should start with a lower case letter (MPtest -> mpTest, or better yet: mersennePrimeTest or isMersennePrime)
• before and after operations and assignments should be a space (for example test= -> test = , x-2 -> x - 2, lucas=lucas -> lucas = lucas, etc)
• no semicolon after curly brackets (}; -> }

Variable Naming

• what's a lucas? lucas lehmer residue? In that case I would actually name it lucasLehmerResidue. It's a bit longer, but also clearer
• if you rename your function to isMersennePrime, mprime would be fine, but it could also be mersennePrime
• argument names: is there a reason that sometimes the argument is named n and sometimes x? If you cannot think of more expressive names, I would at least use the same
• short variable names: nc, i2, and m don't really express what they do
• loop variables: if there is no good reason for it, I wouldn't use y, but i

Unused functions

Why do you have MPtest when you never use it?

Unnecessary Variables

I don't see why you need nc. I would just remove it.

Unnecessary Brackets

In your return statements, you don't really need brackets. Instead of return (n==2); just write return n==2;.

Curly Brackets

Always use curly brackets, even for one-line statements, it makes code easier to read and avoids bugs.

Comments

You need more comments. Your class should have one saying what algorithm you use, as should your functions.

In my opinion, code like this:

 if ((x & 0xfff0000000000000L) == 0L) return (int) StrictMath.sqrt(x);
 final long result = (long) StrictMath.sqrt(2.0d*(x >>> 1));
 return result*result - x > 0L ? (int) result - 1 : (int) result;

would also deserve a comment or two.