hollabaq86
commented
1 year ago 👋 @sksaifuddin thanks so much for the PR! We'll take a look and provide any feedback. It might be a minute until we formally review, but we will take a look!
For internal tracking, ticket 2568
Summary
Checklist
rake test:unit)I have used brain tree in the past for the company I was working with and I am really interested in contributing to this repo. To get myself better with the code base and also to learn more about refactoring techniques, I made some small refactoring changes, which I believe might improve the code readability and management. It was huge learning for me to go through this repo 😁.
I am adding all the details of the refactoring done by and small description of why I think this refactoring is required. Please let me know if you think something is wrong or need more changes, I will do the changes, it will help me get better with the code base.
Refactorings:
Extract Method: (https://refactoring.guru/extract-method) a. Location i. Package: src/main/java/com/braintreegateway ii. Class: Customer.java iii. Method: Customer(NodeWrapper node) iv. Link of files of previous commit (before refactoring): https://github.com/braintree/braintree_java/blob/5927f1f212eb41d5aabf2b5889f882042573ad8f/src/main/java/com/braintreegateway/Customer.java v. Link of file of the commit with refactoring changes (after refactoring): https://github.com/braintree/braintree_java/blob/7c55b6e1f5cda5402387c3475acd8464de147bda/src/main/java/com/braintreegateway/Customer.java vi. Link to commit: https://github.com/braintree/braintree_java/commit/7c55b6e1f5cda5402387c3475acd8464de147bda b. Description: The contents of Customer was too big and was doing two different things: assigning node variables and creating lists of nodeWrapper. These two things can be divided into two different methods to perform “Extract method refactoring”. I’ve extracted the code into two methods: setCustomerNodeWrapperStrings(node); and setCustomerNodeWrapperResponses(node).
Decompose Conditional: (https://refactoring.guru/decompose-conditional) a. Location: i. Package: src/main/java/com/braintreegateway/util/Crypto.java ii. Class: Crypto.java iii. Method: secureCompare(String left, String right) iv. Link of files of previous commit (before refactoring): https://github.com/braintree/braintree_java/blob/449703fa7f1a54067e4a3a93cd9b5adba67bf694/src/main/java/com/braintreegateway/util/Crypto.java v. Link of file of the commit with refactoring changes (after refactoring): https://github.com/braintree/braintree_java/blob/5927f1f212eb41d5aabf2b5889f882042573ad8f/src/main/java/com/braintreegateway/util/Crypto.java vi. Link to commit: https://github.com/braintree/braintree_java/commit/5927f1f212eb41d5aabf2b5889f882042573ad8f b. Description: The method secureCompare had a complex conditional statement. To make it simple I transferred the condition to another method called: checkSecureCompareDirections(), which just returns a Boolean value hence performing Decompose conditional refactoring.
Introduce Explaining Variable: (https://blog.thepete.net/blog/2021/06/24/explaining-variable/#:~:text=An%20Explaining%20Variable%20is%20a,a%20little%20more%20self%2Ddocumenting.) a. Location: i. Package: src/main/java/com/braintreegateway ii. Class: MerchantAccountGateway.java iii. Method: update(String id, MerchantAccountRequest request) iv. Link of files of previous commit (before refactoring): https://github.com/braintree/braintree_java/blob/7c55b6e1f5cda5402387c3475acd8464de147bda/src/main/java/com/braintreegateway/MerchantAccountGateway.java\ v. Link of file of the commit with refactoring changes (after refactoring): https://github.com/braintree/braintree_java/blob/741e351ff48b8af9ff3fa068351e0206b594be07/src/main/java/com/braintreegateway/MerchantAccountGateway.java vi. Link to commit: https://github.com/braintree/braintree_java/commit/741e351ff48b8af9ff3fa068351e0206b594be07 b. Description: The method update makes a put API call using http. For creating the url to make http call there it was directly writing url in the put method. I have made an explaining variable: updateUrl, and uses in put method, instead of directly adding string, now its clear what is the purpose to that String, Hence performing Explaining variable refactoring.
Move method: (https://refactoring.guru/move-method) a. Location: i. Package: src/main/java/com/braintreegateway b. Description: The method connectUrl of clas oAuthGateway.java was mostly just using the objects of Configuration class. So it made more sense to just add it in the Configuration.java class. Since the method was moved from one class to another ( oAuthGateway -> Configuration.java) this is a move method refactoring.
Push-down method: (https://refactoring.guru/push-down-method) a. Location: i. Package: src/main/java/com/braintreegateway b. Description: Request is extended by multiple classes but the method buildXMLElement is being used by just one child class which is: SearchCriteria.java, so instead of having the method in parent we can just push it down to child class, hence making it push down refactoring. The method moved from Request.java (parent) -> SearchCriteria.java (child).
Pull-up method: (https://refactoring.guru/pull-up-method) a. Location: i. Package: src/main/java/com/braintreegateway/util b. Description: There are two methods in classes (Sha1Hasher and Sha256Hasher) hmacHash and sha1Bytes, sha256Bytes, also these two classes implement interface: Hasher. The code in these methods is almost same, it just hardcodes the value of sha algorithm in each individual class. I made changes to pull-up these methods to a parent class called ShaTypeHasher and implement these two methods in this parent class, the SHA algorithm value will be passed as a parameter to the methods. Hence performing pull-up refactoring, since the methods moved from child classes (Sha1Hasher, Sha256Hasher) -> parent (ShaTypeHasher) class. It also resulted in removing duplicate code and making it more readable.