Refactor singletons #26
Conversation
| private int nextClient; | ||
| /** The list of endpoints known to the ConnectionManager. */ | ||
| private CopyOnWriteArrayList<AbstractClient> clients; |
There was a problem hiding this comment.
we may think about starting member variables with prefix m?
There was a problem hiding this comment.
This is the default SQ rule for member variables https://rules.sonarsource.com/java/tag/convention/RSPEC-116 whose regex does not enforce this by default.
I still see the benefit of it (and we should agree on something for sure :P) - So should I update the rule?
There was a problem hiding this comment.
i dont't mind - as it is less effort lets stick to the already configured rule.
core/src/main/java/org/operationcrypto/hivej/communication/ConnectionManager.java
Outdated
Show resolved
Hide resolved
| public class HttpClient implements AbstractClient { | ||
| public class HttpClient extends AbstractClient { | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(HttpClient.class); | ||
| private final String CLIENT_ID; |
There was a problem hiding this comment.
why is CLIENT_ID uppercase? shouldnt it match the usual regex pattern?
There was a problem hiding this comment.
'final' variables should be uppercase, no?
There was a problem hiding this comment.
Even Sonar complains about that one - with the rule you just stated above (https://rules.sonarsource.com/java/tag/convention/RSPEC-116). Writing final variables uppercase is fine to me, but could introduce a rule for that to make sonar happy?
|
Changes since last review:
|
|
SonarCloud Quality Gate failed.
|
Based on #15 - Review and merge that one first!
Fixes #21
Refactors both Singletons ('HiveJConfig' and 'ConnectionManager') to:
There is one difference between both Singletons now: 'HiveJConfig' now uses eager initialization, as with Allow to specifiy multiple endpoints and loadbalance requests between them #15, the HiveJConfig is the only place that adds endpoints to the ConnectionManager so it always needs to be initialized (solves the fixme in the tests too). The 'ConnectionManager' is still "lazy loading".
The following points are open for discussion: