A few things caught my eye:
You don't check whether
e.save actually worked. If it fails, you'll have problems down the line.
meeting.meeting_date_end ||= meeting.meeting_date_start
Don't set an attribute on a record you don't "own"! In this case, a meeting object is being passed to the method to be copied - not altered. Use a local variable instead. It could be a nasty surprise to other parts of the code, to find that the meeting's been altered. Treat the meeting as strictly read-only.
Why are some of your
meeting's date/time-attributes prefixed with the class name? It's redundant; they're attributes on a
Meeting instance, so there's no reason to name them
meeting_*. You also prefix
event.datetime_start with the type, which isn't Ruby-like or Rails-like either. In both cases, something like
ends_at would be more than enough. It'd follow the convention set by
created_at and be immediately recognizable.
Speaking of, why are your meeting's date/time attributes defined in the local timezone? I'd advice you to store everything as UTC, so you don't have to worry about conversion (as you do in your current code). Because timezone conversion always ends up being painful.
e.description = (!meeting.description.blank?) ? meeting.description : ''
The parentheses aren't necessary, but moreover there's the
present? method, which does what you want here: The opposite of
However, you could also just do
e.description = meeting.description || '' since
blank? triggers on
nils and empty strings, but the latter is the default anyway, so you really only want to default in case of
if !meeting.committee.nil? can be written a few other ways:
The last one is the simplest and clearest, but all of them are, to my eyes, better than saying "if the committee is not not there"
My biggest concern is that this is a lot of data duplication - seemingly for no reason. Since
Event already belongs to a
Meeting, why duplicate anything? Everything should be available since you have
event.meeting. Duplicating everything doesn't really make sense as long as you have the
Meeting record. All you gain is the task of keeping two separate sets of data in sync whenever one changes. To me, this approach doesn't really make sense.
So I'd stick with:
event = meeting.events.create(eventcolor_id: params[:eventcolors_select])
and maybe add some accessor methods (decorators) to
Event to give you the correct
datetime_end. No need for the method at all.
Semantically, I'd prefer to have it flipped the other way, though: Make a meeting
belong_to an event. A calendar event is a pretty generic entity: It can be anything with a time and (optionally) a place. A meeting, meanwhile, is a more specific thing. So to my mind, it'd make more sense to have a meeting attached to a calendar event, rather than the other way around.
Or you can do Single Table Inheritance with
Event as the parent class. Or have an even more generic model - or models - the wrap the time and/or place, and have both
Meeting refer to that.
Basically, there are many ways to handle this without duplicating data.
Meanwhile, just looking at the code as-is, you can get rid of some lines:
# Note: May raise ActiveRecord::RecordInvalid
def self.create_from_meeting(meeting, eventcolor)
Event.find_or_initialize_by(meeting_id: meeting.id) do |e|
# copy all the identically-named attributes
%w(title location address1 address2 city state zip description).each do |attribute|
e.datetime_start = Time.zone.local_to_utc(meeting.meeting_date_start)
e.datetime_end = Time.zone.local_to_utc(meeting.meeting_date_end || meeting.meeting_date_start)
e.eventcolor_id = eventcolor
e.save! # raises an exception if it fails
e.committees << meeting.committee if meeting.committee
But it's still not very pretty, just shorter. Again, the main issue is the duplication.