trjhtr

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
 .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

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.

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.

• 
  

  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['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.