Differences between revisions 1 and 3 (spanning 2 versions)

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

-  ⇤ ← Revision 1 as of 2009-03-20 17:35:36 → 
  Size: 3252
  Editor: SoerenCarlsen
  Comment:
+   ← Revision 3 as of 2010-08-16 10:24:30 → ⇥
  Size: 3219
  Editor: localhost
  Comment: converted to 1.6 markup
-Deletions are marked like this.
+Additions are marked like this.
 Line 1:
-= 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)  =
+= 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) =
 Line 18:
-Line 25:
+Line 26:
-Line 27:
+Line 30:
-|| 170-202 || Change indentation (this is actually within a for-loop). || Cosmetic || NOTOK ||
|| 213-215 || This section starts with "<%", but it never ends with a "%>". This is a Crucible bug -> end with empty line. || Cosmetic || NOTOK ||
+|| 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 ||
-Line 31:
+Line 34:
-|| 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 || NOTOK ||
|| 905 || Check with 'ArgumentNotValid' whether they are useable ('checkNotNegative' or 'checkPositive'). || Cosmetic || NOTOK ||
|| 905 || Javadoc this function. This is inherited, thus write 'See parent'. || Cosmetic || NOTOK ||
|| 912-913 || Make Javadoc for the function. || Cosmetic || NOTOK ||
|| 919 || "ASC" and "DESC" should be constants. || Cosmetic || NOTOK ||
|| 941-942 || 'c' and 's' are bad names for a variables. || Cosmetic || NOTOK ||
|| 962-970 || This is also used elsewhere. Thus make into a private method. || Cosmetic || NOTOK ||
+|| 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 ||
-Line 40:
+Line 43:
-|| 224-225 || ArgumentNotValid.checkNotNegative på disse argumenter. || Cosmetic || NOTOK ||
|| 236-238 || Log? or write comment about why not to log. || Cosmetic || NOTOK ||
+|| 224-225 || ArgumentNotValid.checkNotNegative på disse argumenter. || Cosmetic || OK ||
|| 236-238 || Log? or write comment about why not to log. || Cosmetic || OK ||
-Line 44:
+Line 47:
-|| 252-256 || JAVADOC! || Cosmetic || NOTOK ||
+|| 252-256 || JAVADOC! || Cosmetic || OK ||
-Line 47:
+Line 50:
-|| 105, 107, 109, 111, 113, 115, 117 || "status.job.new", "status.job.submitted", ... All these should be constants. || Cosmetic || NOTOK ||
|| 127 || Argument not valid? || Cosmetic || NOTOK ||
+|| 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 ||