How can I make BalanceService PORO I wrote better of any code smell? [closed]

Question

I wrote this method which I think is not effective as it doesn't respond to the missing method when I use the PORO Class.

BalanceService

class BalanceService
  INITIAL_MULTIPLIER = 1
  HUMUS_EFFECT = 1.3

  instance_methods.each do |m|
    undef_method m unless m.to_s =~ /^__|method_missing|respond_to?/
  end

  def initialize(field_data)
    @field_data = field_data
  end

  def method_missing(name, *args)
    super unless respond_to?(name) # If I remove this line it works.

    @field_data.map do |field|
      previous_value = nil
      multiplier = INITIAL_MULTIPLIER

      field[:humus_balance] = field[:crops].inject(0) do |balance, crop|
        (crop[:crop][:value] == previous_value) ? multiplier = multiplier * HUMUS_EFFECT : previous_value = crop[:crop][:value]
        balance + (crop[:crop][:humus_delta] * field[:area] * multiplier)
      end

      field
    end
  end

  def respond_to?(method)
    @field_data.respond_to?("get_#{method}_info") || super
  end
end

When I call it in the fetch_field method below I get undefined method "field_data" for #<BalanceService:0x00007fda9c190e28>. However, if I remove super unless respond_to?(name) from BalanceService class above, it works. I indeed want BalanceService to respond to missing methods.

FieldService

class FieldsService
  include Singleton

  def fetch_fields
    value =  BalanceService.new(field_data) 
    value.field_data 
  end

  private

  def field_data
    # some JSON 
  end
end

Answer 1

You balance class does not have a field_data method.

def fetch_fields
  value =  BalanceService.new(field_data) 
  value.field_data # this method does not exist
end

So you could implement a attr_reader :field_data but that would be undefined with undef_method (which I don't really get what this is for).

This should work

class BalanceService
  INITIAL_MULTIPLIER = 1
  HUMUS_EFFECT = 1.3

  instance_methods.each do |m|
    undef_method m unless m.to_s =~ /^__|method_missing|respond_to?/
  end

  attr_reader :field_data

  def initialize(field_data)
    @field_data = field_data
  end

  def method_missing(name, *args)
    super unless respond_to?(name) # If I remove this line it works.

    @field_data.map do |field|
      previous_value = nil
      multiplier = INITIAL_MULTIPLIER

      field[:humus_balance] = field[:crops].inject(0) do |balance, crop|
        (crop[:crop][:value] == previous_value) ? multiplier = multiplier * HUMUS_EFFECT : previous_value = crop[:crop][:value]
        balance + (crop[:crop][:humus_delta] * field[:area] * multiplier)
      end

      field
    end
  end

  def respond_to?(method)
    @field_data.respond_to?("get_#{method}_info") || super
  end
end

BalanceService.new({}).field_data

The meta programming here makes it incredible hard to understand what this class is supposed to do.

Answer 2

Your "method missing" implementation responds to every method with the same result, ignoring the method name. Essentially you're letting someone call BalanceService.new(data).this_method_does_not_exist == BalanceService.new(data).neither_does_this_one and get true.

I cannot imagine a reason to do this. Do you mean to call @ field_data .get_#{method}_info somewhere?

Also, your method call mutates the input data (destructively) which can cause hard-to-understand bugs.

I'd recommend restructuring to something like:

class BalanceService
  INITIAL_MULTIPLIER = 1
  HUMUS_EFFECT = 1.3
  
  attr_reader :crops, :area
  def initialize(crops:, area:)
    # Using kwargs throws an error if the expected values are not
    # provided or if you pass in anything unexpected.
     @crops = crops
     @area = area
  end

  # This method is still way too complex
  def calculate_humus_balance
    previous_value = nil
    multiplier = INITIAL_MULTIPLIER
    crops.inject(0) do |balance, crop|
      # I'd normally pull the inner loop code out into a method...
      # but we can't easily do this because of 
      # the side-effects on previous_value and multiplier 
      crop_value, humus_delta = crop[:crop].values_at(:value, :humus_delta)
      if crop_value == previous_value
        multiplier = multiplier * HUMUS_EFFECT
      else
        previous_value = crop_value
      end
      balance + (humus_delta * area * multiplier)
    end
  end
end

then call the service like

class FieldsService
  include Singleton

  def fetch_fields
    field_data = build_field_data
    field_data.merge(humus_balance: BalanceService.new(**field_data).calculate_humus_balance)
  end

  private

  def build_field_data
    # some JSON 
  end
end
```