= 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 ||