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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/balanced/resources/funding_instrument.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Balanced::BankAccount.fetch(href) if href.include?('bank_accounts')
end

end

class DebitableFundingInstrument < FundingInstrument
Expand Down