db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew McIntyre (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-2311) Generate the txt and blobs required for the system tests dynamically
Date Thu, 01 Mar 2007 23:27:50 GMT

    [ https://issues.apache.org/jira/browse/DERBY-2311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12477130
] 

Andrew McIntyre commented on DERBY-2311:
----------------------------------------

A few comments on these patches:

It appears the patches were generated down in the system test hierarchy. It's better for the
patches to be generated at the top of the tree, reviewers expect them to be generated that
way, plus it captures all the changes in your checkout in one patch file. It would have been
easier to review this patch if it had been together in one file. Also, there are a lot of
formatting changes in the file. Formatting changes without any content change clutter the
patch and make it harder to review. Avoid unnecessary formatting changes if possible. 

Take a look at http://wiki.apache.org/db-derby/PatchAdvice for some tips on creating patches
that are easy for reviewers to review.

DbTasks.java:

You import these three files, but then never use them:

+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
+import java.util.jar.JarInputStream;

Several places in the file are invocations of Random.nextInt that appear as so:

Rn.nextInt( 102400 - 0 + 1 ) + 0;

While it's obvious that it getting a random size for the stream, it would be good to add a
comment as to why this size was chosen for this object. Also, there isn't a need for the addition
and subtraction here, although in some of the other cases where this appears, a number is
added after the random number is generated. Please condense the addition and subtraction in
these cases and/or add a comment as to why they are necessary.

Datatypes.java:

Same comment regarding the use of Random as above.

There were a lot of System.out.printlns removed, was that intentional?

Minor thing, but in both cases you create a new Random object and continue to use Math.random().
Seems like you could standardize generating your random numbers on one of the two methods.

>  Generate the txt and blobs required for the system tests dynamically 
> ----------------------------------------------------------------------
>
>                 Key: DERBY-2311
>                 URL: https://issues.apache.org/jira/browse/DERBY-2311
>             Project: Derby
>          Issue Type: Sub-task
>          Components: Test
>    Affects Versions: 10.3.0.0
>            Reporter: Manjula Kutty
>         Assigned To: Manjula Kutty
>            Priority: Minor
>             Fix For: 10.3.0.0
>
>         Attachments: DERBY-2311_diff.txt, DERBY-2311_diff2.txt, DERBY-2311_stat.txt,
DERBY-2311_stat2.txt, DERBY-2313_diff.txt
>
>
> Generate the txt and blobs dynamically . So that the size of the jar file will get reduced.
And no need of a seperate resource.jar file

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message