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

Moving different transaction types to subclasses, adding transaction attribute class #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

theunraveler
Copy link

Hello. I saw that there were some notes in the code about refactoring the transaction reading and type system, so I did a bit of work on it. Including:

  • Moving the type symbols to proper subclasses (so that we didn't have to do the dynamic attribute stuff)
  • Cleaning up the Transaction#read method, moving all of the different logic to proper methods on the subclasses
  • Adding a Transaction::Attribute class to properly model transaction attributes

Note that in making all of the transaction types into subclasses, I removed the Transaction#type method. It should be easy to add that to each of the subclasses if we need to preserve the backwards compatibility here---just let me know if that's the case and I'll add to this PR.

Thanks, and I'm looking forward to helping out more with this library!

@theunraveler
Copy link
Author

I made a couple changes to the PR. Most notably:

  1. I defined a utility module called Neo::Utils::Entity that abstracts out all of the attributes assignment logic. My hope is that this module can be reused in other classes to allow for easy assignment of a hash of attributes, etc.
  2. I think a nice paradigm is to have classes accept their attributes in their initialize methods. That way, you could use the class in the future to actually create objects in the Neo API. Classes also then have a corresponding class-level read method that creates a new object of that class from API data. I think this is a nice separation of concerns, and keeps odd-looking #read instance methods out of the data model's API. I hope this makes sense.

@theunraveler theunraveler mentioned this pull request Jan 27, 2018
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.

1 participant