Skip to content

target_reflection_has_associated_record? refactoring #8823

Merged
merged 1 commit into from Jan 8, 2013
View
6 activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -114,11 +114,7 @@ def build_record(attributes)
end
def target_reflection_has_associated_record?
- if through_reflection.macro == :belongs_to && owner[through_reflection.foreign_key].blank?
- false
- else
- true
- end
+ !(through_reflection.macro == :belongs_to && owner[through_reflection.foreign_key].blank?)
@goshakkk
goshakkk added a note Jan 8, 2013

I think instead of !(a == b && something.blank?) it can be a != b && something.present?

@acapilleri
acapilleri added a note Jan 8, 2013

Maybe yes, but I tried to refractor it without change the original logic of the condition

@goshakkk
goshakkk added a note Jan 8, 2013

Let's try to see what actually fits into the name of target_reflection_has_associated_record?

  • target reflection has associated record if reflection is not belongs_to and owner has some key, or
  • target reflection doesn't have associated record if reflection is belongs_to and owner doesn't have some key
@acapilleri
acapilleri added a note Jan 8, 2013

you've convinced me, it seems more clear

@acapilleri
acapilleri added a note Jan 8, 2013

but with your solution many test fails.....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
def update_through_counter?(method)
Something went wrong with that request. Please try again.