[bisq-network/bisq] Bisq Network Monitor: Babysteps (#2181)

[Chris Beams]

cbeams requested changes on this pull request.

This was a very quick review from my side. Mostly style nits, but my question about the removal of gradle-witness data should be addressed before approval.

> -        'javax.inject:javax.inject:91c77044a50c481636c32d916fd89c9118a72195390452c81065080f957de7ff',
-        'aopalliance:aopalliance:0addec670fedcd3f113c5c8091d783280d23f75e3acb841b61a9cdb079376a08',
-        'com.lambdaworks:scrypt:9a82d218099fb14c10c0e86e7eefeebd8c104de920acdc47b8b4b7a686fb73b4',
-        'com.google.zxing:core:11aae8fd974ab25faa8208be50468eb12349cd239e93e7c797377fa13e381729',
-        'com.cedricwalter:tor-binary-geoip:fbd7656a262607e5a73016e048d5270cbabcd4639a1795b4b4e762df8877429d',
-        'com.github.JesusMcCloud:jtorctl:ba71601cbe50474ccc39a17bc6f7880c1412d8d19b94d37aee69ea2917f72046',
-        'org.apache.commons:commons-compress:5f2df1e467825e4cac5996d44890c4201c000b43c0b23cffc0782d28a0beb9b0',
-        'org.tukaani:xz:a594643d73cc01928cf6ca5ce100e094ea9d73af760a5d4fb6b75fa673ecec96',
-        'com.madgag.spongycastle:core:8d6240b974b0aca4d3da9c7dd44d42339d8a374358aca5fc98e50a995764511f',
-        'net.jcip:jcip-annotations:be5805392060c71474bf6c9a67a099471274d30b83eef84bfc4e0889a4f1dcc0',
-        'org.bitcoinj:orchid:f836325cfa0466a011cb755c9b0fee6368487a2352eb45f4306ad9e4c18de080',
-        'com.squareup.okhttp:okhttp:b4c943138fcef2bcc9d2006b2250c4aabbedeafc5947ed7c0af7fd103ceb2707',
-        'org.jetbrains.kotlin:kotlin-stdlib-common:4b161ef619eee0d1a49b1c4f0c4a8e46f4e342573efd8e0106a765f47475fe39',
-        'com.squareup.okio:okio:114bdc1f47338a68bcbc95abf2f5cdc72beeec91812f2fcd7b521c1937876266',
-    ]
-}

Was this removal intentional?

> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
+ * License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.monitor;
+
+import java.util.Properties;
+
+/**
+ * Configurable is configurable.
+ * 
+ * @author Florian Reimair
+ */

I'd drop any boilerplate Javadoc like this and those that follow. Fine to have it if it really explains something, but otherwise it's line noise.

> + * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.monitor;
+
+import java.util.Properties;
+
+/**
+ * Configurable is configurable.
+ * 
+ * @author Florian Reimair
+ */
+public abstract class Configurable {
+
+

No need for double blank lines here

> + * under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or (at
+ * your option) any later version.
+ *
+ * Bisq is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
+ * License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.monitor;
+
+

No need for double blank lines here

> +import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import bisq.monitor.metric.TorHiddenServiceStartupTime;
+
+public class TorHiddenServiceStartupTimeTests {
+
+    private class DummyReporter extends Reporter {
+
+        private long result;
+
+        @Override
+        public void report(long value) {
+            result = value;
+

```suggestion
```

> +            report(Long.parseLong(values.values().iterator().next()));
+        }
+
+        @Override
+        public void report(Map<String, String> values, String prefix) {
+            report(values);
+        }
+
+        @Override
+        public void report(long value, String prefix) {
+            report(value);
+        }
+
+    }
+
+    private final static File torWorkingDirectory = new File(TorHiddenServiceStartupTimeTests.class.getSimpleName());

Please declare fields together at top of class, not interspersed throughout methods.

> + * under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or (at
+ * your option) any later version.
+ *
+ * Bisq is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
+ * License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.monitor;
+
+

No need for double blank lines here

>  
     dependencies {
-        compile project(':core')
-        compile "com.sparkjava:spark-core:$sparkVersion"
-        compile 'net.gpedro.integrations.slack:slack-webhook:1.1.1'
+        compile 'org.slf4j:slf4j-api:1.7.22'
+        compile 'ch.qos.logback:logback-core:1.1.10'
+        compile 'ch.qos.logback:logback-classic:1.1.10'

Since these dependencies are now repeated between `:monitor` and `:common`, their version numbers can be extracted to `$sf4jVersion` and `$logbackVersion` to avoid version drift between the two subprojects.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/2181#pullrequestreview-188361166
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20181228/8c19a6e3/attachment.html>