2
\$\begingroup\$

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?

\$\endgroup\$

migrated from stackoverflow.com Jun 7 '16 at 15:33

This question came from our site for professional and enthusiast programmers.

5
\$\begingroup\$

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

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 = <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
\$\endgroup\$
  • \$\begingroup\$ 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 :-/) \$\endgroup\$ – Oliver Jun 7 '16 at 16:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.