/hg/icedtea-web: Properly disconnect all connected http connecti...

[Omair Majid]

Hi,

* Jiri Vanek <jvanek at redhat.com> [2014-05-12 04:19]:
> On 05/09/2014 11:55 PM, Omair Majid wrote:
> >* jvanek at icedtea.classpath.org <jvanek at icedtea.classpath.org> [2014-05-05 09:41]:
> >>changeset e8b21e10ead6 in /hg/icedtea-web
> >>details: http://icedtea.classpath.org/hg/icedtea-web?cmd=changeset;node=e8b21e10ead6
> >>author: Jiri Vanek <jvanek at redhat.com>
> >>date: Mon May 05 15:38:16 2014 +0200
> >>
> >>	Properly disconnect all connected http connections.
> >
> >When you are fixing code, please adjust surrounding code too.
> ...
> >There are lots of similar empty @param's in this changeset. Please fix them
> >all.
> 
> 
> Thank you for kick.
> 
> This patch is fixing javadocs from mentioned push, and adds few more, mostly
> javadoc, but also imports and non final -> final changes and <> operator.
> where I hit it during cleaning.

Please consider splitting this up into separate patches. These are
touching a lot more code that I asked for. I am not sure I am prepared
to review all this right now :)

> 
> This patch have small overleap with Andrew's "[rfc][icedtea-web]
> Resource/ResourceTracker clean up" and Your's urlconnection conn  -> long
> lastMOdified+long contentLength signature change

I like some of the changes, and dislike some of the others. This is a
bit of a personal preference, though.

For example, I am strongly against code that looks like this:

    /** @return the timeout */
    public long getTimeOut() {

Because the comment adds absolutely nothing new to the code and just
obscures things. In fact, it's dangerous because the next time the code
is updated, the comment will not be. After a few changes, the comment
will be entirely misleading an inaccurate.

I strongly encourage you to only add comments when they are needed
(e.g.: the code itself fails to explain something), and then make them
actually useful.

> +++ b/netx/net/sourceforge/jnlp/cache/CacheEntry.java	Mon May 12 10:13:31 2014 +0200

> +import java.io.File;
> +import java.net.URL;
> +import net.sourceforge.jnlp.Version;
> +import net.sourceforge.jnlp.util.PropertiesFile;
>  import net.sourceforge.jnlp.util.logging.OutputController;
> +
>  import static net.sourceforge.jnlp.runtime.Translator.R;

I think we have a convention of putting the static imports right next to
the package statement and before any class-imports.

> +     * 
> +     * @param connection connection on which to initialize resource
>       */

> +    void initialize(long modified, long length) {

The comment is already wrong!

And this is _exactly_ why I prefer to avoid comments. They are so much
full of noise that even you, the author, missed it.

> +     * @return URL of this location

>      public URL getLocation() {

The comment pretty much re-iterates what the code already says: it
returns a URL corresponding to the location. Without any input
arguments, I already assume it's for the `this` object. Do we really
need this comment?

> +     * @return when the item was updated

>      public long getLastUpdated() {

Likewise. If you are going to add a comment here, perhaps you can make a
note of the units (seconds)? But even better, just make it part of the
method name.

> +++ b/netx/net/sourceforge/jnlp/cache/CacheUtil.java	Mon May 12 10:13:31 2014 +0200

> +     * @param u1 first url to compare
> +     * @param u2 second url to compare
> +     * @return whether the  u1 and u2 points to same resource or not
>       */
>      public static boolean urlEquals(URL u1, URL u2) {

You know, this sounds exactly like a method that belongs in UrlUtils.

>       * Note: Because of how our caching system works, deleting jars of another javaws
>       * process is using them can be quite disasterous. Hence why Launcher creates lock files
>       * and we check for those by calling {@link #okToClearCache()}
> +     * @return if the cache could be cleared and was cleared

s/@return if/@return true if/

> +++ b/netx/net/sourceforge/jnlp/runtime/ApplicationInstance.java	Mon May 12 10:13:31 2014 +0200

>      /** the file */
> -    private JNLPFile file;
> +    private final JNLPFile file;

I see that you are making lots of fields final. I generally like that.
Immutable objects are awesome because they can be shared safely across
threads. On the other hand, I am not sure what the benefit of a class
that has some fields non-final.

One convention that I (personally) use is to use final in all fields
when I explicitly want to mark a class as thread safe. When some fields
are final and others are not, I just leave all fields as non-final.

>      /**
>       * Remove an Application Listener
> +     * @param listener toeb removed

If you are going to write comments everywhere, at least try and make
them as correct as you can. 

> +     * Returns the jnlpfile on whic is this application based
> +     * @return LP file for this task.

Same here :(

Thanks,
Omair

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681