[rfc][icetea-web] Resource Setter/Getter Refactoring

[Omair Majid]

Hi,

Thanks for the patch!

* Jie Kang <jkang at redhat.com> [2014-05-16 16:24]:
> 2014-05-16  Lukasz Dracz  <ldracz at redhat.com>
>             Jie Kang      <jkang at redhat.com>
> 
>     * netx/net/sourceforge/jnlp/cache/Resource.java:
>     (location, localFile, requestVersion, downloadVersion,
>     transferred, size, status) made fields private and added
>     setters and getters, and all calling sites refactored
>     * netx/net/sourceforge/jnlp/cache/ResourceTracker.java:
>     Calling sites refactored
>     * netx/net/sourceforge/jnlp/cache/ResourceUrlCreator.java:
>     Calling sites refactored
>     * tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTest.java:
>     (testGetLocation, testGetRequestVersion, testGetDownloadVersion,
>     testTransferredIsZero, testIncrementTransferred, testSizeIsNegativeOne,
>     testSetSize, testStatusIsCopied) added tests

Please read the GNU ChangeLog guidelines and follow them whenever
possible: http://www.gnu.org/prep/standards/html_node/Change-Logs.html

Please pay attention to the syntax (where ':' goes) and the phrasing
(including tenses and keywords like 'Likewise' and 'All callers
changed').

> +++ b/netx/net/sourceforge/jnlp/cache/Resource.java	Fri May 16 11:12:45 2014 -0400

> -     * Returns the tracker that first created or monitored the
> +     * Returns the tracker that first creprivate ated or monitored the

There's a typo here.

> +    /**localFile

You probably meant to remove this.

> +     * Returns the local file being downloaded to
> +     * @return the local file being downloaded

This comment is duplicated. Just like duplication in code is a bad idea,
so is duplication in comments.

> +    public File getLocalFile() {
> +    	return localFile;
> +    }

Please see the Code Style:
http://icedtea.classpath.org/wiki/IcedTea-Web#Code_style

> +    
> +    /**
> +     * Sets the local file to be downloaded to
> +     * @param lFile

If you are documenting a parameter using `@param`, please document it
completely.

However, my preference is to leave off the `@param` wherever possible
and instead make the combination of the method name and the variable
name itself descriptive. I think you don't need it here because a method
named `setLocalFile` that takes a File is fairly obvious.

> +    public void setLocalFile(File lFile) {
> +    	localFile = lFile;

`lFile` should probably be named `localFile`. I would write is as:

  public void setLocalFile(File localFile) {
    this.localFile = localFile;
  }

> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java	Fri May 16 11:12:45 2014 -0400

> +            File finalFile = CacheUtil.getCacheFile(resource.getLocation(), resource.getDownloadVersion()); // This is where extracted version will be, or downloaded file if not compressed.

This last line is definitely too long. Can you move the comment to the
line above?

> -                resource.transferred = downloadLocationFile.length();
> +                resource.incrementTransferred(downloadLocationFile.length());

Can you add an assert here that the current transferred amount is 0?

> +++ b/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTest.java	Fri May 16 11:12:45 2014 -0400
> +	@Test
> +	public void testGetLocation() throws Exception {
> +		String testName = "GetLocation";
> +		Resource res = createResource(testName);
> +		URL location = res.getLocation();
> +		URL dummyUrl = new URL("http://example.com/applet" + testName + ".jar");
> +		assertEquals(location, dummyUrl);

I like that this unit test is small and testing one isolated thing at a
time. But one thing that it's doing appears like magic at first: why is
that `dummyURL` same as the URL of the resource? Can you make this part
more explicit? If the test fails, it should be obvious what is wrong,
and what the right result should be.

> +	public void testIncrementTransferred() throws Exception {

> +		long original = res.getTransferred();

Excellent. This keeps this test independent of the one before (which
verifies the initial value).

> +		res.incrementTransferred(10);
> +		assertEquals(original + 10, res.getTransferred());

Can you make the `10` a constant? That should make it really, really
explicit that the both values should be exactly the same.

Thanks,
Omair

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