guygriffiths
commented
10 years ago I've reviewed the code and have found a number of points which could do with changing (see below). I have focused on the actual code, and Jane is going to look at the comments. There are quite a lot of comments, but most of them are fairly minor, and this is a big commit so it's to be expected. Once you have addressed these issues and anything which Jane highlights, you should just push another commit to your master branch. That should (hopefully) update the pull request without us needed to open a new one. Add to this conversation if you have any questions/issues. Comments follow:
CurvilinearDatasetTest
117: An exception is thrown here (as expected), so the statements at lines 119, 121, and 123 are not tested at all.
testCurvlinearCoords: lon_data and lon_values should be named so that it is obvious which one is the expected values and which is the data values
178: cellList, not celllist
200-204: This isn't used anywhere
RangeListTest
83-86: This isn't true because it's traditional, it's true because that's what's in the underlying data
RectilinearGridDatasetTest
221-224: These statements don't test much - afterApplyPlugin just uses exactly the same code as is called in dataset.addVariablePlugin(), plus they are in a loop but don't depend on the loop at all. These lines as well as line 198 should be removed.
231-232: You should calculate these expected values, not use the plugin to do it (since you are trying to test the plugin)
testReturnEmptyTimeseriesFeatures: Also, it's better to use loops such as for(GridCell2D cell : samplePoints) - there's no need to use an index and get(i).
testMapFeaturesWithWrongParams(): This throws an exception (as expected) at line 346, so the test stops there on the first loop. Nothing below line 347 is tested in this method. Also use cell.getCentre() as a target point instead of a BoundingBox
testMapFeatures(): Use cell.getCentre() in PlottingDomainParams, not cell.getFootprint(). Needs more comments
testTimeSeriesFeatures(): cell.getCentre(), not cell.getFootprint() again.
getFeatureByZExtent(): needs comments, it's not obvious what this does
Should be using cell.getCentre() as the target horizontal position rather than cell.getFootprint() for the BoundingBox in PlottingDomainParams on lines: 211, 297, 312, etc. Search cell.getFootprint() in the code to find them all.
858: This is only true for lat/lon grids. That's fine because that's all that's being tested, but you need to specify this in the documentation
GridCoordinates2D
Why have you committed this? It is a minor (and incorrect) change to formatting.
Domain1DMapperTest
Unused imports, unused variable. Remove all compiler warnings before committing code. The rest of this class we can review once you have re-implemented the testIterator method, but that can be in the next pull request (i.e. don't worry about it for this review)
PRTreeFeatureIndexerTest
31: Incorrect documentation link
SimpleTemporalDomainTest
This should test some ISOChronology times, and it would be helpful to say why assert statements should fail (e.g. 29th Feb is not present in a NoLeapChronology)
TrajectoryDomainTest
Could do with more comments on why asserts should be true/false
GridFeatureTest
Remove main method
RectilinearGridImplTest
61-62: Make these private
ReferenceableAxisImplTest
43, 69, 90, etc.: Incorrect spelling - reference doesn't work
testIsAscending(): Needs to test a case that is false
RegularAxisImplTest
140: Never gets tested, as line 139 throws exception
TimeAxisImplTest
Should be merged with TimeAxisTest.
testIsAscending(): Should test a non-ascending axis too.
VerticalAxisImplTest
testIsAscending(): Should test a non-ascending axis too.
AbstractImmutableArrayTest
Needs to test that set fails (i.e. it really is immutable).
CollectionUtilsTest
114: Never executed as previous line throws exception
TimeUtilsTest
testGetTimeRangeForString: Needs to test some non-UTC strings
purplecat7
You may postpone the merge as Domain1Dmapper needs to be write again.