Creating a percentage complete method for a form

Question

I am a relatively new Rails developer and doctor doing research. In my Rails based research projects I often have questionnaires for which I provide a "% complete" parameter.

Here is an example. You can see that each of the score attributes is given a value and then I can create a %. It looks awful and very un-Ruby-like. I would be keen once and for all to get an experts view and give me some steerage on how this could be cleaned up.

This method is in the Patient class:

 def percent_complete(user, patient)
   score = Score.where(user_id: user.id, patient_id: self.id).first
  if score.nil?
    return "0"
  end
  @status = 0
   score_array = [score.dx1, score.dxcon1, score.db1, score.dbcon1, score.biopsy, score.mgt]
   score_array.each do | element|
    if element == score.dx1
      @status = @status + 20
    elsif element == score.db1
      @status = @status + 20
    elsif element == score.dxcon1
      @status = @status + 20
    elsif element == score.dbcon1
      @status = @status + 20
    elsif element == score.biopsy
      @status = @status + 10
    elsif element == score.mgt
      @status = @status + 10
    end
   end
  @status
 end

Each Score belongs to a Patient (the Patient has many scores). Each Score belongs to a User (a User has many scores).

score.dx1, score.dxcon1, score.db1, score.dbcon1, score.biopsy, score.mgt

These are the Score class attributes:

• dx1 = diagnosis1 (string)
• dxcon1 = diagnostic confidence1 (integer)
• db1 = disease behaviour1 (string)
• dbcon1 = confidence on disease behaviour (integer)
• biopsy = should you biopsy? (boolean)
• mgt = management (string)

I am now trying something like this:

 def percent_complete(user, patient)
    score = Score.where(user_id: user.id, patient_id: self.id).first
    if score.nil?
      return "0"
    end
    score_completeness = [score.dx1, score.dxcon1, score.db1, score.dbcon1, score.biopsy, score.mgt]
    @status = 0
    score_completeness.each do | field |
       @status += (100/6.0) unless field.blank?  
    end
   @status
 end

Answer 1

Looking only at your last block:

• Why return the string "0" instead of the number?
• The hard coded array is cryptic, and a potential maintenance hassle. I'd recommend moving it to a method on the Score model to abstract things.
• The hardcoded 100/6.0 is basically a magic number. It'd be better to get the length of whatever array you're working with than assume it's always 6.
• Why is @status an instance variable? Your method is meant to return something. If it returns at the first return line, you never even set @status to anything, which'll lead to errors later on, if you expect it to be set. Conversely, if you don't expect it to be set, or don't intend to use it, why set it?

I imagine you could do something like this:

def percent_complete(user, patient)
  score = Score.where(user_id: user.id, patient_id: self.id).first
  return 0 if score.nil? || score.steps.empty?
  100.0 * score.completed_steps.length / score.steps.length
end

where Score#steps and Score#completed_steps look like:

def steps
  [dx1, dxcon1, db1, dbcon1, biopsy, mgt]
end

def completed_steps
  steps.reject(&:blank)
end

Answer 2

There are some things that are a bit unclear when it comes to the associations between User and Patient, so I'll assume that this is how everything is setup:

class User
  has_many :scores
  has_one :patient
end

class Score
  belongs_to :patient
  belongs_to :user
end

class Patient
  has_many :scores
  belongs_to :user
end

Here are some things you could improve:

• Separation of Concerns: score calculation feels like it shouldn't be a Patient class concern. Instead it should happen at the Score class. Also if the calculation of score turns out to be more complex in the future, it should be extracted to a separate class for Granularity.
• Single Responsibility Principle: each method should do one thing. The Patient#percent_complete is currently doing lots of stuff, it knows to much. It should request the percentage from the score and just that.
• Better naming: come up with better method names that reflect exactly what each method does. For example #percent_complete isn't that descriptive. What does it stand for? How can a patient be complete? This are just questions I asked myself when I saw your code.

Regarding code syntax, you could do the following:

• Don't use self for reading attributes. Using just the attribute works. self should be used when you are writing. Take a look here to see examples on when to use (or not) self.
• Since each patient has_many :scores you can use the scores method in your code and get the scores scoped to the current patient instance. Take a look at Rails association methods for more information on which methods become available when defining associations.
• Assuming that each patient belongs_to a User, you can use the user_id attribute in order to save yourself a query when requesting the score.
• Since percent_complete is an Patient instance method, why do you require patient as an argument. It's not used anywhere in the method.
• Encapsulation: do not expose methods that you don't need to be in the public interface.

You could do something like this:

class Patient
  has_many :scores
  belongs_to :user
end

class Score
  belongs_to :patient
  belongs_to :user

  def progress
    100 * completed_attrs_count / attrs_for_completion.count.to_f
  end

  private

  def attrs_for_completion
    %i[dx1 dxcon1 db1 dbcon1 biopsy mgt]
  end

  def completed_attrs_count
    attrs_for_completion.reject do |attr|
      public_send(attr).blank?
    end.count
  end
end

class Patient
  has_many :scores
  belongs_to :user

  def percent_complete
    score && score.progress || 0
  end

  private

  def score
    scores.where(user_id: user_id).first
  end
end