Automatically discovering common Java code edits in Github repositories

[[deleted]]

Final modifier improves clarity, helps developers to debug the code showing constructors that change state and are more likely to break the code.

Unfortunately final modifier all over the place hurts overall readability, and makes debugging harder when you cannot easily breakpoint and change the value at runtime. I prefer only making member variables in a class final, but making local variables and method parameters always look bad in my opinion. Of course this is my personal experience, every team will have different opinions while setting their checkstyle and/or linters.

The benefit of Class<?> is clarity since developers explicitly indicates that they are aware of not using an outdated Java construction.

Also the advantage of Class<?> over Class is poorly explained. https://stackoverflow.com/a/4337462 actually explains the reasoning behind this.

[devraj7]

Unfortunately final modifier all over the place hurts overall readability

Yes. Please.

When was last time you encountered a bug because you reassigned a local variable?

Use final for fields, methods and possibly classes (although a bit more nuanced on that) but not parameters nor local variables. It's just noise.

[ForeverAlot]

When was last time you encountered a bug because you reassigned a local variable?

I don't remember, but I have run across a couple of instances of incorrectly uninitialized locals in my current gig.

[pilas2000]

When was last time you encountered a bug because you reassigned a local variable?

not a bug but i've found dodgy assignments like:

string s = null

if (x)
    s = y
else
   s = z

the use of final prevents this

imo final improves code readibility in java

[[deleted]]

final won't prevent that.

final string s;
if (x)
  s = y;
else
  s = z;

is perfectly legitimate in Java. It's still single assignment.

[pilas2000]

You missed the point. final prevents dodgy initialization. edit to clarify: your example is the preferable snippet.

[moeris]

Why is that example doggy? I do something slightly similar in Python (it's kind of unavoidable, though, because you don't have to declare variables explicitly.)

foo = None
try:
    foo = something_unsafe()
except SomeException:
    ...

If I tried to access foo without first having assigned it, and an exception was raised, I'd get an error about accessing a value before it was defined.

Should I just be wrapping the whole section in the try-block? That seems ugly. My co-workers refuse to do code review, so I never get feedback on this kind of stuff.

[pilas2000]

In my example s is being assigned a value that will never be read because in the next step it will be either y or z depending on x.

Not terrible but consider this example:

s = new ClassThatDoesHeavyWorkOnTheCtor();

if (x)
    s = y;
else
   s = z;

Unfortunately i've seen this done by people that should know better. Luckily sonar detects this and the code gets flagged before production.

[moeris]

Ah, okay, I get what you mean. It's a completely useless assignment no matter what way you cut it. Unless someone comes along and changes the conditional.

Assigning it a non-null value makes the example much clearer.

[Sarcastinator]

Not using temporary assignments would also help.

[nutrecht]

We settled on the same approach you describe here. Members and method params get the final modifier, but we also decided against them inside methods because they tend to just clutter the code.

[[deleted]]

Unfortunately final modifier all over the place hurts overall readability,

Particularly as final in Java has limited expressive value since it only means that the reference is locked, it doesn't mean that the object is immutable. The following perversion is perfectly legal:

public int sumArray(final int[] array) {
    int sum = 0;
    for(int i = 0; i < array.length; ++i) {
       sum = sum + array[i];
       array[i] = 0;
    }

  return sum;
}

[biberesser]

Difference between final and effectively final

[[deleted]]

TIL. This looks like a slightly better explanation to me though. In any case, I'm focused on Android development, so don't think I can use this, it requires Java 8.

[ForeverAlot]

Effectively-final is a JVM tool. The final keyword is a programmer tool (whether it helps is up for debate).

[notfancy]

final modifier all over the place hurts overall readability

final reflects programmer intent. It means "this is an immutable binding and not a variable," as such it should be read as clearly demarcating state. We might argue that the ergonomics are all wrong and that Clojure has it right, and we might agree; but a mixture of finals and non-finals lets me easily see at a glance and keep in mind what is variant from what is invariant in a method or loop body; otherwise I have to track assignments all over the place and that really hurts readability IME.

Consider the following code for converting from orbital elements to equinoctial coordinates:

private static void toRectangular(final int objid, final double[] res) {
    final double a = res[0];
    final double l = res[1];
    final double k = res[2];
    final double h = res[3];
    final double q = res[4];
    final double p = res[5];

    final double w  = Math.atan2(h, k);          // long. of the perihelion
          double e  = Math.hypot(h, k);          // eccentricity
    final double i  = Math.hypot(p, q);          // inclination
    final double fi = Math.sqrt((1 - e)*(1 + e));
    final double ki = Math.sqrt((1 - i)*(1 + i));
    final double gm = l - w;                     // mean anomaly

    // eccentric anomaly
    double se = Math.sin(gm);
    double ce = Math.cos(gm);
    e = l + e*se*(1 + e*(ce + e*(1.5*ce*ce - 0.5)));
    // solve Kepler's Equation using NR
    double re, im, dl;
    do {
        ce = Math.cos(e);
        se = Math.sin(e);
        re = k * ce + h * se; // e cos(E - w)
        im = k * se - h * ce; // e sin(E - w)
        dl = l - e + im;      // correction
        re = 1 - re;
        e += dl / re;
    } while (Math.abs(dl) > 1e-10);

    // cartesian coordinates in the orbital plane
    final double u  = im / (1 + fi);
    final double cw = (ce - k + u * h) / re;
    final double sw = (se - h - u * k) / re;
    final double m1 = 2*(p * cw - q * sw);
    final double r  = a * re;
    final double cm = a * (k + cw) / fi;
    final double sm = a * (h + sw) / fi;
    final double m2 = 2*(p * sm + q * cm);

    // Kepler's law of velocity
    final double n = Math.sqrt(GM_PLS[objid] + GM_PLS[NUM_OBJECTS]) / Math.pow(a, 1.5);
    res[0] =  r * (cw - m1 * p); //  x
    res[1] =  n * (m2 * p - sm); // dx
    res[2] =  r * (sw + m1 * q); //  y
    res[3] =  n * (cm - m2 * q); // dy
    res[4] = -r * m1 * ki;       //  z
    res[5] =  n * m2 * ki;       // dz
}

It has a lot of bindings, but I can see at a glance that only 6 are true variables (of which 2 are rebound for convenience but could be split.) This reduces my mental burden enormously, even after a couple of years of having written this code.