HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

How would you ruby-fy this non-AR model class for Ruby on Rails app?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thishowyounonappwouldrailsforrubyclass

Problem

This is a class from my rails application. It is used as non-database model in my RoR application and uses syntax similar to AR design pattern. I wonder is there a way to make this look more like Ruby code?

class Role

  def self.all
    role_list = Array.new

    r1 = Role.new
    r1.id = 1
    r1.symbol = :owner
    r1.name = "Owner"
    r1.has_all_permissions = true
    role_list  permission.id, :role_id => self.id)

    if permission_roles.size == 0
      permission_role = PermissionRole.new
      permission_role.permission_id = permission.id
      permission_role.role_id = self.id
      return permission_role.save
    end

    true
  end

  def has_permission?(symbol)
    permission = Permission.find_by_symbol(symbol)

    if permission == nil
      raise StandardError, "Permission does not exist"
    end

    permission_roles = PermissionRole.where(:permission_id => permission.id, :role_id => self.id)

    if permission_roles.size > 0
      return true
    else
      return false
    end
  end

  attr_accessor :id
  attr_accessor :symbol
  attr_accessor :name
  attr_accessor :has_all_permissions

end

Solution

For starters:

def initialize(attributes = {})
  attributes.each do |attr, value|
    self.send "#{attr}=", value
  end
end


Would allow you to turn:

r1 = Role.new
r1.id = 1
r1.symbol = :owner
r1.name = "Owner"
r1.has_all_permissions = true
role_list << r1


into:

role_list  1,
                       :symbol => :owner,
                       :name => "Owner",
                       :has_all_permissions => true}


which is much more ActiveRecord-like.

Storing the roles_list in a global variable might be acceptable for your application and would also allow you to have:

def self.create(attributes = {})
  $roles_list << Role.new(attributes)
end


and:

def self.all
  return $roles_list if $roles_list

  # rest of code to init $roles_list with default roles
end


for further ActriveRecord-ness

Then:

def self.find(id)
  roles = Role.all

  roles.each do |role|
    if role.id == id
      return role
    end
  end

  nil
end


could be re-written like so:

def self.find(id)
  Role.all.find{|r| r.id == id}
end


That is because Array has its own #find method. The same refactoring could be applied to self.find_by_symbol.

As a final note, I would move the attr_accessor lines to the top.

Hope I helped :-)

Code Snippets

def initialize(attributes = {})
  attributes.each do |attr, value|
    self.send "#{attr}=", value
  end
end
r1 = Role.new
r1.id = 1
r1.symbol = :owner
r1.name = "Owner"
r1.has_all_permissions = true
role_list << r1
role_list << Role.new {:id => 1,
                       :symbol => :owner,
                       :name => "Owner",
                       :has_all_permissions => true}
def self.create(attributes = {})
  $roles_list << Role.new(attributes)
end
def self.all
  return $roles_list if $roles_list

  # rest of code to init $roles_list with default roles
end

Context

StackExchange Code Review Q#3934, answer score: 3

Revisions (0)

No revisions yet.