⇤ ← Revision 1 as of 2010-06-09 06:42:39
5063
Comment:
|
4952
|
Deletions are marked like this. | Additions are marked like this. |
Line 10: | Line 10: |
{{{ | {{{ |
Line 14: | Line 15: |
Line 17: | Line 17: |
|| Some of the logging levels are too low. The timer thread should log at INFO each time it runs and we should consider adding INFO level logging for each file indexed. || Minor || NOTOK || | || Some of the logging levels are too low. The timer thread should log at INFO each time it runs and we should consider adding INFO level logging for each file indexed. || Minor || OK || |
Line 22: | Line 22: |
|| 36-37 || finish javadoc || Cosmetic || NOTOK || || 41 || javadoc || Cosmetic || NOTOK || || 51 || javadoc || Cosmetic || NOTOK || || 60 || javadoc || Cosmetic || NOTOK || || 67 || javadoc || Cosmetic || NOTOK || || 75 || javadoc || Cosmetic || NOTOK || || 91 || javadoc || Cosmetic || NOTOK || || 109-111 || unnecessary empty lines || Cosmetic || NOTOK || |
|| 36-37 || finish javadoc || Cosmetic || OK || || 41 || javadoc || Cosmetic || OK || || 51 || javadoc || Cosmetic || OK || || 60 || javadoc || Cosmetic || OK || || 67 || javadoc || Cosmetic || OK || || 75 || javadoc || Cosmetic || OK || || 91 || javadoc || Cosmetic || OK || || 109-111 || unnecessary empty lines || Cosmetic || OK || |
Line 32: | Line 32: |
|| 123-136 || Get Søren to accept these new external libraries. || Cosmetic || NOTOK || | || 123-136 || Get Søren to accept these new external libraries. || Cosmetic || OK || |
Line 35: | Line 35: |
|| 74 || javadoc || Cosmetic || NOTOK || || 85 || javadoc || Cosmetic || NOTOK || || 106 || javadoc || Cosmetic || NOTOK || |
|| 74 || javadoc || Cosmetic || OK || || 85 || javadoc || Cosmetic || OK || || 106 || javadoc || Cosmetic || OK || |
Line 40: | Line 40: |
|| 93 || javadoc || Cosmetic || NOTOK || || 98 || javadoc || Cosmetic || NOTOK || || 102 || javadoc || Cosmetic || NOTOK || || 106 || javadoc || Cosmetic || NOTOK || || 110 || javadoc || Cosmetic || NOTOK || || 114 || javadoc || Cosmetic || NOTOK || || 118 || javadoc || Cosmetic || NOTOK || || 127 || What does that mean? || NA || NOTOK || || 132 || javadoc || Cosmetic || NOTOK || || 136 || javadoc || Cosmetic || NOTOK || || 140 || javadoc || Cosmetic || NOTOK || || 144 || javadoc || Cosmetic || NOTOK || || 148 || javadoc || Cosmetic || NOTOK || || 182-188 || comment about what is done here! why only one file? || Cosmetic || NOTOK || || 191 || javadoc || Cosmetic || NOTOK || || 192-209 || Please use some empty lines and comments in this function. It is currently quite difficult to read. || Cosmetic || NOTOK || || 212 || javadoc || Cosmetic || NOTOK || || 230 || javadoc || Cosmetic || NOTOK || || 267 || javadoc || Cosmetic || NOTOK || |
|| 93 || javadoc || Cosmetic || OK || || 98 || javadoc || Cosmetic || OK || || 102 || javadoc || Cosmetic || OK || || 106 || javadoc || Cosmetic || OK || || 110 || javadoc || Cosmetic || OK || || 114 || javadoc || Cosmetic || OK || || 118 || javadoc || Cosmetic || OK || || 127 || What does that mean? || NA || OK || || 132 || javadoc || Cosmetic || OK || || 136 || javadoc || Cosmetic || OK || || 140 || javadoc || Cosmetic || OK || || 144 || javadoc || Cosmetic || OK || || 148 || javadoc || Cosmetic || OK || || 182-188 || comment about what is done here! why only one file? || Cosmetic || OK || || 191 || javadoc || Cosmetic || OK || || 192-209 || Please use some empty lines and comments in this function. It is currently quite difficult to read. || Cosmetic || OK || || 212 || javadoc || Cosmetic || OK || || 230 || javadoc || Cosmetic || OK || || 267 || javadoc || Cosmetic || OK || |
Line 61: | Line 61: |
|| 68-104 || All property names should be contants. || Cosmetic || NOTOK || || 98-105 || Can the settings be empty? What would happen? And why is this the only settings, which are tested for whether they are empty? Write comment about it! || Cosmetic || NOTOK || || 117 || javadoc || Cosmetic || NOTOK || |
|| 68-104 || All property names should be contants. || Cosmetic || OK || || 98-105 || Can the settings be empty? What would happen? And why is this the only settings, which are tested for whether they are empty? Write comment about it! || Cosmetic || OK || || 117 || javadoc || Cosmetic || OK || |
Line 66: | Line 66: |
|| 37 || javadoc || Cosmetic || NOTOK || || 62 || odd indentation || Cosmetic || NOTOK || |
|| 37 || javadoc || Cosmetic ||Postponed || || 62 || odd indentation || Cosmetic ||Postponed || |
Line 70: | Line 70: |
|| 216 || javadoc || Cosmetic || NOTOK || | || 216 || javadoc || Cosmetic || OK || |
Line 73: | Line 73: |
|| 67-68 || Javadoc || Cosmetic || NOTOK || | || 67-68 || Javadoc || Cosmetic || OK || |
Line 76: | Line 76: |
|| 26 || javadoc class || Cosmetic || NOTOK || | || 26 || javadoc class || Cosmetic || OK || |
Line 79: | Line 79: |
|| 65-66 || no need to create a variable for the instance of the batchjob. Just: client.batch(new FileListJob(), ... || Cosmetic || NOTOK || || 74-81 || What is done if the line already exists? nothing. Do something, or at least make a comment about it. || Cosmetic || NOTOK || |
|| 65-66 || no need to create a variable for the instance of the batchjob. Just: client.batch(new FileListJob(), ... || Cosmetic || OK || || 74-81 || What is done if the line already exists? nothing. Do something, or at least make a comment about it. || Cosmetic || OK || |
Line 83: | Line 83: |
|| General || This only contains sporadic javadoc, which deviates from our code-style. || Cosmetic || NOTOK || | || General || This only contains sporadic javadoc, which deviates from our code-style. || Cosmetic || Postponed || |
Review (NS-154): Wayback indexer component
Author |
Colin |
Moderator |
Colin |
State |
Closed |
Objectives
This component creates an unsorted cdx file for each file in the archive.
Total Time Used (Coding,Documentation,Review):
time used jolf - 1md csr 16md
General comments:
Description |
Classification |
Status |
Some of the logging levels are too low. The timer thread should log at INFO each time it runs and we should consider adding INFO level logging for each file indexed. |
Minor |
OK |
Comments on file 'trunk/src/dk/netarkivet/wayback/indexer/GenericHibernateDAO.java', revision 1407
Lines |
Description |
Classification |
Status |
36-37 |
finish javadoc |
Cosmetic |
OK |
41 |
javadoc |
Cosmetic |
OK |
51 |
javadoc |
Cosmetic |
OK |
60 |
javadoc |
Cosmetic |
OK |
67 |
javadoc |
Cosmetic |
OK |
75 |
javadoc |
Cosmetic |
OK |
91 |
javadoc |
Cosmetic |
OK |
109-111 |
unnecessary empty lines |
Cosmetic |
OK |
Comments on file 'trunk/build.xml', revision 1391
Lines |
Description |
Classification |
Status |
123-136 |
Get Søren to accept these new external libraries. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/wayback/indexer/WaybackIndexer.java', revision 1407
Lines |
Description |
Classification |
Status |
74 |
javadoc |
Cosmetic |
OK |
85 |
javadoc |
Cosmetic |
OK |
106 |
javadoc |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/wayback/indexer/ArchiveFile.java', revision 1407
Lines |
Description |
Classification |
Status |
93 |
javadoc |
Cosmetic |
OK |
98 |
javadoc |
Cosmetic |
OK |
102 |
javadoc |
Cosmetic |
OK |
106 |
javadoc |
Cosmetic |
OK |
110 |
javadoc |
Cosmetic |
OK |
114 |
javadoc |
Cosmetic |
OK |
118 |
javadoc |
Cosmetic |
OK |
127 |
What does that mean? |
NA |
OK |
132 |
javadoc |
Cosmetic |
OK |
136 |
javadoc |
Cosmetic |
OK |
140 |
javadoc |
Cosmetic |
OK |
144 |
javadoc |
Cosmetic |
OK |
148 |
javadoc |
Cosmetic |
OK |
182-188 |
comment about what is done here! why only one file? |
Cosmetic |
OK |
191 |
javadoc |
Cosmetic |
OK |
192-209 |
Please use some empty lines and comments in this function. It is currently quite difficult to read. |
Cosmetic |
OK |
212 |
javadoc |
Cosmetic |
OK |
230 |
javadoc |
Cosmetic |
OK |
267 |
javadoc |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/wayback/indexer/HibernateUtil.java', revision 1407
Lines |
Description |
Classification |
Status |
68-104 |
All property names should be contants. |
Cosmetic |
OK |
98-105 |
Can the settings be empty? What would happen? And why is this the only settings, which are tested for whether they are empty? Write comment about it! |
Cosmetic |
OK |
117 |
javadoc |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/wayback/indexer/ArchiveFileDAO.java', revision 1407
Lines |
Description |
Classification |
Status |
37 |
javadoc |
Cosmetic |
Postponed |
62 |
odd indentation |
Cosmetic |
Postponed |
Comments on file 'trunk/src/dk/netarkivet/wayback/NetarchiveResourceStore.java', revision 1391
Lines |
Description |
Classification |
Status |
216 |
javadoc |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/wayback/indexer/IndexerQueue.java', revision 1407
Lines |
Description |
Classification |
Status |
67-68 |
Javadoc |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/wayback/indexer/WaybackIndexerApplication.java', revision 1407
Lines |
Description |
Classification |
Status |
26 |
javadoc class |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/wayback/indexer/FileNameHarvester.java', revision 1407
Lines |
Description |
Classification |
Status |
65-66 |
no need to create a variable for the instance of the batchjob. Just: client.batch(new FileListJob(), ... |
Cosmetic |
OK |
74-81 |
What is done if the line already exists? nothing. Do something, or at least make a comment about it. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/wayback/WaybackSettings.java', revision 1407
Lines |
Description |
Classification |
Status |
General |
This only contains sporadic javadoc, which deviates from our code-style. |
Cosmetic |
Postponed |