1
\$\begingroup\$

This class calculates the gains and losses (in USD) from the four leading cryptocurrencies using the Coinbase API. (The full repo can be found here)

In addition to general feedback there are some specific items I would like feedback on:

  • What can I do to improve my tests? (For example, is it silly of me to test only that a method returns a float like in .crypto_amount_in_wallet? If so are there any other ways I could go about testing this method, that wouldn't just be a test of the Coinbase API?)
  • Am I using the VCR gem in a way that makes sense?
  • Is there a better or more conventional way to mock data than recording the VCR cassettes and modifying the responses by hand?
  • Should I consider separating functionality that purely wraps API endpoints into a separate class, or leave them where they are?

Class

require 'coinbase/wallet' require 'dotenv' class Currency def initialize(symbol:, api_client:) raise ArgumentError 'Must specify currency symbol (BTC BCH LTC ETH)' if symbol.nil? || !([:BTC, :LTC, :BCH, :LTC].include?(symbol)) raise ArgumentError 'Currency requires a valid coinbase client.' if api_client.nil? || api_client.class != Coinbase::Wallet::Client @symbol = symbol @api_client = api_client @account = @api_client.account(symbol) @crypto_amount_in_wallet = @account['balance']['amount'] @usd_invested = @account['native_balance']['amount'] end def symbol return @symbol end def api_client return @api_client end def account account = self.api_client.account(self.symbol) account.refresh! return account end def crypto_amount_in_wallet return Float(self.account['balance']['amount']) end def usd_invested transactions = self.account.transactions total_invested = transactions .map { |t| t['native_amount']['amount'].to_f } .reduce(:+) return Float(total_invested) end def current_cash_in_value Float(self.account['native_balance']['amount']) ## TODO: Use the buy/quote endpoint instead end def usd_lost loss = self.usd_invested - self.current_cash_in_value if loss.negative? # i.e. $1.00 - $10.00 = -$9.00 means that $9.00 have been made as profit, so return a $0.00 loss. return 0.0 else return loss end end def usd_gained gain = self.current_cash_in_value - self.usd_invested if gain.negative? # i.e. $1.00 - $100.00 = -$99.00 means that $99.00 have been lost as profit, so return a $0.00 as a gain. return 0.0 else return gain end end end

Spec

require 'rspec' require_relative '../lib/currency.rb' describe Currency do before (:all) do VCR.use_cassette('client_and_currency') do @api_client = Coinbase::Wallet::Client.new(api_key: ENV['COINBASE_KEY'], api_secret: ENV['COINBASE_SECRET']) @currency = Currency.new(symbol: :BTC, api_client: @api_client) end end describe '#initialize' do it 'raises an ArgumentError when a new currency is instantiated without a symbol' do expect { Currency.new(api_client: @api_client) }.to raise_error ArgumentError end it 'raises an ArgumentError if no coinbase client object is passed' do expect { Currency.new(symbol: :BTC) }.to raise_error ArgumentError end it 'returns a new object of type "Currency"' do VCR.use_cassette('currency_init') do expect(Currency.new(symbol: :BTC, api_client: @api_client)).to be_a_kind_of Currency end end end describe '.symbol' do it 'returns a symbol' do expect(@currency.symbol).to be_a Symbol end it 'is one of :BTC, :LTC, :BCH, :ETH' do expect([:BTC, :LTC, :BCH, :ETH].include?(@currency.symbol)).to be true end end describe '.api_client' do it 'properly instantiates a coinbase client' do expect(@currency.api_client).to be_a Coinbase::Wallet::Client end it 'doesn\'t raise an error' do expect { @currency.api_client }.not_to raise_error end end describe '.account' do it 'returns a hash' do VCR.use_cassette('account_hash') do expect(@currency.account).to be_a Hash end end it 'has 11 keys' do VCR.use_cassette('account_hash') do expect(@currency.account.keys.count).to eql(11) end end it 'matches the symbol' do VCR.use_cassette('account_hash') do expect(@currency.symbol.to_s).to eq(@currency.account['currency']) end end end describe '.crypto_amount_in_wallet' do it 'is a float' do VCR.use_cassette('crypto_amount_in_wallet') do expect(@currency.crypto_amount_in_wallet).to be_a Float end end end describe '.usd_invested' do it 'is a float' do VCR.use_cassette('account') do expect(@currency.usd_invested).to be_a Float end end end describe '.current_cash_in_value' do it 'is a float' do VCR.use_cassette('current_cash_in_val') do expect(@currency.current_cash_in_value).to be_a Float end end end describe '.usd_lost' do context 'given no loss' do it 'should return 0.0' do VCR.use_cassette('usd_no_loss') do expect(@currency.usd_lost).to eql(0.0) end end end context 'given a loss' do it 'should return 9.00 as a loss' do VCR.use_cassette('usd_loss') do expect(@currency.usd_lost).to eql(10.0 - 1.0) end end end end describe '.usd_gained' do context 'with no gain' do it 'returns 0.0 as a gain' do VCR.use_cassette('usd_no_gain') do expect(@currency.usd_gained).to eql(0.0) end end end context 'with a gain' do it 'returns 10.0 as a gain' do VCR.use_cassette('usd_gain') do expect(@currency.usd_gained).to eql(10.0) end end end end end
\$\endgroup\$

3 Answers 3

Reset to default
2
\$\begingroup\$

Rorshark's points are important: if your class is doing too many things, it probably means you have other classes hiding in there. Before you ask about how good your tests are, you should be asking if you are testing the right things.

The defined methods in your class:

  • symbol
  • api_client
  • account
  • crypto_amount_in_wallet
  • usd_invested
  • current_cash_in_value
  • usd_lost
  • usd_gained

The two constructor arguments (symbol, api_client) aren't used except in the account method. This tells me that the class may be abstracted too far, or that you need a separate account class as Rorshark suggested.

You have a currency, but it's not the main thing here. The main thing seems to be the account and the operations on the account (amount_in_wallet, invested, cash_value, usd_lost, usd_gained). The currency value is just an argument needed to get a handle on the account.

So, if you renamed the class 'Account', made the account method private or just turned it into a class variable you'd have something a little more cohesive.

Some stylistic nits:

  • def symbol ... and def api_client ... could be condensed to attr_reader :symbol, :api_client

  • it's a little more Ruby-esque to skip return for end-of-method return values, but not a critical thing

Some things to be careful with:

  • using floats when it comes to currency will come back to bite you. Consider converting everything to integers early (multiply currency by some precision factor: 100, 1000, 10,000, or even 100,000) and then you can just use integer math everywhere until it's time to display.
\$\endgroup\$
1
\$\begingroup\$

Since most of the useful methods here involve doing agregate calculations on “account”, you may want to consider renaming the class to “Account” and passing in the account hash in the initializer rather than passing in an api client and currency code.

That would isolate the actual logic from the coinbase api entirely and would make your code much easier to test.

You wouldnt need VCR at all if you did that.

\$\endgroup\$
4
  • \$\begingroup\$ Not a bad idea, I feel like doing so would invalidate one of the purposes behind the class though: which is to get up to date account data each time the account getter is called. \$\endgroup\$ Commented Mar 24, 2018 at 22:06
  • \$\begingroup\$ Sure, but the phrase “one of the purposes” is telling, there. A class would ideally have one purpose. \$\endgroup\$ Commented Mar 26, 2018 at 16:52
  • \$\begingroup\$ Also, a class already exists for "getting up to date account data", it's called Coinbase::Wallet::Client, which I'm assuming does its own validation of "currency symbol". \$\endgroup\$ Commented Mar 26, 2018 at 16:56
  • \$\begingroup\$ The purpose of the class is to provide up to date data about a specific currency. Hence why it's a Currency class not an Account class. Not requesting the data each time (as you've suggested) would invalidate the purpose of the class. Which is to abstract each currency for a specific user into its own object, in as close to real time as possible. This class does have one purpose. \$\endgroup\$ Commented Apr 18, 2018 at 3:16
1
\$\begingroup\$

  • What can I do to improve my tests? (For example, is it silly of me to test only that a method returns a float like in .crypto_amount_in_wallet? If so are there any other ways I could go about testing this method, that wouldn't just be a test of the Coinbase API?

    Yes, your tests are weak, you should focus in the core logic of your class and assert edge cases. Testing the type of the return isn't enough.

  • Am I using the VCR gem in a way that makes sense?

    Yes, VCR is a good choise when the third-service don't provide testing mocks (example, FOG gem has built-in testing mocks). But you don't need to test everything with VCR, you can mock some code (to assert edge cases), and use VCR to do integration testing.

  • Is there a better or more conventional way to mock data than recording the VCR cassettes and modifying the responses by hand?

    I think it's preferable to never modify responses by hand, you should simulate your senario with a real data. The reason is that cassetes should be disposable, if you need to update your testes, or the service API changes, you just need to thrown then way and record new ones.

  • Should I consider separating functionality that purely wraps API endpoints into a separate class, or leave them where they are?

    @Rorshark justed this, you can create an AccountWrapper to do this.

I applied some improvements in your class applying a little of ruby-style-guide, here is the final result:

(sorry renaming your methods, but you can ignore this :D)

require 'coinbase/wallet' class AccountStatus SUPPORTED_SYMBOLS = [:BTC, :LTC, :BCH, :LTC] attr_reader :symbol, :api_client def initialize(symbol:, api_client:) raise ArgumentError 'Must specify currency symbol (BTC BCH LTC ETH)' unless SUPPORTED_SYMBOLS.include?(symbol) raise ArgumentError 'Currency requires a valid coinbase client.' unless api_client.is_a?(Coinbase::Wallet::Client) @symbol = symbol @api_client = api_client end def account @account ||= api_client.account(symbol).tap(&:refresh!) end def crypto_balance account['balance']['amount'] end def cash_invested account.transactions.map { |t| t['native_amount']['amount'] }.sum end def cash_current account['native_balance']['amount'] # TODO: Use the buy/quote endpoint instead end def usd_lost # returns 0.0 on negative values [cash_invested - cash_current, 0.0].max end def usd_gained # returns 0.0 on negative values [cash_current - cash_invested, 0.0].max end end

Few things to notice:

  • I removed some unused variables and .nil? checks on constructor params (nil values will fail the boolean check)
  • Cached the @account variable, I think it's not a good idea to keep refreshing it, but you must decide on that
  • Removed the conditional on lost/gained methods using Array#max
  • Removed Float conversions, I read that the coinbase gem already converts money amounts into BigDecimal ref

I'll work in the test file later and update this answer with it, sorry I'm out of time now.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the time you took for the review and for answering my questions! And I would love feedback on the tests if you have a chance. \$\endgroup\$ Commented Mar 25, 2018 at 2:30
  • 1
    \$\begingroup\$ I would be totally willing to mark this as the answer if you were to provide more feedback on the spec. Specifically if you could point me in the right direction as far as mocking data from the Coinbase::Wallet::Client. \$\endgroup\$ Commented Apr 18, 2018 at 3:10

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.