# 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 => @user.id).order(:day).last
end

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

def timeframe
@current_weight = Weight.where(:user_id => @user.id).order(:day).last
@start_weight = Weight.where(:user_id => @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 => 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 => current_user.id).each do |weight|
@result[weight.day] = weight.kilograms
end

@profile = Profile.where(:user_id => 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 => current_user.id).each ...
# ...
@current_weight = Weight.where(:user_id => @user.id).order(:day).last


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


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 = <calculation>
end


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

def current_weight
# ...
@current_weight ||= <calculation>
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 => @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

• Wow! Thanks a lot for this massive input. I will work through it. However, using @weights ||= current_user.weights results in an "undefined local variable or method `current_user' for #<WeightCalculator:0x007f834890d170>" error. Using @weights ||= @user.weights works. (Sorry btw. for my crappy code. Just started coding two months ago :-/) Jun 7 '16 at 16:06