From fc3ade392a384bc3cd39409b729b8386c54013cf Mon Sep 17 00:00:00 2001
From: Jose Farias <31393016+josefarias@users.noreply.github.com>
Date: Mon, 22 Apr 2024 06:30:42 -0600
Subject: [PATCH] Refactor `TimeSeries` artifacts (#651)
* Reindent TimeSeries classes
* Fix spacing in time series tests
* Remove trend tests where current is nil
I think if we've gotten this far with a nil value for current, there's a data integrity problem.
If we allow this, we'll have to be very defensive in our code. Best to raise and fix early.
* Reindent Money class
* Refactor TimeSeries artifacts
* Use as_json in TimeSeries
* Bring back tests for trends where current is nil
* Bring back trend test
* Correctly enumerate trend test
* Use favorable_direction for trend_styles helper
* Make trend public in TimeSeries::Value
* Allow nil current values in trends
I think I might've gotten it wrong before, nils might appear in trends if values are unavailable for snapshots
* Clean up TimeSeries::Trend
* Skip trend values same class validations if any values are nil
* Refactor Money
* Remove object parsing in TimeSeries::Value
We're only every passing hashes
---
app/helpers/application_helper.rb | 6 +-
app/models/time_series.rb | 132 +++++++++--------------
app/models/time_series/trend.rb | 93 +++++++++++-----
app/models/time_series/value.rb | 60 +++++++----
app/views/shared/_trend_change.html.erb | 2 +-
app/views/shared/_value_heading.html.erb | 2 +-
lib/money.rb | 131 ++++++++++++----------
test/models/time_series/trend_test.rb | 7 +-
test/models/time_series_test.rb | 31 +++---
9 files changed, 258 insertions(+), 206 deletions(-)
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index d37472bd..56be0a49 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -53,13 +53,13 @@ module ApplicationHelper
def trend_styles(trend)
fallback = { bg_class: "bg-gray-500/5", text_class: "text-gray-500", symbol: "", icon: "minus" }
- return fallback if trend.nil? || trend.direction == "flat"
+ return fallback if trend.nil? || trend.direction.flat?
bg_class, text_class, symbol, icon = case trend.direction
when "up"
- trend.type == "liability" ? [ "bg-red-500/5", "text-red-500", "+", "arrow-up" ] : [ "bg-green-500/5", "text-green-500", "+", "arrow-up" ]
+ trend.favorable_direction.down? ? [ "bg-red-500/5", "text-red-500", "+", "arrow-up" ] : [ "bg-green-500/5", "text-green-500", "+", "arrow-up" ]
when "down"
- trend.type == "liability" ? [ "bg-green-500/5", "text-green-500", "-", "arrow-down" ] : [ "bg-red-500/5", "text-red-500", "-", "arrow-down" ]
+ trend.favorable_direction.down? ? [ "bg-green-500/5", "text-green-500", "-", "arrow-down" ] : [ "bg-red-500/5", "text-red-500", "-", "arrow-down" ]
when "flat"
[ "bg-gray-500/5", "text-gray-500", "", "minus" ]
else
diff --git a/app/models/time_series.rb b/app/models/time_series.rb
index 001c7bdd..14342a13 100644
--- a/app/models/time_series.rb
+++ b/app/models/time_series.rb
@@ -1,83 +1,57 @@
-
class TimeSeries
- attr_reader :type
+ DIRECTIONS = %w[ up down ].freeze
- def self.from_collection(collection, value_method, options = {})
- data = collection.map do |obj|
- { date: obj.date, value: obj.public_send(value_method), original: obj }
- end
- new(data, options)
+ attr_reader :values, :favorable_direction
+
+ def self.from_collection(collection, value_method)
+ collection.map do |obj|
+ {
+ date: obj.date,
+ value: obj.public_send(value_method),
+ original: obj
+ }
+ end.then { |data| new(data) }
+ end
+
+ def initialize(data, favorable_direction: "up")
+ @favorable_direction = (favorable_direction.presence_in(DIRECTIONS) || "up").inquiry
+ @values = initialize_values data
+ end
+
+ def first
+ values.first
+ end
+
+ def last
+ values.last
+ end
+
+ def on(date)
+ values.find { |v| v.date == date }
+ end
+
+ def trend
+ TimeSeries::Trend.new \
+ current: last&.value,
+ previous: first&.value,
+ series: self
+ end
+
+ # `as_json` returns the data shape used by D3 charts
+ def as_json
+ {
+ values: values.map(&:as_json),
+ trend: trend.as_json,
+ favorable_direction: favorable_direction
+ }.as_json
+ end
+
+ private
+ def initialize_values(data)
+ [ nil, *data ].each_cons(2).map do |previous, current|
+ TimeSeries::Value.new **current,
+ previous_value: previous.try(:[], :value),
+ series: self
+ end
end
-
- def initialize(data, options = {})
- @type = options[:type] || :normal
- initialize_series_data(data)
- end
-
- def values
- @values ||= add_trends_to_series
- end
-
- def first
- values.first
- end
-
- def last
- values.last
- end
-
- def on(date)
- values.find { |v| v.date == date }
- end
-
- def trend
- TimeSeries::Trend.new(
- current: last&.value,
- previous: first&.value,
- type: @type
- )
- end
-
- # Data shape that frontend expects for D3 charts
- def to_json(*_args)
- {
- values: values.map do |v|
- {
- date: v.date,
- value: JSON.parse(v.value.to_json),
- trend: {
- type: v.trend.type,
- direction: v.trend.direction,
- value: JSON.parse(v.trend.value.to_json),
- percent: v.trend.percent
- }
- }
- end,
- trend: {
- type: @type,
- direction: trend.direction,
- value: JSON.parse(trend.value.to_json),
- percent: trend.percent
- },
- type: @type
- }.to_json
- end
-
- private
- def initialize_series_data(data)
- @series_data = data.nil? || data.empty? ? [] : data.map { |d| TimeSeries::Value.new(d) }.sort_by(&:date)
- end
-
- def add_trends_to_series
- [ nil, *@series_data ].each_cons(2).map do |previous, current|
- unless current.trend
- current.trend = TimeSeries::Trend.new(
- current: current.value,
- previous: previous&.value,
- type: @type
- )
- end
- current
- end
- end
end
diff --git a/app/models/time_series/trend.rb b/app/models/time_series/trend.rb
index 6a748056..70f9077e 100644
--- a/app/models/time_series/trend.rb
+++ b/app/models/time_series/trend.rb
@@ -1,48 +1,93 @@
class TimeSeries::Trend
- attr_reader :type
+ include ActiveModel::Validations
- # Tells us whether an increasing/decreasing trend is good or bad (i.e. a liability decreasing is good)
- TYPES = %i[normal inverse].freeze
+ attr_reader :current, :previous
- def initialize(current: nil, previous: nil, type: :normal)
- validate_data_types(current, previous)
- validate_type(type)
+ delegate :favorable_direction, to: :series
+
+ validate :values_must_be_of_same_type, :values_must_be_of_known_type
+
+ def initialize(current:, previous:, series: nil)
@current = current
@previous = previous
- @type = type
+ @series = series
+
+ validate!
end
def direction
- return "flat" if @previous.nil? || @current == @previous
- return "up" if @current && @current > @previous
- "down"
+ if previous.nil? || current == previous
+ "flat"
+ elsif current && current > previous
+ "up"
+ else
+ "down"
+ end.inquiry
end
def value
- return Money.new(0) if @previous.nil? && @current.is_a?(Money)
- return 0 if @previous.nil?
- @current - @previous
+ if previous.nil?
+ current.is_a?(Money) ? Money.new(0) : 0
+ else
+ current - previous
+ end
end
def percent
- return 0.0 if @previous.nil?
- return Float::INFINITY if @previous == 0
- ((extract_numeric(@current) - extract_numeric(@previous)).abs / extract_numeric(@previous).abs.to_f * 100).round(1).to_f
+ if previous.nil?
+ 0.0
+ elsif previous.zero?
+ Float::INFINITY
+ else
+ change = (current_amount - previous_amount).abs
+ base = previous_amount.abs.to_f
+
+ (change / base * 100).round(1).to_f
+ end
+ end
+
+ def as_json
+ {
+ favorable_direction: favorable_direction,
+ direction: direction,
+ value: value,
+ percent: percent
+ }.as_json
end
private
- def validate_type(type)
- raise ArgumentError, "Invalid type" unless TYPES.include?(type)
+ attr_reader :series
+
+ def values_must_be_of_same_type
+ unless current.class == previous.class || [ previous, current ].any?(&:nil?)
+ errors.add :current, "must be of the same type as previous"
+ errors.add :previous, "must be of the same type as current"
+ end
end
- def validate_data_types(current, previous)
- return if previous.nil? || current.nil?
- raise ArgumentError, "Current and previous values must be of the same type" unless current.class == previous.class
- raise ArgumentError, "Current and previous values must be of type Money or Numeric" unless current.is_a?(Money) || current.is_a?(Numeric)
+ def values_must_be_of_known_type
+ unless current.is_a?(Money) || current.is_a?(Numeric) || current.nil?
+ errors.add :current, "must be of type Money, Numeric, or nil"
+ end
+
+ unless previous.is_a?(Money) || previous.is_a?(Numeric) || previous.nil?
+ errors.add :previous, "must be of type Money, Numeric, or nil"
+ end
+ end
+
+ def current_amount
+ extract_numeric current
+ end
+
+ def previous_amount
+ extract_numeric previous
end
def extract_numeric(obj)
- return obj.amount if obj.is_a? Money
- obj
+ if obj.is_a? Money
+ obj.amount
+ else
+ obj
+ end
end
end
diff --git a/app/models/time_series/value.rb b/app/models/time_series/value.rb
index fbf8ab31..e691159d 100644
--- a/app/models/time_series/value.rb
+++ b/app/models/time_series/value.rb
@@ -1,32 +1,46 @@
class TimeSeries::Value
- include Comparable
+ include Comparable
+ include ActiveModel::Validations
- attr_accessor :trend
- attr_reader :value, :date, :original
+ attr_reader :value, :date, :original, :trend
- def initialize(obj)
- @original = obj.fetch(:original, obj)
+ validates :date, presence: true
+ validate :value_must_be_of_known_type
- if obj.is_a?(Hash)
- @date = obj[:date]
- @value = obj[:value]
- else
- @date = obj.date
- @value = obj.value
- end
+ def initialize(date:, value:, original: nil, series: nil, previous_value: nil)
+ @date, @value, @original, @series = date, value, original, series
+ @trend = create_trend previous_value
- validate_input
+ validate!
+ end
+
+ def <=>(other)
+ result = date <=> other.date
+ result = value <=> other.value if result == 0
+ result
+ end
+
+ def as_json
+ {
+ date: date,
+ value: value.as_json,
+ trend: trend.as_json
+ }
+ end
+
+ private
+ attr_reader :series
+
+ def create_trend(previous_value)
+ TimeSeries::Trend.new \
+ current: value,
+ previous: previous_value,
+ series: series
end
- def <=>(other)
- result = date <=> other.date
- result = value <=> other.value if result == 0
- result
+ def value_must_be_of_known_type
+ unless value.is_a?(Money) || value.is_a?(Numeric)
+ errors.add :value, "must be a Money or Numeric"
+ end
end
-
- private
- def validate_input
- raise ArgumentError, "Date is required" unless @date.is_a?(Date)
- raise ArgumentError, "Money or Numeric value is required" unless @value.is_a?(Money) || @value.is_a?(Numeric)
- end
end
diff --git a/app/views/shared/_trend_change.html.erb b/app/views/shared/_trend_change.html.erb
index f7d9d337..e71aef0d 100644
--- a/app/views/shared/_trend_change.html.erb
+++ b/app/views/shared/_trend_change.html.erb
@@ -1,7 +1,7 @@
<%# locals: { trend: } %>
<% styles = trend_styles(trend) %>
- <% if trend.direction == "flat" %>
+ <% if trend.direction.flat? %>
No change
<% else %>
<%= styles[:symbol] %><%= trend.value.is_a?(Money) ? format_money(trend.value.abs) : trend.value.abs %>
diff --git a/app/views/shared/_value_heading.html.erb b/app/views/shared/_value_heading.html.erb
index af55c34a..21cd1a6d 100644
--- a/app/views/shared/_value_heading.html.erb
+++ b/app/views/shared/_value_heading.html.erb
@@ -16,7 +16,7 @@
<% if trend.nil? %>
Data not available for the selected period
- <% elsif trend.direction == "flat" %>
+ <% elsif trend.direction.flat? %>
No change vs. prior period
<% else %>
diff --git a/lib/money.rb b/lib/money.rb
index cee14dc8..a9d55d37 100644
--- a/lib/money.rb
+++ b/lib/money.rb
@@ -1,73 +1,94 @@
class Money
- include Comparable
- include Arithmetic
+ include Comparable, Arithmetic
+ include ActiveModel::Validations
- attr_reader :amount, :currency
+ attr_reader :amount, :currency
- class << self
- def default_currency
- @default ||= Money::Currency.new(:usd)
- end
+ validate :source_must_be_of_known_type
- def default_currency=(object)
- @default = Money::Currency.new(object)
- end
+ class << self
+ def default_currency
+ @default ||= Money::Currency.new(:usd)
end
- def initialize(obj, currency = Money.default_currency)
- unless obj.is_a?(Money) || obj.is_a?(Numeric) || obj.is_a?(BigDecimal)
- raise ArgumentError, "obj must be an instance of Money, Numeric, or BigDecimal"
- end
-
- @amount = obj.is_a?(Money) ? obj.amount : BigDecimal(obj.to_s)
- @currency = obj.is_a?(Money) ? obj.currency : Money::Currency.new(currency)
+ def default_currency=(object)
+ @default = Money::Currency.new(object)
end
+ end
- # TODO: Replace with injected rate store
- def exchange_to(other_currency, date = Date.current)
- return self if @currency == Money::Currency.new(other_currency)
- rate = ExchangeRate.find_rate(from: @currency, to: other_currency, date: date)
- return nil if rate.nil?
- Money.new(@amount * rate.rate, other_currency)
+ def initialize(obj, currency = Money.default_currency)
+ @source = obj
+ @amount = obj.is_a?(Money) ? obj.amount : BigDecimal(obj.to_s)
+ @currency = obj.is_a?(Money) ? obj.currency : Money::Currency.new(currency)
+
+ validate!
+ end
+
+ # TODO: Replace with injected rate store
+ def exchange_to(other_currency, date = Date.current)
+ if currency == Money::Currency.new(other_currency)
+ self
+ elsif rate = ExchangeRate.find_rate(from: currency, to: other_currency, date: date)
+ Money.new(amount * rate.rate, other_currency)
end
+ end
- def cents_str(precision = @currency.default_precision)
- format_str = "%.#{precision}f"
- amount_str = format_str % @amount
- parts = amount_str.split(@currency.separator)
+ def cents_str(precision = currency.default_precision)
+ format_str = "%.#{precision}f"
+ amount_str = format_str % amount
+ parts = amount_str.split(currency.separator)
- return "" if parts.length < 2
-
- parts.last.ljust(precision, "0")
+ if parts.length < 2
+ ""
+ else
+ parts.last.ljust(precision, "0")
end
+ end
- # Basic formatting only. Use the Rails number_to_currency helper for more advanced formatting.
- alias to_s format
- def format
- whole_part, fractional_part = sprintf("%.#{@currency.default_precision}f", @amount).split(".")
- whole_with_delimiters = whole_part.chars.to_a.reverse.each_slice(3).map(&:join).join(@currency.delimiter).reverse
- formatted_amount = "#{whole_with_delimiters}#{@currency.separator}#{fractional_part}"
- @currency.default_format.gsub("%n", formatted_amount).gsub("%u", @currency.symbol)
+ # Use `format` for basic formatting only.
+ # Use the Rails number_to_currency helper for more advanced formatting.
+ def format
+ whole_part, fractional_part = sprintf("%.#{currency.default_precision}f", amount).split(".")
+ whole_with_delimiters = whole_part.chars.to_a.reverse.each_slice(3).map(&:join).join(currency.delimiter).reverse
+ formatted_amount = "#{whole_with_delimiters}#{currency.separator}#{fractional_part}"
+
+ currency.default_format.gsub("%n", formatted_amount).gsub("%u", currency.symbol)
+ end
+ alias_method :to_s, :format
+
+ def as_json
+ { amount: amount, currency: currency.iso_code }.as_json
+ end
+
+ def <=>(other)
+ raise TypeError, "Money can only be compared with other Money objects except for 0" unless other.is_a?(Money) || other.eql?(0)
+
+ if other.is_a?(Numeric)
+ amount <=> other
+ else
+ amount_comparison = amount <=> other.amount
+
+ if amount_comparison == 0
+ currency <=> other.currency
+ else
+ amount_comparison
+ end
end
+ end
- def to_json(*_args)
- { amount: @amount, currency: @currency.iso_code }.to_json
- end
+ def default_format_options
+ {
+ unit: currency.symbol,
+ precision: currency.default_precision,
+ delimiter: currency.delimiter,
+ separator: currency.separator
+ }
+ end
- def <=>(other)
- raise TypeError, "Money can only be compared with other Money objects except for 0" unless other.is_a?(Money) || other.eql?(0)
- return @amount <=> other if other.is_a?(Numeric)
- amount_comparison = @amount <=> other.amount
- return amount_comparison unless amount_comparison == 0
- @currency <=> other.currency
- end
-
- def default_format_options
- {
- unit: @currency.symbol,
- precision: @currency.default_precision,
- delimiter: @currency.delimiter,
- separator: @currency.separator
- }
+ private
+ def source_must_be_of_known_type
+ unless @source.is_a?(Money) || @source.is_a?(Numeric) || @source.is_a?(BigDecimal)
+ errors.add :source, "must be a Money, Numeric, or BigDecimal"
+ end
end
end
diff --git a/test/models/time_series/trend_test.rb b/test/models/time_series/trend_test.rb
index eb79913d..1ad73421 100644
--- a/test/models/time_series/trend_test.rb
+++ b/test/models/time_series/trend_test.rb
@@ -7,6 +7,7 @@ class TimeSeries::TrendTest < ActiveSupport::TestCase
assert_equal Money.new(50), trend.value
assert_equal 100.0, trend.percent
end
+
test "up" do
trend = TimeSeries::Trend.new(current: 100, previous: 50)
assert_equal "up", trend.direction
@@ -19,8 +20,8 @@ class TimeSeries::TrendTest < ActiveSupport::TestCase
test "flat" do
trend1 = TimeSeries::Trend.new(current: 100, previous: 100)
- trend3 = TimeSeries::Trend.new(current: 100, previous: nil)
- trend2 = TimeSeries::Trend.new(current: nil, previous: nil)
+ trend2 = TimeSeries::Trend.new(current: 100, previous: nil)
+ trend3 = TimeSeries::Trend.new(current: nil, previous: nil)
assert_equal "flat", trend1.direction
assert_equal "flat", trend2.direction
assert_equal "flat", trend3.direction
@@ -39,7 +40,7 @@ class TimeSeries::TrendTest < ActiveSupport::TestCase
end
test "empty" do
- trend =TimeSeries::Trend.new
+ trend = TimeSeries::Trend.new(current: nil, previous: nil)
assert_equal "flat", trend.direction
end
end
diff --git a/test/models/time_series_test.rb b/test/models/time_series_test.rb
index df0b6d40..e2bfc564 100644
--- a/test/models/time_series_test.rb
+++ b/test/models/time_series_test.rb
@@ -6,7 +6,7 @@ class TimeSeriesTest < ActiveSupport::TestCase
assert_equal Money.new(100), series.first.value
assert_equal Money.new(200), series.last.value
- assert_equal :normal, series.type
+ assert_equal "up", series.favorable_direction
assert_equal "up", series.trend.direction
assert_equal Money.new(100), series.trend.value
assert_equal 100.0, series.trend.percent
@@ -18,21 +18,18 @@ class TimeSeriesTest < ActiveSupport::TestCase
assert_equal 100, series.first.value
assert_equal 200, series.last.value
assert_equal 100, series.on(1.day.ago.to_date).value
- assert_equal :normal, series.type
+ assert_equal "up", series.favorable_direction
assert_equal "up", series.trend.direction
assert_equal 100, series.trend.value
assert_equal 100.0, series.trend.percent
end
- test "when nil or empty array passed, it returns empty series" do
- series = TimeSeries.new(nil)
- assert_equal [], series.values
-
+ test "when empty array passed, it returns empty series" do
series = TimeSeries.new([])
assert_nil series.first
assert_nil series.last
- assert_equal({ values: [], trend: { type: "normal", direction: "flat", value: 0, percent: 0.0 }, type: "normal" }.to_json, series.to_json)
+ assert_equal({ values: [], trend: { favorable_direction: "up", direction: "flat", value: 0, percent: 0.0 }, favorable_direction: "up" }.to_json, series.to_json)
end
test "money series can be serialized to json" do
@@ -41,16 +38,16 @@ class TimeSeriesTest < ActiveSupport::TestCase
{
date: 1.day.ago.to_date,
value: { amount: "100.0", currency: "USD" },
- trend: { type: "normal", direction: "flat", value: { amount: "0.0", currency: "USD" }, percent: 0.0 }
+ trend: { favorable_direction: "up", direction: "flat", value: { amount: "0.0", currency: "USD" }, percent: 0.0 }
},
{
date: Date.current,
value: { amount: "200.0", currency: "USD" },
- trend: { type: "normal", direction: "up", value: { amount: "100.0", currency: "USD" }, percent: 100.0 }
+ trend: { favorable_direction: "up", direction: "up", value: { amount: "100.0", currency: "USD" }, percent: 100.0 }
}
],
- trend: { type: "normal", direction: "up", value: { amount: "100.0", currency: "USD" }, percent: 100.0 },
- type: "normal"
+ trend: { favorable_direction: "up", direction: "up", value: { amount: "100.0", currency: "USD" }, percent: 100.0 },
+ favorable_direction: "up"
}.to_json
series = TimeSeries.new([ { date: 1.day.ago.to_date, value: Money.new(100) }, { date: Date.current, value: Money.new(200) } ])
@@ -60,12 +57,12 @@ class TimeSeriesTest < ActiveSupport::TestCase
test "numeric series can be serialized to json" do
expected_values = {
- values: [
- { date: 1.day.ago.to_date, value: 100, trend: { type: "normal", direction: "flat", value: 0, percent: 0.0 } },
- { date: Date.current, value: 200, trend: { type: "normal", direction: "up", value: 100, percent: 100.0 } }
- ],
- trend: { type: "normal", direction: "up", value: 100, percent: 100.0 },
- type: "normal"
+ values: [
+ { date: 1.day.ago.to_date, value: 100, trend: { favorable_direction: "up", direction: "flat", value: 0, percent: 0.0 } },
+ { date: Date.current, value: 200, trend: { favorable_direction: "up", direction: "up", value: 100, percent: 100.0 } }
+ ],
+ trend: { favorable_direction: "up", direction: "up", value: 100, percent: 100.0 },
+ favorable_direction: "up"
}.to_json
series = TimeSeries.new([ { date: 1.day.ago.to_date, value: 100 }, { date: Date.current, value: 200 } ])