Can i make Rails this code more elegant?

Question

I have this class:

class User < ActiveRecord::Base

  attr_accessor :name, :email, :hr_id

  def initialize(attributes = {:name => "Missing Name",:email => "Missing Email",:hr_id => "Missing HR ID"})
      @name = attributes[:name]
      @email = attributes[:email]
      @hr_id = attributes[:hr_id]
  end

  def print_employee
    "Employee No: #{@hr_id} - #{@name} (#{@email})"
  end
end

And i use it like this:

def help
    employee = User.new
    employee.name = "Dude"
    employee.email  = "Dude@gmail.com"
    employee.hr_id = "129836561"
    @employee = employee.print_employee
end

My question is, how can i make the code in help shorter and more elegant?

I tried:

employee = User.new('dude','dude@gmail.com','129836561')
@employee = employee.print_employee

But i got errors.

am I dreaming or you're really overriding the initialize method of ActiveRecord? — apneadiving, Mar 24 '14 at 08:45
create a builder to set default values etc, dont touch the core class — apneadiving, Mar 24 '14 at 08:45
I am new to rails and this is part of the learning process, but i am initializing the class User, which i created. Unless i am missing something? — , Mar 24 '14 at 08:47
DO NOT monkey patch rails classes... Rails is not your app, nor your code, if you expect it to work, be kind with it — apneadiving, Mar 24 '14 at 08:48
Can you elaborate? From my understanding, i created new model & controller named User, and the 'initialize' method referring to the class `User` that i created, not the core class. Unless as i said, i'm totally wrong - in which case i'll appreciate some explanadtion — , Mar 24 '14 at 08:50
http://blog.dalethatcher.com/2008/03/rails-dont-override-initialize-on.html — apneadiving, Mar 24 '14 at 08:54
but, really... for default values etc, use a builder, dont spoil your class with this — apneadiving, Mar 24 '14 at 08:55
@apneadiving Perhaps it may be helpful to explain to the original poster a little more than you originally did, because you come off as a little blunt, snarky, and mean. — , Mar 24 '14 at 09:07
I'm sorry if I didn't click on the link you posted. If I had, I wouldn't have bothered writing my answer. For future comments like that, can we please put something that summarizes the link? A simple "use `after_initialize` as stated in this link" should suffice. — jvnill, Mar 24 '14 at 15:43
I think that overriding the initialize method is a bad idea here, if you want to give default values to your model, try this post http://stackoverflow.com/questions/1550688/how-do-i-create-a-default-value-for-attributes-in-rails-activerecords-model — Uelb, Mar 24 '14 at 08:58

jvnill · Accepted Answer · 2014-03-24T15:53:12.350

3

You are looking for after_initialize and/or after_find callbacks. See the docs

after_initialize :set_default_values

private

def set_default_values
  self.name ||= 'Missing name'
end

NOTE

As apneadiving has mentioned, this is not the correct way to approach your problem but I think this is the best answer to your question how to make the code more elegant. For best practice, search for service classes like apneadiving's answer and how to use them in your controller to set default values.

edited Mar 24 '14 at 15:53

answered Mar 24 '14 at 08:57

jvnill

29,479
4
83
86

yeah thats what I told him in comments but... after_initialize is run for every stuff retrieved from db which not useful (and can lead to perf issues) + its not the role of the model to know its default value, this logic must be extracted – apneadiving Mar 24 '14 at 09:00
1

see: http://en.wikibooks.org/wiki/Ruby_on_Rails/ActiveRecord/Callbacks#cite_note-performance-1 – apneadiving Mar 24 '14 at 09:02
this is an error to proceed this way... easy for sure but not correct – apneadiving Mar 24 '14 at 09:13
@apneadiving why? The model is responsible for its data, I can't figure out a better place. In the controller? And then you implement an API, you copy-paste initialization values into the API controller? – pdu Mar 24 '14 at 09:36
@pduersteler builders are a common design pattern. Dive into the existing litterature. I'm just following best practices and recommend people not to use callbacks spoiling perfs and codebase – apneadiving Mar 24 '14 at 09:59
@pduersteler just think about it: isnt is absurd to execute code each time you build a new instance AND each time you get instances from db? It should be done only once in a builder. I did quote the after_initialize in my comments before this answer was posted, but its no decent solution – apneadiving Mar 24 '14 at 10:02
yep. i totally agree with you @apneadiving. I've never needed to use `after_initialize` and `after_find`. Although I think this is the best answer to his question which is how to make the code more elegant. This is better than monkey patching the `initialize` method. – jvnill Mar 24 '14 at 15:38
Sorry but elegant doesnt mean clean paint over rusted metal, which is what after_initialize does in my opinion, thats why I disnt submit this as an answer – apneadiving Mar 24 '14 at 16:02

apneadiving · Answer 2 · 2014-03-24T16:16:53.140

1

Do this:

class UserBuilder

  attr_reader :params

  def initialize(params = {})
    @params = params
  end

  def build
    User.new(default_params.merge(params))
  end

  def default_params
    {
      :name => "Missing Name",
      :email => "Missing Email",
      :hr_id => "Missing HR ID"
    }
  end
end

Then:

UserBuilder.new.build

Or:

UserBuilder.new({:name => 'foo'}).build

edited Mar 24 '14 at 16:16

answered Mar 24 '14 at 08:58

apneadiving

114,565
26
219
213

hey, just a quick comment. you forgot to set a default value to params so calling `new` without any arguments will not raise any error. – jvnill Mar 24 '14 at 16:12

score 0 · Answer 3 · answered Mar 24 '14 at 09:27

So I did a Google search for something like "ActiveRecord default values", and one of the things I found was this Ruby gem that's supposed to set this up for you, so it might be worth checking out: default_value_for.

Example usage from the docs:

class User < ActiveRecord::Base
  default_value_for :name, "(no name)"
  default_value_for :last_seen do
    Time.now
  end
end

u = User.new
u.name       # => "(no name)"
u.last_seen  # => Mon Sep 22 17:28:38 +0200 2008

Can i make Rails this code more elegant?

3 Answers3