[rfc][icedtea-web] Code cleanup patches

[Andrew Azores]

On 02/04/2014 03:28 PM, Andrew Azores wrote:
> On 02/04/2014 01:24 AM, Jacob Wisor wrote:
>> On 02/03/2014 09:32 PM, Andrew Azores wrote:
>>> Hi,
>>>
>>> The larger two of the three attached patches do a few simple things:
>>> 1) make variables/parameters/fields final, where applicable. I feel 
>>> this makes
>>> it more clear which variables are used eg as accumulators or will be 
>>> returned by
>>> the method, etc. This also allows the removal of some local 
>>> variables which were
>>> created simply to have "final versions" of other variables, in order 
>>> for them to
>>> be captured by anonymous inner classes for example.
>>> 2) prefix fields with "this" in very long methods. In shorter 
>>> methods it can
>>> remain clear that fields are fields since no declaration for that 
>>> name can be
>>> seen, but in very long methods, it can become more confusing.
>>> 3) fix formatting. Eg remove useless comments, remove whitespace 
>>> from "empty"
>>> lines, remove trailing whitespace
>>
>> Looks good to me.
>>
>> Just on nit:
>>
>> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java 
>> b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> [...]
>> -            tracker.addResource(jar.getLocation(),
>> -                    jar.getVersion(), file.getDownloadOptions(),
>> +           this. tracker.addResource(jar.getLocation(),
>> +                    jar.getVersion(), this.file.getDownloadOptions(),
>>
>> Please remove the space between this. and tracker.
>
> Ah, nice catch :)
>
>>
>> You may also want to check for "){" malformed block openings and 
>> replace them with ") {".
>
> Yea, might as well. And we should probably get stricter on this kind 
> of thing during review, too.
>
>>
>> Most of the exception parameters of catch clauses do not need to be 
>> explicitly marked final because they are never assigned to hence they 
>> become /effectively/ /final/. The OpenJDK compiler does take this 
>> into account when generating byte code indeed. But, as I understand 
>> the purpose of these patches was to increase readability.
>>
>> Jacob
>
> Pretty much just for readability's sake, yes.
>
> Thanks,
>

Huh, apparently my IDE was having some difficulties yesterday. Some of 
the places where I prefixed a variable identifier with "this." were 
actually within inner classes and so are actually invalid, but my syntax 
highlighting was apparently nonfunctional and I wasn't quite paying 
close enough attention evidently. This occurred three times in 
JNLPClassLoader. If you apply these patches and find that you can't 
build, well, this is why ;) I won't bother attaching the fixed patches 
because they would be identical other than this.

Thanks,

-- 
Andrew A