Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

code refactoring #121

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sksaifuddin
Copy link

Summary

Checklist

  • Added changelog entry
  • Ran unit tests (rake test:unit)
  • I understand that unless this is a Draft PR or has a DO NOT MERGE label, this PR is considered to be in a deploy ready state and can be deployed if merged to main

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:

  1. 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: 7c55b6e
    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).

  2. 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: 5927f1f
    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.

  3. 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](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.

  4. 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.

  5. 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).

  6. 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.

@sksaifuddin sksaifuddin requested a review from a team as a code owner April 7, 2023 02:20
@sksaifuddin sksaifuddin changed the title Refactor improve code code refactoring Apr 7, 2023
@hollabaq86
Copy link
Contributor

👋 @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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants