For me, your program never terminates, and this is the reason why. See here for how to use BigInteger
and loops. See here for how to use BigInteger
and loops. It should be:
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 me, your program never terminates, and this is the reason why. See here for how to use BigInteger
and loops. It should be:
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
orisMersennePrime
) - 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 bemersennePrime
- argument names: is there a reason that sometimes the argument is named
n
and sometimesx
? If you cannot think of more expressive names, I would at least use the same - short variable names:
nc
,i2
, andm
don't really express what they do - loop variables: if there is no good reason for it, I wouldn't use
y
, buti
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.