1.9.1 Technical Code Review

This ticket documents the code review of the nshmp-haz repository tagged branch 1.9.1.

added In Progress label

assigned to @khaynie

docs/pages/Building-&-Running.md

• Ran into some issues using Docker - I think they are associated with issue #470 (closed)

Docker use and documentation has been revised and works as expected.

docs/pages/About-the-NSHMP.md

• Typo in 2nd paragraph - "The models are used is site-specific"

Fixed in commit ac32420d, MR !559 (merged)

docs/pages/Model-Editions.md

• Typo in Note on the 2014 conterminous US NSHM section - "...and include support in the Wester US for ..."

Fixed typo in b93d96f1, MR !559 (merged)

docs/pages/Source-Types.md

• Typo in 3rd paragraph - "Because gridded seismicity model may be governed by ..."
• Typo in 1st paragraph of the Fault Sources section - "If a fault source is represented with a logic treek then ..."
• Another typo in 1st paragraph of the Fault Sources section - "MFDs associated with finite fault models may be explicitely defined or or derived from ..."

Fixed typos in 30ab9798, MR !559 (merged)

src/test

• HazardCurveTest.java, NshmTestsLarge.java, and NshmTestsSmall.java have blocks of commented code that kinda look like it should not be commented for the tests to properly run ?
• However, unit tests in the pipeline do some to be successful. The coverage rate is 48% which could be improved.

• Cleaned up stale test classes
• Added test coverage issue for next sprint #501
• NshmTestsLarge has been updated as part of our concurrent domain reviews. They are long running tests that we trigger manually after changes from nshmp-lib are merged.

java/gov/usgs/earthquake/nshmp

• Is it necessary to have such a long file path in both src/main and src/test?

This is a peculiarity of java that one just learns to live with but that also is quite useful in terms of carving out a namespace. It also simplifies separating out all tests and resources when building the libray. In most circumstances (IDE's, GitLab) the empty directories are collapsed. The split into source and test is also something that has been adopted by the Java world and build tools are happy with it so I supposed we must too. I

To-Do Items

There are a number of ToDo items staggered throughout the repository. Some of them appear as if they should be addressed while others are perhaps meant more as a comment. Please address these. Here is a list where I saw such ToDo items:

• src/main/java/gov/usgs/earthquake/nshmp/DeaggCalc.java line 124
• src/main/java/gov/usgs/earthquake/nshmp/DeaggEpsilon.java lines 137, 194
• src/main/java/gov/usgs/earthquake/nshmp/DeaggIml.java line 125
• src/main/java/gov/usgs/earthquake/nshmp/RateCalc.java line 190
• src/main/java/gov/usgs/earthquake/nshmp/site/NshmpSite.java lines 23-25
• src/main/java/gov/usgs/earthquake/nshmp/site/NshmpSiteFiles.java lines 59, 213, 398
• src/main/java/gov/usgs/earthquake/nshmp/www/services/DeaggEpsilonService.java line 151
• src/main/java/gov/usgs/earthquake/nshmp/www/services/HazardService.java lines 72, 160, 368
• src/main/java/gov/usgs/earthquake/nshmp/www/services/RateService.java line 308
• src/main/java/gov/usgs/earthquake/nshmp/www/services/ServicesUtil.java line 39
• Src/main/java/gov/usgs/earthquake/nshmp/www/services/ServletUtil.java lines 73, 106
• src/main/java/gov/usgs/earthquake/nshmp/www/services/SourceServices.java line 62
• ws.Dockerfile line 26

The TODOs in this repository have either been addressed where appropriate and removed in !591 (merged)

Significant refactoring of and improvements to the CLI and web service endpoints also contributed to cleaning up TODOs.

Commented Code Blocks

There were a few areas where code was commented. I suggest that these are removed, or if they may be needed then perhaps add a comment explaining why they are to remain commented. Else, maybe some sort of if or for loop could be added. See list location of commented code blocks below:

• src/main/java/gov/usgs/earthquake/nshmp/site/CybershakeSite.java line 21 - still waiting for a response?
• src/main/java/gov/usgs/earthquake/nshmp/www/meta/Metadata.java lines 52-65, 70-71, 77-85
• src/main/java/gov/usgs/earthquake/nshmp/www/services/HazardService.java lines 151-159
• src/main/java/gov/usgs/earthquake/nshmp/www/services/RateService.java lines 136, 140
• Src/main/java/gov/usgs/earthquake/nshmp/www/services/ServletUtil.java lines 67, 89-91, 186-207
• src/main/java/gov/usgs/earthquake/nshmp/www/services/SourceServices.java lines 77-99, 112-116

These and other stale comments have been removed in !591 (merged)

gradle/wrapper/gradle-wrapper.properties

• There are updated versions of Gradle available that could perhaps be used

We updated dependencies across the board in !573 (merged)
Upgrades also helped address log4j vulnerability

src/main/java/gov/usgs/earthquake/nshmp/site/NshmpSiteFiles.java

• Line 85 is missing a closing parentheses

Fixed in !591 (merged)

src/main/java/gov/usgs/earthquake/nshmp/HazardMaps.java

• Line 56 – maybe specify the file must be called curves.csv so it is clear to the user (who may not look closely at the code)

When using this class as a standalone CLI program, the filename can actually be whatever the user wants it to be, as long as it can be parsed as csv. Clarified CLI usage string in !591 (merged)

changed the description

mentioned in merge request !559 (merged)

mentioned in commit ba522893

closed with commit ac32420d