Skip to content

Instantly share code, notes, and snippets.

@edavis10
Created August 9, 2011 22:37
Show Gist options
  • Save edavis10/1135390 to your computer and use it in GitHub Desktop.
Save edavis10/1135390 to your computer and use it in GitHub Desktop.
Refactoring
def validate_status_changes
return if changes.keys == ["status"]
return if changes["status"].present? && changes["status"].second == "open"
return if changes["status"].present? && changes["status"].first == "open"
errors.add_to_base(:cant_update_locked_deliverable) if locked?
errors.add_to_base(:cant_update_closed_deliverable) if closed?
end
def validate_status_changes
if changes.keys == ["status"]
noop("Allow changes to the status only")
elsif changes["status"].present? && changes["status"].second == "open"
noop("Allow any changes when going to 'open'")
elsif changes["status"].present? && changes["status"].first == "open"
noop("Allow any changes when going from 'open' to another status")
else
errors.add_to_base(:cant_update_locked_deliverable) if locked?
errors.add_to_base(:cant_update_closed_deliverable) if closed?
end
end
@gilesbowkett
Copy link

class << changes
  def open?
    ["status"] == self.keys ||
      self["status"].present? && ("open" == self["status"].second || "open" == self["status"].first)
  end
end

def validate_status_changes
  return if changes.open?

  errors.add_to_base(:cant_update_locked_deliverable) if locked?
  errors.add_to_base(:cant_update_closed_deliverable) if closed?
end

not literally, necessarily - would prefer seeing it on some kind of object

@edavis10
Copy link
Author

edavis10 commented Aug 9, 2011

Interesting, didn't think about defining a method on the changes object (it's from ActiveRecord's dirty tracking). I'll try something like that out and see (open? is used in the main class so I don't want to reuse that method name here).

@elight
Copy link

elight commented Aug 9, 2011

+1 to Giles' version. I'm a big fan of the "extracting boolean expressions into predicate methods" idiom.

@edavis10
Copy link
Author

edavis10 commented Aug 9, 2011

Can't get that version to work. changes is an instance method that isn't available.

undefined local variable or method `changes' for #Class:0xb5cfa6b0 (NameError)

@edavis10
Copy link
Author

edavis10 commented Aug 9, 2011

That got me unblocked, here is what I came up with (tests all green)

  def valid_status_change?
    change_to_status_only? || changing_to_the_open_status? || changing_from_the_open_status?
  end

  def change_to_status_only?
    ["status"] == changes.keys
  end

  def changing_to_the_open_status?
    changes["status"].present? && "open" == changes["status"].second
  end

  def changing_from_the_open_status?
    changes["status"].present? && "open" == changes["status"].first
  end

  def validate_status_changes
    return if valid_status_change?

    errors.add_to_base(:cant_update_locked_deliverable) if locked?
    errors.add_to_base(:cant_update_closed_deliverable) if closed?
  end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment