Class/model hierarchy for a school-management application

Question

Please find how I would model a system that had teachers, students, classes and parents. Instead of creating a model for each user type, I created only two models, User and Profile. Each user can have multiple profiles and each profile can have a unique profile_type.

I would love to get some feedback on my proposed architecture.

class User < ActiveRecord::Base
  has_many :profiles
  has_many :user_klasses
  has_many :klasses, through: :user_klasses

  def get_student_classes
    student_profile = profiles.select { |p| p.profile_type == 'student' }.first
    return [] if student_profile.nil?

    return klasses.select { |k| k.profile_id == student_profile.id }
  end

  def get_teacher_classes
    teacher_profile = profiles.select { |p| p.profile_type == 'teacher'}.first
    return [] if teacher_profile.nil?

    return klasses.select { |k| k.profile_id == teacher_profile.id }
  end

end

class Klass < ActiveRecord::Base
  has_many :user_klasses
  has_many :users, through: :user_klasses
  belongs_to :teacher, class_name: 'Profile', -> { where profile_type: 'teacher' },
                        dependent: :destroy
end

class UserKlasses < ActiveRecord::Base
  belongs_to :user
  belongs_to :klass
end

class Profile < ActiveRecord::Base
  belongs_to :user
end

Answer 1

You mention that you would rather not create extra models but I would consider creating a model for Teacher, Student and for Parent. It is not really that much more work but will probably keep the separation between the concepts much cleaner in the long run (as per Damien's comments). I personally would also use names like TeacherRole and ParentRole or TeacherFacet.

I would also consider keeping the cardinality as has_one unless you need to deal with the possibility that a user needs more than one teacher profile (possibly because they teach in more than one places) or more than one parent profile (maybe they have multiple children).

You obviously haven't shown your complete data model but it looks wrong that each user is linked to classes through the user_klasses relation but then you query classes using a profile_id field on the Klass. Firstly you probably want to query the profile_id on the UserKlass model (the profile_id on Klass looks like it refer to the teacher's profile) but you should probably put the klasses relation on the Profile model (or whatever you call it).

One other thing I noticed here:

  belongs_to :teacher, class_name: 'Profile', -> { where profile_type: 'teacher' },
                    dependent: :destroy

You probably don't want to remove the teacher record if a class is deleted, they may be teaching other classes after all. However you probably want to delete UserKlass if the Klass (or User for that matter) is deleted. Likewise it would make sense to delete the Profile record when a User is deleted.

Answer 2

Though there is nothing strictly wrong with your proposed design, I would say be wary of the additional data and behaviour that can be tied to students, teachers, and parents.

What you want to avoid is:

class Profile
  def general_method
    if profile_type == 'student'
      # do this
    elsif profile_type == 'parent'
      # do this
    elseif profile_type == 'teacher'
      # do this
    end
  end
end

One way of resolving this would be single table inheritance, but that would only resolve the behavioural pollution. The data would still be in the same table, which can develop into a messy schema as your models grow apart in structure and behaviour.

There is nothing wrong with jumping to something else AFTER that happens, but you could spend a little more time designing upfront to avoid it.

It comes down to do you want a messy database or a messy object model? Are a few redundant columns that bad? For example, having teacher's salary and student's allowance in the same table? The alternative is separating the table, but then you have duplicate columns on each table such as first/last name, address.

As you talk it through like this you begin to understand that it probably IS worth having a few potentially redundant columns in the table just to avoid those duplicate columns on multiple tables. Having said that, DB design is not my day-job, so keep looking around and see what works for you!

---

Aside from the design, let's talk a little about the code itself.

  def get_student_classes
    student_profile = profiles.select { |p| p.profile_type == 'student' }.first
    return [] if student_profile.nil?

    return klasses.select { |k| k.profile_id == student_profile.id }
  end

First problem is you have named your method get_student_classes. In Ruby we would simply call this student_classes. Forget getter and setter naming conventions. You get by calling the method itself, you set by assigning (student_classes = etc).

Second problem above is that you've used select. This will fetch all records from the database and then loop over them. Sure, it doesn't matter when there are only 10 or so records, but it becomes much less efficient as the data piles up. To prevent that, use a query. On top of that, use a scope to build that query.

The third problem is that you are performing two queries to get what you want (klasses.select).

What you want to do is run a single query to get the classes you want without the need to loop over and filter them in Ruby.

Hate to hit you with an anti-climax but I've run out of steam tonight :) I would advise you perhaps post it on SO. There is surely a better way of querying for that data.