Review (NS-155): FR 1881: BatchGUI

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

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

Lines	Description	Classification	Status
28	Add a blank line after each constant declaration for ease of readability.	Cosmetic	REJECT

Lines	Description	Classification	Status
221	You mean the OutputStream is null?	Cosmetic	REJECT

Lines	Description	Classification	Status
429, 433	jar file	Minor	OK

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

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

Lines	Description	Classification	Status
1	The Id is for a different class.	Cosmetic	OK