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

Fetch FundingInstrument and Return the appropriate resource #203

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

Fetch FundingInstrument and Return the appropriate resource #203

wants to merge 1 commit into from

Conversation

taylorbrooks
Copy link

This saves people from metaprogramming module names ::Card or ::BankAccount based on hrefs.

@@ -2,6 +2,11 @@ module Balanced

class FundingInstrument

def fetch(href)
Balanced::Card.fetch(href) if href.include?('cards')
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make more sense to use the attribute defined in define_hypermedia_types and then build a registry?

i think this is what the library does when it's determining what type to load for data returned via the api. if we went down this approach then you do not need to explicitly register the type of funding instruments in this method, rather they could be read from a registry.

i'm not 100% sure how to do this in ruby but something like

def fetch(href):
    self.funding_instruments.each do |fi|
        if href.include?(fi.define_hypermedia_types)
            fi.fetch(href)
        end
    end
end

now when subsequent funding instruments are added to the library they will automatically register themselves and this lookup function will work.

Copy link
Author

Choose a reason for hiding this comment

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

You mean so you don't have to add something like:

  Balanced::Bitcoin.fetch(href) if href.include?('bitcoin')

Or whatever the new payment may be?

I would tend to agree with you on building something that allows for more FundingInstruments to be added elsewhere as resources and this module just adapts. Rather than having to explicitly call out each payment method in an if statement.

The only pushback I'd give is... what's the likelihood of adding new funding instruments in the future? And should this code be optimized for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a new funding instrument going out next week :)

I'm just being idealistic here, it's not a requirement, more like an ideal way to implement this feature.

@rserna2010 can you take a look and give it the merge if you're OK with it?

@remear
Copy link
Contributor

remear commented Dec 19, 2014

I feel like it makes more sense to solve #135.

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