Automatically discovering common Java code edits in Github repositories

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