Best practices, variables naming and readability
ecerchia opened this issue · 1 comments
ecerchia commented
- Would rename var a to smth else more expressive and would add some glue/boilerplate code to extract the fields and either return them or null if they don't exist. This looks like duplicate code and it's not very readable.
- Same here https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/blockexplorer/Block.java#L42 and https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/blockexplorer/SimpleBlock.java#L22
- I prefer objects over primitives see issue [https://github.com//issues/21], ex where this can be applied [https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/blockexplorer/SimpleBlock.java, https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/blockexplorer/Transaction.java]
- Was not sure what the magic number -1 [https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/blockexplorer/Transaction.java#L38 ] represents and I see some explanations here https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/blockexplorer/Transaction.java#L104. Would be useful to make use of a variable to convey this message.
Ex:
Integer unconfirmedTxBlockHeight = -1;
if (tx.get("block_height").getAsLong() != null) {
blockHeight = tx.get("block_height").getAsLong();
} else {
blockHeight = unconfirmedTxBlockHeight;
}
- Same renaming considerations and NUPE safeguarding can be applied here https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/blockexplorer/UnspentOutput.java#L9
- since both methods here operate on tx parameters, this is a code smell that we are trying to define behaviour for same object, so we need to extract tx in an instance variable and change both methods from static to instance methods https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/pushtx/PushTx.java
- we need to pay attention to these hardcoded URLs which are scattered throughout our code base. Ex: https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/wallet/Wallet.java#L19, https://github.com/blockchain/api-v1-client-java/search?utf8=%E2%9C%93&q=https%3A%2F%2Fblockchain.info%2Fapi%2Fblockchain_wallet_api. If we ever need to change them we need to go to every blockchain repository and search for occurrences. This will not scale if the number of projects will grow a lot. This looks like should be stored in a unique repo in the form of a map and retrieve the value by a stable key so that the value can change without modifying client code
- probably want to implement Singleton pattern here by locking the class https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/HttpClient.java#L22
- this is not best practice for implementing polymorphism and providing flexibility with the implementations. The idea of defining the HttpClientInterface interface is a great start. We can take this further by asking our clients to create their own class that implements the interface and change the behaviour as they wish https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/HttpClient.java#L29
- the code in finally should be extracted in its own method https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/HttpClient.java#L99
- exception swallowing here https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/HttpClient.java#L136. Not a good idea to silently fail without knowing what and where smth went wrong
- memory leak potential. We don't close the buffer https://github.com/blockchain/api-v1-client-java/blob/master/src/main/java/info/blockchain/api/HttpClient.java#L144
https://docs.oracle.com/javase/7/docs/api/java/io/BufferedReader.html#close()