0.7.3 Technical Code Review

This ticket documents the technical code review performed against tag 0.7.3 of the nshmp-lib project.

added In Progress label

assigned to @emartinez

gradle.properties

• Consider updating versions to latest

• Updated to close-to-most-recent versions of all dependencies, including node and gradle, the versions for which are specified outside gradle.properties
• The absolute most recent versions of SpotBugs and Guava produce false positives
• See !220 (merged)

build.gradle

• Consider using URL-encoded project name rather than project ID in Maven repo
  • e.g., https://code.usgs.gov/api/v4/projects/ghsc%2fnshmp%2fnshmp-lib/packages/maven

• Updated to project name
• See !220 (merged)

URL-encoded project name is not supported for publishing only getting artifacts.

Using environment variable to get project id.

See !223 (merged)

.gitlab-ci.yml

• Include dependency for pipeline-templates should reference an immutable tag, not the main branch
  • Consider not using a pipeline templates project - this project is not public
• Try to avoid using changes on rules. These can lead to skipping critical steps as changes only looks at current HEAD commit and not full history from ancestor.
• CI/CD variables (project- and inherited) should be masked and probably protected

No longer references a external project for templates.

Templates were added in !222 (merged)

I'm concerned at the amount of data (gmm, fault/surface, etc...) that appears to be included in the project. These data should be released as a data release and imported to the project as part of the build process.

The repository does indeed include a lot of tabular data resources. These are mostly either lookup tables (for table based ground motion models, GMMs) or tables of coefficients for use with GMMs that are based on functional forms. These data are derived from published models (journals) and are are critical for the functioning of the GMMs in nshmp-lib. It is inappropriate for us to then re-publish them on ScienceBase. Moreover, GMMs often go through a significant amount of review and revision and the granular control over these files afforded by git is impossible if we were to put them in ScienceBase; every time we need to adjust a single number, or add new files for new models we'd have to go though the (long) ScienceBase revision process. In other hazard codes, coefficient tables are almost always inlined as hard coded arrays. Also, not a great practice. Although there are quite a few of them, the files themselves are compact and their compressed size is only 2.3 MB. We are leaving these resources in the repository.

Is there some trick to get the tests to work on Windows? I get a lot of invalid character errors. They seem to reference the sigma, delta, etc... special characters.

We have not been able to test building in a Windows environment, only Windows subsystem for linux. It is an encoding problem; we use the UTF-8 character set.

Should avoid use of deprecated methods. For example: AbstractSourceSet.weight Deprecated methods should indicate preferred method/approach to use instead.

In working through this and other reviews, many classes, methods, and fields marked as @Deprecated were removed. Lingering occurrences were removed in !246 (merged) with only a few relevant ones remaining.

DoubleDataTests.toString method test seems to fail:

I suspect the UTF-8 encoding issue on Windows may be to blame here. On linux, all tests pass. The above strings look identical which is odd.

Test coverage could be improved. Currently sitting about 44% overall. Some packages seem well-covered/tested, but others largely not.

Test coverage increased to 70% with the addition of scaled down model tests in !202 (merged). Continued development and refactoring is going to increase this number, specifically:

• gmm package: missing tests for GMMs currently under development
• geo package: missing tests for little used classes that are slated for deprecation and removal
• fault.surface package: most classes slated for deprecation and removal

Java Docs and/or method signatures could be updated to indicate exceptions may be thrown. Particularly this is common for precondition checks throwing exceptions, so it is not obvious these might occur. Since they are typically unchecked, this doesn't cause a problem, but it is nice to document such things for developers.

In considering this comment during review, we believe our exception documentation to be adequate for the time being, but could use improvement as the project moves forward. In general, we do not document null pointers; as a policy in this and other libraries users should expect a NPE if they've passed null. We've deliberately tried to limit public APIs, and documentation of other exceptions for these classes is fairly complete. We also use the builder pattern extensively wherein exception checking and validation is built into builder methods.

There are a large number of TODO that could potentially be completed. Approaching a 1.0.0 release, I would not expect to see so many remaining TODOs. Some are rather simple (e.g., write documentation, add logging), others suggest problems (e.g., TODO THIS IS WRONG).

At the least, these might be documented as known issues or included in some development road map.

Agreed. Many are stale or were put in as reminders but are so low priority that a comment in the code is all that's needed. As such we've done a lot of cleanup:

• Removed 300+ TODOs in !226 (merged):
  • Deleted outdated or already fixed ones
  • Added to or created issues
  • Removed TODO and left as implementation comment or developer note for future refactoring
• Remaining TODOs are mostly in GMM files undergoing active development or are in fact high priority items

assigned to @pmpowers and unassigned @emartinez

I have completed the technical code review as documented above. I've re-assigned this to @pmpowers for reconciliation.

changed milestone to %Coding 'til Christmas

removed milestone %Coding 'til Christmas

changed milestone to %NSHM Hazard Tool launch

I have completed addressing the items noted in this issue and am reassigning to @emartinez for review.

added Needs Review label

removed In Progress label