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

Upgrade TransactionHandler: Improve readability and error handling #152

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bhaskar-allam
Copy link

Simplified the String() method for TransactionHandlerType:

  • Removed the error return, as it's not necessary for a String() method.
  • Used a switch statement without fallthrough for better readability.
  • Added a named error ErrInvalidTransactionHandlerType for better error handling.

Improved the Call method:

  • Used make to pre-allocate the values slice with a capacity of 2.
  • Renamed someResp to response for better clarity.

Refactored the NewTransactionHandler function:

  • Extracted the validation logic into a separate validateTransactionHandler function for better readability and maintainability.
  • Used fmt.Errorf with %w to wrap the error and provide more context.

Improved error messages:

  • Used errors.New for static error messages.

  • Used fmt.Errorf for dynamic error messages.

  • Updated the struct initialization in NewTransactionHandler to use field names for better clarity.

Signed-off-by: Bhaskar Allam [email protected]

@bhaskar-allam bhaskar-allam requested a review from a team as a code owner October 21, 2024 12:41
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the code doesn't look to use the error return value from TransactionHandlerType.String(), removing it seems reasonable. However, you have not updated the accompanying unit tests so the project will no longer build

@bhaskar-allam
Copy link
Author

I should update transaction_handler_test.go also and run Unit Tests againest transaction_handler.go , right ?

Signed-off-by: Bhaskar Ram <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants