{"action":"created","issue":{"url":"https://api.github.com/repos/usgs/asl-sensor-suite/issues/31","repository_url":"https://api.github.com/repos/usgs/asl-sensor-suite","labels_url":"https://api.github.com/repos/usgs/asl-sensor-suite/issues/31/labels{/name}","comments_url":"https://api.github.com/repos/usgs/asl-sensor-suite/issues/31/comments","events_url":"https://api.github.com/repos/usgs/asl-sensor-suite/issues/31/events","html_url":"https://github.com/usgs/asl-sensor-suite/issues/31","id":325417834,"node_id":"MDU6SXNzdWUzMjU0MTc4MzQ=","number":31,"title":"Code review for 1.4.0 Approved release","user":{"login":"jholland-usgs","id":4482969,"node_id":"MDQ6VXNlcjQ0ODI5Njk=","avatar_url":"https://avatars1.githubusercontent.com/u/4482969?v=4","gravatar_id":"","url":"https://api.github.com/users/jholland-usgs","html_url":"https://github.com/jholland-usgs","followers_url":"https://api.github.com/users/jholland-usgs/followers","following_url":"https://api.github.com/users/jholland-usgs/following{/other_user}","gists_url":"https://api.github.com/users/jholland-usgs/gists{/gist_id}","starred_url":"https://api.github.com/users/jholland-usgs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jholland-usgs/subscriptions","organizations_url":"https://api.github.com/users/jholland-usgs/orgs","repos_url":"https://api.github.com/users/jholland-usgs/repos","events_url":"https://api.github.com/users/jholland-usgs/events{/privacy}","received_events_url":"https://api.github.com/users/jholland-usgs/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-05-22T18:44:45Z","updated_at":"2018-05-31T20:45:11Z","closed_at":null,"author_association":"MEMBER","body":"A code review is needed for https://github.com/usgs/asl-sensor-suite/tree/v1.4.0-rc1-reconciliation.\r\nThe review should follow this set of guidelines: \r\nhttps://github.com/usgs/best-practices/blob/master/software/reviews.md#code-review"},"comment":{"url":"https://api.github.com/repos/usgs/asl-sensor-suite/issues/comments/393673743","html_url":"https://github.com/usgs/asl-sensor-suite/issues/31#issuecomment-393673743","issue_url":"https://api.github.com/repos/usgs/asl-sensor-suite/issues/31","id":393673743,"node_id":"MDEyOklzc3VlQ29tbWVudDM5MzY3Mzc0Mw==","user":{"login":"mguy-usgs","id":1719809,"node_id":"MDQ6VXNlcjE3MTk4MDk=","avatar_url":"https://avatars0.githubusercontent.com/u/1719809?v=4","gravatar_id":"","url":"https://api.github.com/users/mguy-usgs","html_url":"https://github.com/mguy-usgs","followers_url":"https://api.github.com/users/mguy-usgs/followers","following_url":"https://api.github.com/users/mguy-usgs/following{/other_user}","gists_url":"https://api.github.com/users/mguy-usgs/gists{/gist_id}","starred_url":"https://api.github.com/users/mguy-usgs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mguy-usgs/subscriptions","organizations_url":"https://api.github.com/users/mguy-usgs/orgs","repos_url":"https://api.github.com/users/mguy-usgs/repos","events_url":"https://api.github.com/users/mguy-usgs/events{/privacy}","received_events_url":"https://api.github.com/users/mguy-usgs/received_events","type":"User","site_admin":false},"created_at":"2018-05-31T20:45:10Z","updated_at":"2018-05-31T20:45:10Z","author_association":"MEMBER","body":"Overall good package, I found no significant issues. Good javadoc in general. I successfully compiled, ran automated tests and ran and used the GUI. I only got it to use up to 230M of RAM, but it dropped back down to 42M (thanks java garbage collection!). The following are recommendations for consideration but not required.\r\n \r\n\r\nMinor/Recommendations:\r\n- Readme.md: this sentence is incomplete under Requirements, Software section \"...produces an executable JAR file that can be launched either through command line or the . \"\r\n- I recommend adding a link or the DOI to the paper for this application into the Readme.md\r\n-  https://github.com/usgs/asl-sensor-suite/blob/master/src/main/java/asl/sensor/CalProcessingServer.java#L54 * If a process is already using that port it silently terminates. - good to note this, but recommend logging an error to notify rather than \"silently terminating\"\r\n- https://github.com/usgs/asl-sensor-suite/blob/d168357ee478122e0b1e33b25181bf503200f8ce/src/main/java/asl/sensor/CalProcessingServer.java#L104 recommend logging something rather than just exiting since an exception was caught\r\n- https://github.com/usgs/asl-sensor-suite/blob/master/src/main/java/asl/sensor/CalProcessingServer.java#L121 also throws SeedFormatException and CodecException (same for rest of methods in this class that throw exceptions)\r\n- https://github.com/usgs/asl-sensor-suite/blob/d168357ee478122e0b1e33b25181bf503200f8ce/src/main/java/asl/sensor/CalProcessingServer.java#L563 Several uses of SuppressWarnings, I recommend these are commented as why they are used and there (e.g. rather than changing/fixing the code)\r\n- https://github.com/usgs/asl-sensor-suite/blob/d168357ee478122e0b1e33b25181bf503200f8ce/src/main/java/asl/sensor/CalProcessingServer.java#L619 if RandData is used externally then why is it defined as a nested class? Would defining it outside of this class (not nested) also fix your compiler warnings?\r\n- recommend adding comments about units or formats for all the appropriate getters  in RandData (https://github.com/usgs/asl-sensor-suite/blob/d168357ee478122e0b1e33b25181bf503200f8ce/src/main/java/asl/sensor/CalProcessingServer.java#L621) \r\n- any time you have units good to add to comments, for example, is latestCorrelation is seconds? https://github.com/usgs/asl-sensor-suite/blob/d168357ee478122e0b1e33b25181bf503200f8ce/src/main/java/asl/sensor/experiment/AzimuthExperiment.java#L49\r\n- the number 3 is hard coded several times in here, maybe make it a static variable for readability and ensured consistency especially with arrays - one example use is at https://github.com/usgs/asl-sensor-suite/blob/4e9f46bae3c5da5fdfde26083e11bf90a9962051/src/main/java/asl/sensor/experiment/NoiseExperiment.java#L41\r\n-  you hard coded your array size here, just making sure you won't exceed it? https://github.com/usgs/asl-sensor-suite/blob/4e9f46bae3c5da5fdfde26083e11bf90a9962051/src/main/java/asl/sensor/experiment/ResponseExperiment.java#L52\r\n- SineExperiment is missing the nice javadoc and intro comment like the rest of the classes https://github.com/usgs/asl-sensor-suite/blob/4e9f46bae3c5da5fdfde26083e11bf90a9962051/src/main/java/asl/sensor/experiment/SineExperiment.java#L10\r\n- hard coded number 3 several times (line 30, 54, 89, 94) should this be a static to ensure consistency and help readability? https://github.com/usgs/asl-sensor-suite/blob/4e9f46bae3c5da5fdfde26083e11bf90a9962051/src/main/java/asl/sensor/experiment/SpectrumExperiment.java#L30\r\n- commented out lines of code maybe removed? just looks like you changed your mind from using a Map to a List https://github.com/usgs/asl-sensor-suite/blob/4e9f46bae3c5da5fdfde26083e11bf90a9962051/src/main/java/asl/sensor/input/InstrumentResponse.java#L50\r\n-  These tools seem fairly generic and maybe usable by others, may want to consider making them another project as a reusable library https://github.com/usgs/asl-sensor-suite/tree/v1.4.0-rc1-reconciliation/src/main/java/asl/sensor/utils\r\n- I ran the automated tests (gradle test), but I did initially get an error on trying to clone seismic_test_data, so I cloned it myself into my local asL_sensor_suite/src/test/resources and then gradle test worked fine would be nice to just mention in the ReadMe.md that test data should go into this directory (in data dir) in asl_sensor_suite, but good to see the automated tests ( https://github.com/usgs/asl-sensor-suite/tree/v1.4.0-rc1-reconciliation/src/test/java/asl/sensor )\r\n"}}
{"actor":{"display_login":"mguy-usgs"}}