NetarchiveSuite
Review (NS-29): Review of FR 1160 (Status and order selection (771) on History/Harveststatus-perharvestrun.jsp), and FR 1162 (Multiselect of status on all jobs)
Author |
Søren |
Moderator |
Søren |
State |
Closed |
Objectives
/trunk/src/dk/netarkivet/harvester/datamodel/JobDBDAO.java, r590, Lines 519-580 /trunk/src/dk/netarkivet/harvester/datamodel/JobDBDAO.java, r590, Lines 250-257 /trunk/src/dk/netarkivet/harvester/datamodel/JobStatus.java, r571, All lines /trunk/webpages/History/Harveststatus-perharvestrun.jsp, r648, /trunk/src/dk/netarkivet/harvester/webinterface/HarvestStatus.java
Summary
Follow up by SVC
Total Time Used (Coding,Documentation,Review):
Time use (Coding,Documentation,Review) SVC: 2.5 JOLF: 0.3
General comments:
Description |
Classification |
Status |
Comments on file 'trunk/webpages/History/Harveststatus-perhd.jsp', revision 714
Lines |
Description |
Classification |
Status |
170-202 |
Change indentation (this is actually within a for-loop). |
Cosmetic |
REJECTED |
213-215 |
This section starts with "<%", but it never ends with a "%>". This is a Crucible bug -> end with empty line. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/harvester/datamodel/JobDBDAO.java', revision 590
Lines |
Description |
Classification |
Status |
546-548 |
Though I can't see it for sure, due to the lacking lines, the 'sql' varialble seems to be a String. It should properly be a 'StringBuilder', since it has a lot of other strings appended. |
Cosmetic |
OK |
905 |
Check with 'ArgumentNotValid' whether they are useable ('checkNotNegative' or 'checkPositive'). |
Cosmetic |
OK |
905 |
Javadoc this function. This is inherited, thus write 'See parent'. |
Cosmetic |
OK |
912-913 |
Make Javadoc for the function. |
Cosmetic |
OK |
919 |
"ASC" and "DESC" should be constants. |
Cosmetic |
OK |
941-942 |
'c' and 's' are bad names for a variables. |
Cosmetic |
OK |
962-970 |
This is also used elsewhere. Thus make into a private method. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/harvester/webinterface/HarvestStatus.java', revision 714
Lines |
Description |
Classification |
Status |
224-225 |
ArgumentNotValid.checkNotNegative på disse argumenter. |
Cosmetic |
OK |
236-238 |
Log? or write comment about why not to log. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/harvester/datamodel/JobDAO.java', revision 590
Lines |
Description |
Classification |
Status |
252-256 |
JAVADOC! |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/harvester/datamodel/JobStatus.java', revision 571
Lines |
Description |
Classification |
Status |
105, 107, 109, 111, 113, 115, 117 |
"status.job.new", "status.job.submitted", ... All these should be constants. |
Cosmetic |
OK |
127 |
Argument not valid? |
Cosmetic |
OK |
.