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

refactor: replace OpenStruct with Data for performance and readability improvement #3307

Merged

Conversation

Yash-Singh-Pathania
Copy link
Contributor

@Yash-Singh-Pathania Yash-Singh-Pathania commented Oct 4, 2024

Description

This PR refactors the codebase by replacing OpenStruct with Ordered Options for improved performance and immutability. The change ensures that Data is used to represent immutable objects, which provides better memory usage and execution speed compared to OpenStruct. No additional dependencies are required for this change.

Fixes # (issue)
#3245

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

N/A (No UI changes involved)

Manual review steps

  1. Verify that the accounts method returns the correct objects using the new Data class.
  2. Ensure that perform_request correctly handles the refactored Response class in test mode.

Manual reviewer: please leave a comment with output from the test if that's the case.

Copy link

codeclimate bot commented Oct 4, 2024

Code Climate has analyzed commit 7a3ebc5 and detected 0 issues on this pull request.

View more on Code Climate.

docs.avohq.io Outdated Show resolved Hide resolved
@Yash-Singh-Pathania
Copy link
Contributor Author

@adrianthedev can you please review this , I think its done sorry for the delay I just got started with my masters
thanks a ton.

@Yash-Singh-Pathania
Copy link
Contributor Author

@Paul-Bob would you be reviewing this ?

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

It feels unnatural to create a new class and override the initializer; it seems like we're not using the right tool for the job.

I came across this article discussing alternatives to OpenStruct:
https://dev.to/lorankloeze/alternative-for-rubys-openstruct-i72

I think we should consider using a more appropriate tool, such as Struct or Data.

Struct is significantly faster than OpenStruct (~80 times faster), although I’m unsure how it compares to Data. Let’s either research or run a quick benchmark, similar to the one in the article, to determine which option Struct or Data is faster, and proceed with that.

@Yash-Singh-Pathania
Copy link
Contributor Author

@Paul-Bob let me go through this quickly and run a quick benchmark for the same.

@Paul-Bob Paul-Bob added Refactor and removed Feature labels Nov 1, 2024
Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @Yash-Singh-Pathania

I've completed the PR by using Struct instead of previous approaches.

@Paul-Bob Paul-Bob merged commit 4123495 into avo-hq:main Nov 1, 2024
22 checks passed
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.

3 participants