Review (NS-155): FR 1881: BatchGUI
Author |
Jonas |
Moderator |
Jonas |
State |
Closed |
Objectives
Review the code and perform sanity test. Moreover, there might be a security policy violation in the post-processing. Please verify!
Total Time Used (Coding,Documentation,Review):
Time use (Coding,Documentation,Review) CSR: 0.6 JOLF: 12
General comments:
Description |
Classification |
Status |
Comments on file 'trunk/src/dk/netarkivet/common/webinterface/BatchGUI.java', revision 1410
Lines |
Description |
Classification |
Status |
General |
I don't really like the way code and markup are blended together in this class. I think it makes the code hard to maintain. I'd much rather see the utility methods return datastructures containing information about the batchjobs. The jsp page could then have responsibility for displaying the content of these datastructures. Probably this is too large a refactoring to be considered for now. |
Minor |
REJECT |
3, 215 |
What is this? |
Cosmetic |
OK |
201 |
Where are the argN parameters for contructors read/used ? |
Minor |
REJECT |
221 |
This would throw an IllegalArgumentException if there is no parameter-free ctor. |
Minor |
OK |
231 |
Try something like ${jobno}'-.*(?<!metadata-[0-9]).arc' - matches any file which isn't a metadata file. |
Minor |
OK |
348 |
Create Feature Requests (or Bug Reports) to add @Resource annotations to any other batch job classes we already have. |
Cosmetic |
OK |
440-463 |
This sections need comments. |
Cosmetic |
OK |
511 |
comment that duplicates are ignored, since it is a HashSet |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/webinterface/Constants.java', revision 1410
Lines |
Description |
Classification |
Status |
28 |
Add a blank line after each constant declaration for ease of readability. |
Cosmetic |
REJECT |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/FileBatchJob.java', revision 1403
Lines |
Description |
Classification |
Status |
221 |
You mean the OutputStream is null? |
Cosmetic |
REJECT |
Comments on file 'trunk/src/dk/netarkivet/common/settings.xml', revision 1406
Lines |
Description |
Classification |
Status |
429, 433 |
jar file |
Minor |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/CommonSettings.java', revision 1406
Lines |
Description |
Classification |
Status |
410-411 |
jar-batch job! |
Cosmetic |
OK |
416-417 |
jar file |
Minor |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/LoadableFileBatchJob.java', revision 1400
Lines |
Description |
Classification |
Status |
102 |
If loadBatchJob() fails then the next line will throw a NPE, no? Better to check for null condition. |
Minor |
OK |
Comments on file 'trunk/webpages/QA/QA-batchjob-retrieve-resultfile.jsp', revision 1384
Lines |
Description |
Classification |
Status |
64 |
I find it distracting to have to save the file and open it with an editor. Why not use text/plain and send the results to the browser? In other cases (download of order templates and global crawler traps) we let the user choose whether or not to download in the GUI. |
Minor |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/utils/batch/ByteJarLoader.java', revision 1410
Lines |
Description |
Classification |
Status |
1 |
The Id is for a different class. |
Cosmetic |
OK |