List enrolled users & show checkmark if all activities are complete

I'm a beginner with rails working on a rails 4 project to use with my students. Users enroll in lessons; on enrollment, four activities are created for each user (expositions, scrambled, dictations, and spellings). Each of these activities has a number of words which are set to false in the db upon creation, and change to true as the user rewrites them correctly. At the bottom of each lesson's view I'm displaying a list of enrolled users. If the user completes all four activities, I'm displaying a font-awesome checkmark icon.

I'm noticing as more students enroll in each lesson that loading times for the lesson show view are increasing--most probably because my setup to find whether each user completed the lessons is inefficient.

In my lessonss controller I wrote helper methods to list the lesson's enrolled users, and to generate arrays with users who completed each of the lessons activities. In the view page, I'm testing whether each of the listed users completed all four activities in order to add the "completed" checkmark to his or her name.

As things are right now the code does what it's supposed to, but I'd love to learn and improve the student-user's experience. Any feedback regarding how I can improve this process will be well received.

In lessons controller:

```
helper_method :enrolled_users
def enrolled_users
lesson = Lesson.find(params[:id])
enrollments = lesson.enrollments
enrolled_users = enrollments.map { |enrollment| enrollment.user }
end

helper_method :completed_scram
def completed_scram
lesson = Lesson.find(params[:id])
completed_scram = enrolled_users.map do |user|
current_enrollment = lesson.enrollment_for(user)
all_unscramble = current_enrollment.scrambled_words
user if all_unscramble.all? { |scrambled| scrambled.completed == true }
end
end

helper_method :completed_expos
def completed_expos
lesson = Lesson.find(params[:id])
completed_expos = enrolled_users.map do |user|

Before looking at the code, there's a simple and immediate option for speeding things up: When a user completes an activity (or a part of one), check the user's other activities, and set an overall completed flag on the user's enrollment accordingly. That way, you only need to check completion for one user at a time and only in connection with an activity update (whereas right now, you check completion for everyone, all the time). Whenever you need to display the list, you'll already have completed boolean values to use.

So that's one way to go.

Now, looking at the code, the first thing I see is a lot of duplication.

Second thing I see is that you unnecessarily abbreviate stuff. Ironic that "spelling" is spelt "spel"... Really, just write what you mean, and mean what you write. There's no reason whatsoever to abbreviate.

Anyway, duplication: The only thing that differs from method to method is the activity name.

So you could get rid of all the completed_* method, and just do this:

def users_with_completed_activity(name)
  lesson = Lesson.find(params[:id])
  enrolled_users.map do |user|
    current_enrollment = lesson.enrollment_for(user)
    user if current_enrollment.send(name).all?(&:completed?)
  end
end

Note that this:

all? { |x| x.completed == true }

is just checking "is true true?", so it's the same as:

all? { |x| x.completed }

which, with Ruby's shortcuts, is the same as:

all?(&:completed)

However there's another weird thing going on. You load the Lesson record. Then you call enrolled_users (which also loads the Lesson, by the way), which finds users from their enrollments, and then, for each user, you go back and get their Enrollment.

So:

-
Fetching the Lesson should probably be extracted into a before_action (Rails' scaffolded controllers include examples of exactly this)

-
You're getting the enrollments to get the users to get their enrollments... Notice the runaround there?

Sidestepping that, you get something like (assuming Lesson's been set in a before_action):

def users_with_completed_activity?(name)
  @lesson.enrollments.map do |enrollment|
    enrollment.user if enrollment.send(name).all?(&:completed?)
  end
end

Of course, I'm not exactly sure why you want an array with a mix of User records and nils. All it means is that you make it vastly more complicated in the view because you have to use include?. And that's slow, since it has to look through the array every time. So you have a system that'll get exponentially slower the more users you add.

It'd make more sense to send back an array of user/completed pairs:

def users_with_completed_activity?(name)
  @lesson.enrollments.map do |enrollment|
    [enrollment.user, enrollment.send(name).all?(&:completed?)]
  end
end

which you can then step through like:

users_with_completed_activity(:spelling).each do |user, completed|
  # do something
end

Getting a combined completed-all-activities boolean I'll leave to the reader (since this is all just stop-gap improvements anyway - read on).

I'd also ask why the Enrollment is also responsible for tracking activity completion. It's not necessarily wrong, but it doesn't seem the best fit either. Whether or not a user completes an activity doesn't really affect their enrollment. Whether or not you do well, you're presumably still enrolled. But with this structure, that's what happens. So that's a little odd.

Lastly, you have a call to uniq because - for some reason - it seems it's possible for users to be enrolled multiple times? I can't see any other reason. But why would someone be enrolled multiple times? It doesn't make sense in the real world, nor as a software model. How do you know which enrollment to update when an activity is completed? Which enrollment to check?

Now, none of this is likely to make things go much faster. The trouble is still that you're simply doing too much each request. As mentioned in the beginning, the simplest solution would probably be to update a flag on the enrollment, when a user completes an activity. Then that's all you need to check. It's not something that changes very often, so why go through all the motions of figuring it out every time? It's like a bank account balance: If every time you wanted to check your balance, the bank had to sum up all deposits and withdrawals, things would grind to a halt. So it stores the current balance and keeps it up to date.

There are many other ways to go about this (doing more processing in the database layer, doing different data modelling, in-memory caching, etc. etc.) but the simplest one is simply to store the things you need to access often.

StackExchange Code Review Q#140499, answer score: 2