HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

IntModulus reloaded

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
reloadedintmodulusstackoverflow

Problem

As my question Modular arithmetic was hard to review and terribly long, I've added a few comments to the easier part, namely IntModulus.

The code uses Guava and Lombok. As usual, feel free to ignore my slightly deviating coding conventions.

My concerns are speed and correctness. The whole code including the test can be found on github. If someone feels like reviewing the test, I'll add it here.
IntModulus

```
import static com.google.common.base.Preconditions.checkArgument;

import lombok.Getter;

/**
* This class provides common modular arithmetic.
* Results of all methods are ints guaranteed to be non-negative and less then modulus.
*
* The method names were chosen to be as short as {@code Math.pow}.
*/
public final class IntModulus {
private IntModulus(int modulus) {
this.modulus = modulus;
}

@SuppressWarnings("boxing") public static IntModulus newModulus(int modulus) {
checkArgument(modulus>0, "Modulus must be positive, got %s", modulus);
return new IntModulus(modulus);
}

public int pow(long base, long exp) {
checkArgument(exp>=0, "Only non-negative exponents are implemented."); //TODO allow negative exponents
if (modulus==1) return 0;
if (exp==0) return 1;
return powInternal(mod(base), exp);
}

private int powInternal(int base, long exp) {
assert base>=0;
if (base0; exp>>=1) {
if ((exp&1) != 0) result = mul(result, x);
x = square(x);
}
return result;
}

private int square(int x) {
final long x2 = x;
return (int) ((x2 * x2) % modulus); // The cast is safe and the result is surely non-negative.
}

/** Return a non-negative value less than modulus and congruent to the exact product. */
public int mul(long x, long y) {
return mul(mod(x), mod(y));
}

/** Return a non-negative value less than modulus and congruent to the exact product. */
public int mul(int x, i

Solution

Note: I changed a few things to fit my own personal style, but since it's solely opinion I didn't comment on them. It's mostly reordering things or whitespace.

I'd recommend renaming mul to multiply, just because there's no particular reason to have it short.

private int square(int x) {
    final long x2 = x;
    return (int) ((x2 * x2) % modulus); // The cast is safe and the result is surely non-negative.
}


Sure, it's the second x, but xLong would be a lot clearer then x2.

Also:

// The cast is safe and the result is surely non-negative.


Why is the cast safe? Sure, after a second I can understand that it's because modulus is an int and therefore the result has to be within the bounds of int, but... okay, maybe that's a bad example. The point I'm trying to make is that when you make an assertion, you should explain why. [This space intentionally left blank]

For your RTI binary method implementation: I would suggest adding a comment explaining how it works, but the Wikipedia article does a fine enough job.

I'm gonna clarify on a point I made in my last review: Be careful with asserts. You use them pretty well here, but there are a couple of things you should always do:

-
Use the extended form -- assert condition : "why"; instead of just assert condition; and use the String to explain why failing that assert is bad. It'll make your code clearer, and when you want to debug, you won't have to struggle to remember why that assert has to pass -- it'll be right there in the error message.

-
Only use them in places where they're expected to pass, and failing it is only ever caused by a bug in the program. You're already doing this, but I still feel like it's worth mentioning.

fixMod at first seemed like an odd name -- I kept reading it as fixedModulus, but if I'm reading it right, it's "fix" as in "correct", right? I'd suggest adding a quick Javadoc to that effect, so you can remember easier when you want to change this.

For consistency, you should add long versions of add and sub as well.

WRT performace: There's not much you could do, aside from manually inlining things to shave off some readability.

Code Snippets

private int square(int x) {
    final long x2 = x;
    return (int) ((x2 * x2) % modulus); // The cast is safe and the result is surely non-negative.
}
// The cast is safe and the result is surely non-negative.

Context

StackExchange Code Review Q#95048, answer score: 4

Revisions (0)

No revisions yet.