How could this “complex method” for codeclimate be refactored?

Question 1

I’m trying to figure out how to refactor some code based on what codeclimate is telling me. So far it seems that any method that is longer than 5 lines or has an if statement is “complex”. In this instance, I’m trying to create an event for a calendar based off of a meeting record.

Here is the Event model:

class Event < ActiveRecord::Base
  validates :title, :eventcolor_id, presence: true
  belongs_to :meeting
  belongs_to :eventcolor

  def self.create_from_meeting(meeting, eventcolor)
    meeting.meeting_date_end ||= meeting.meeting_date_start
    Event.find_or_initialize_by(meeting_id: meeting.id) do |e|
      e.title = meeting.title
      e.datetime_start = Time.zone.local_to_utc(meeting.meeting_date_start)
      e.datetime_end = Time.zone.local_to_utc(meeting.meeting_date_end)
      e.location = meeting.location
      e.address1 = meeting.address1
      e.address2 = meeting.address2
      e.city = meeting.city
      e.state = meeting.state
      e.zip = meeting.zip
      e.description = (!meeting.description.blank?) ? meeting.description : ''
      e.eventcolor_id = eventcolor
      e.save
      e.committees << meeting.committee if !meeting.committee.nil?
    end
  end
end

which is called from my meetings_controller with this line:

Event.create_from_meeting(meeting, params[:eventcolors_select])

Question 2

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 starts_at and ends_at would be more than enough. It’d follow the convention set by updated_at and 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 blank?.
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 nil.
Similarly, this if !meeting.committee.nil? can be written a few other ways:
```
unless meeting.commitee.nil?
if meeting.commitee.present?
if meeting.commitee
```
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_start and 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 Event and 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.send("#{attribute}=", meeting.send(attribute))
    end
    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
  end
end

But it’s still not very pretty, just shorter. Again, the main issue is the duplication.

Leave a Reply Cancel reply