Refactoring #1
Refactoring #1
Conversation
rafasoares
left a comment
rafasoares
left a comment
There was a problem hiding this comment.
Some comments on the Data::Base class
lib/trustly/data/base.rb
Outdated
| ret = data.each_with_object([]) do |element, acc| | ||
| processed_element = vacuum(element) unless element.nil? | ||
| next if processed_element.nil? | ||
|
|
||
| acc.push(processed_element) | ||
| end |
There was a problem hiding this comment.
This looks like it could be replaced by data.compact.
Or filter_map, since you're targeting Ruby 2.7+
There was a problem hiding this comment.
not exactly, compact doesn't get rid of empty arrays or hashes, while this recursive vacuum does something like:
=> cleaner.vacuum([1,2,[],{a: {}}])
=> [1,2]
There was a problem hiding this comment.
Yeah, it was an oversight, but filter_map would cover that, since you could just recursively call vacuum in it.
lib/trustly/data/base.rb
Outdated
|
|
||
| acc.push(processed_element) | ||
| end | ||
| ret.length.zero? ? nil : ret |
There was a problem hiding this comment.
| ret.length.zero? ? nil : ret | |
| ret.empty? ? nil : ret |
Or ret.presence if you're willing to add a dependency to active_support/core_ext.
lib/trustly/data/base.rb
Outdated
| ret = data.each_with_object({}) do |(key, element), acc| | ||
| processed_element = vacuum(element) unless element.nil? | ||
| next if processed_element.nil? | ||
|
|
||
| acc[key] = processed_element | ||
| end | ||
| ret.length.zero? ? nil : ret |
lib/trustly/data/base.rb
Outdated
| def vacuum(data) | ||
| case data | ||
| when Array then vacuum_array_data(data) | ||
| when Hash then vacuum_hash_data(data) | ||
| else data | ||
| end | ||
| end |
There was a problem hiding this comment.
I feel like this entire "vacuum" logic belongs in a separate class/module like DataCleaner or something like that. It can still be a child of the trustly/data module if it doesn't apply anywhere else, but I'd rather have it outside the Base class.
There was a problem hiding this comment.
It could very well be a module that gets included in the Base class, but it would still be separate in the code.
lib/trustly/data/base.rb
Outdated
| def stringify_hash(hash) | ||
| hash.each_with_object({}) do |(k, v), acc| | ||
| acc[k.to_s] = v.is_a?(Hash) ? stringify_hash(v) : v | ||
| end | ||
| end |
There was a problem hiding this comment.
Missed case when v is array
There was a problem hiding this comment.
I should probably rename it to deep_stringify_keys, but right, arrays are not handled here :)
There was a problem hiding this comment.
Sometimes I wish active_support/core_ext was part of the Ruby stdlib 🙃
Added rubocop and specs (with simplecov), faraday is used for web requests, Ruby 2.7 is set as a lower bound required version.