← Back to Upcase

How do I refactor this method?


(Charlieanna) #1

Codeclimate says it has a lot of duplication.

 def award_points_to_user_if_activity_completed(user_activity)
    if user_activity.completed?
      if user_activity.total_steps_covered >= user_activity.challenge.number_of_steps &&  user_activity.total_steps_covered < (user_activity.challenge.number_of_steps + 100000) && user_activity.level == "Basic"
        points = user_activity.total_steps_covered
         current_user.award_points(points)
        return "Congrats for finishing the challenge. You just got #{points} points. Now pick another challenge."
      end
      if user_activity.total_steps_covered >= (user_activity.challenge.number_of_steps + 100000) && user_activity.level == "Basic"
        points = user_activity.total_steps_covered
        current_user.award_points(points)
        return "Congrats. You not only finished the basic challenge but also overcame the advanced challenge. Make sure you choose a better challenge to earn more points."
      end
      if user_activity.total_steps_covered >= user_activity.challenge.number_of_steps && user_activity.level == "Advanced"
        points = 2*(user_activity.total_steps_covered - (user_activity.challenge.number_of_steps - 100000)) + user_activity.challenge.number_of_steps - 100000
         current_user.award_points(points)
        return "Congrats for finishing the challenge. You just got #{points} points. Now pick another challenge."
      end
      if user_activity.total_steps_covered < (user_activity.challenge.number_of_steps - 100000)  && user_activity.level == "Advanced"
        points = user_activity.total_steps_covered
        current_user.award_points(points)
        return "Oops. You missed your basic target. I think you should go for a lower target to get points and badges. It is ok. People often get stuck based on their lifestyls. That is why we are here. "
      end

      if user_activity.total_steps_covered < user_activity.challenge.number_of_steps  && user_activity.level == "Basic"
        points = user_activity.total_steps_covered
        current_user.award_points(points)
        return "Oops you missed the target. Based on your lifestyle you can choose one that suits you better."
      end
      if user_activity.total_steps_covered >= (user_activity.challenge.number_of_steps - 100000)  && user_activity.total_steps_covered < user_activity.challenge.number_of_steps && user_activity.level == "Advanced"
        points = user_activity.total_steps_covered
        current_user.award_points(points)
        return "Congrats. You finished the basic challenge but not the advanced challenge. It is ok. But this time we want to make sure you go for the advanced challenge first."
      end
    end
  end

(Guirec Corbel) #2

WHO! There is a ton of refactoring to do. First of all, your method is definitevly to long. I think you must to move all conditions in the user_activity class and call it like this :

def award_points_to_user_if_activity_completed(user_activity)
  if user_activity.completed?
    if user_activity.basic_challenge_ended?
      #..
    end

    if user_activity.add_challenge_ended?
      #...
    end

    if user_activity.advenced challenge_ended?
      #...
    end
    #... other condition with a explicit name
  end
end

You probably must to create private method like UserActivity#advanced? or UserActivity#base?.

After that, you have points = user_activity.total_steps_covered except one time. The points are all the sames except on time. You can refactor it by doing a method UserActivity#winned_points based on the conditions and you have defined. And you is like this : current_user.award_points(user_activity.winned_points).

After that, you should use conditions only for have the text. Again, I think you can move some logic in the model and add a name. For example, user_activity can give full_complete depending of the comdition. After that, you can use I18n like this I18n.t("messages.#{user_activity.name}").

If you follow my suggestion, the code will look like this:

def award_points_to_user_if_activity_completed(user_activity)
  if user_activity.completed?
    current_user.award_points(user_activity.winned_points)
    I18n.t("messages.#{user_activity.name}")
  end
end

There is probably some refactoring to again. Take a look at this video : http://tx.pignata.com/2013/04/mwrc-code-smells-talk.html.