Weight-loss calculator I have a table that holds a user's weight for specific days. In my weights_controller.rb index action I can show these weights - no problem. What I also want to show on the index is the user's current weight, his weight loss over time as well as the timeframe he achieved this weight loss.

Thus I created a weight_calculator.rb to do the needed calculations:

class WeightCalculator
  def initialize(profile, user)
     @profile = profile
     @user = user
  end

  def current_weight
    @current_weight = Weight.where(:user_id =&gt; @user.id).order(:day).last
  end

  def weight_loss
    @current_weight = Weight.where(:user_id =&gt; @user.id).order(:day).last
    @start_weight = Weight.where(:user_id =&gt; @user.id).order(:day).first
    @weight_loss = @current_weight.kilograms - @start_weight.kilograms
  end

  def timeframe
    @current_weight = Weight.where(:user_id =&gt; @user.id).order(:day).last
    @start_weight = Weight.where(:user_id =&gt; @user.id).order(:day).first
    @weight_loss_timeframe = @current_weight.day - @start_weight.day
    @weeks = @weight_loss_timeframe.to_i/7
  end
end


This is my weights_controller.rb index action:

  def index
    @weights = Weight.where(:user_id =&gt; current_user.id).order(day: :desc)
    @weight_loss = WeightCalculator.new(@profile, current_user).weight_loss
    @weeks = WeightCalculator.new(@profile, current_user).timeframe

    @result = {}
    Weight.where(:user_id =&gt; current_user.id).each do |weight|
      @result[weight.day] = weight.kilograms
    end

    @profile = Profile.where(:user_id =&gt; current_user.id)
  end


Everything works fine in this setup. However, there is clearly some repition in my weight_calculator.rb and I am asking myself what would be the best way to write this code to be DRY conform?
There is really quite a lot wrong here.

As a very simple improvement, why aren't you using ActiveRecord's associations? Why do you constantly do this...

Weight.where(:user_id =&gt; current_user.id).each ...
# ...
@current_weight = Weight.where(:user_id =&gt; @user.id).order(:day).last


Instead of this?

current_user.weights.each ...
# ... 
current_weight = @user.weights.order(:day).last


That alone would drastically improve the readability of your code.

You can also make the loop in your index action more idiomatic:

@result = current_user.weights.map do
  [weight.day, weight.kilograms]
end.to_h


You also do some weird things in WeightCalculator with instance vairables. Your methods all seem to both return a value, and assign that same value to an instance vairable that is never used. There is no reason to do this:

def current_weight
  # ...
  @current_weight = &lt;calculation&gt;
end


Instead, you should just return the calculation, or memoize it via ||=:

def current_weight
  # ...
  @current_weight ||= &lt;calculation&gt;
end


If I were working on this code, I would refactor it in the following way to take advantage of the above few points:

class WeightCalculator
  def initialize(profile, user)
     @profile = profile
     @user = user
  end

  def weights
    Weight.where(:user_id =&gt; @user.id)
    # or this, doesn't matter:
    @weights ||= @user.weights
  end

  def start_weight
    @start_weight ||= weights.order(:day).first
  end

  def current_weight
    @current_weight ||= weights.order(:day).last
  end

  def weight_loss
    current_weight.kilograms - start_weight.kilograms
  end

  def timeframe
    (current_weight.day - start_weight.day).to_i / 7
  end
end




Your index action is also pretty broken.

First, You try to access @profile to pass it to your WeightCalculator before you set it, and then later you set it to Profile.where(...), which returns an association, not a specific profile. Again, using associations, you probably just want

@profile = current_user.profile


Second, you query Weights all over the place, over and over. You constantly go through the effort of typing out Weight.where(...) instead of just doing it once and reusing the returned set of weights.  You even make multiple calculators for absolutely no reason, and inside the calculator you don't even reuse weights! Just do things once, there is no reason to continually run the same queries over and over, even if they are being cached by ActiveRecord.

calculator = WeightCalculator.new(@profile, current_user)
@weights = calculator.weights

@result = @weights.map do |weight|
  [weight.day, weight.kilograms]
end.to_h