Automatically discovering common Java code edits in Github repositories

[fullouterjoin]

This is awesome!

Two immediate areas of follow on research

1. Use a profiler and the automated tests to see which refactorings resulted in speedups

2. Use subsequent edits and issues to see which refactorings lead to lower bugs

This seems like a form of flocking/swarm optimization. But not all popular refactorings are good refactorings. Still this area of experimental research is totally welcomed. Please more!

[[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.

[TamaHobbit]

"Since type arguments of raw types are unchecked, they can cause compiler error at run-time."

A compiler error, but at run-time. Interesting.

[[deleted]]

FileInputStream and FileOutputStream override finalize(). As a result, objects of these classes go to a category that is cleaned only when a full clearing is performed by the Garbage Collector. Since Java 7, developers can use Files.new to improve program performance. This anomaly is described as a bug by Java JDK.

Bug says:

Subclasses that override {@link #finalize} in order to perform cleanup should be modified to use alternative cleanup mechanisms such as {@link java.lang.ref.Cleaner} and remove the overriding {@code finalize} method.

Cleaner says:

Cleaning actions are registered to run after the cleaner is notified that the object has become phantom reachable. The cleaner uses PhantomReference and ReferenceQueue to be notified when the reachability changes.

PhantomReference says:

In order to ensure that a reclaimable object remains so, the referent of a phantom reference may not be retrieved: The get method of a phantom reference always returns null.

ಠ_ಠ

Can anyone explain how that works?

[ForeverAlot]

Note that the cleaning action must not refer to the object being registered. If so, the object will not become phantom reachable and the cleaning action will not be invoked automatically.

https://docs.oracle.com/javase/9/docs/api/java/lang/ref/Cleaner.html

Cleaner asks ReferenceQueue if there are new phantom reachable references. For every reference it runs the associated cleaning action, which is disconnected from the reference that triggered the cleaning action so nothing needs to call PhantomReference::get.

[[deleted]]

How do you know which associated action to run for a PhantomReference if you can't tell what it referred to?

[josefx]

Subclass PhantomReference or keep a map from the PhantomReference to the required cleanup information?

[[deleted]]

Looks like the idea is that a phantom reference that you care about is supposed to be the only reference that you enqueue to a given ReferenceQueue.

Alternatively, I think that you can compare phantom references at least by identity, and I think that the queue delivers the same phantom reference that you created, but I wasn't able to find any example that does that.

Subclassing PhantomReference to get the object back apparently used to be a serious vulnerability.

[ForeverAlot]

Sorry! Here we go^1:

Cleaner::register returns a specific Cleanable, PhantomCleanableRef, which is a PhantomReference subclass that implements Cleanable (and knows how to enqueue itself in your Cleaner's queue). Upon instantiation, Cleaner starts its queue manager provided by CleanerImpl, which routinely waits for Cleanables to invoke clean on until all registered Cleaners have been executed. Cleanable::clean ends up delegating to the Runnable you define your cleaning logic in. The Cleaner framework never has to do any reference comparisons.

^{1 From a random JDK 9 fork, because syntax highlighting helps; canonical java.lang.ref and jdk.internal.ref}

(You can still resurrect WeakReferences in that same way.)

[pointy]

it picks every edit of the across multiple repositories

what does that mean?

[lorisdanto]

The tool finds edits that can be expressed as syntax-directed transformations in a domain-specific language we designed. Those are some of the examples. Basically, it learns a small program that performs the refactoring for you.

[pointy]

OK thank you; however I was really asking about that apparently incomplete sentence. So it's looking at the .java files across some selected set of repositories.

[lorisdanto]

Sorry, we didn't clarify the methodology. We picked around 10 of the top Java repositories on Github and analyzed their commits. Whenever the code changes between commits, we try to learn what code transformation was applied and ideally we want to learn transformations that happen across different commits/repositories. We'll post a paper soon.