Reduce complexity in 2 _inverse_of methods #16202

Closed
wants to merge 1 commit into
from

Projects

None yet

6 participants

@johnkelly

In polymoprhic_inverse_of:
Use an early return and a guard to raise an error instead of nested if statements which in the true/true case returns the condition of the if statement

In automatic_inverse_of:
Fix the comment to say false instead of nil
Reduce complexity by moving to a early return and a method level rescue instead of a begin..rescue..end.
Eliminates an explicit return in the middle of the method

@rafaelfranca rafaelfranca commented on an outdated diff Jul 17, 2014
activerecord/lib/active_record/reflection.rb
@@ -399,13 +399,10 @@ def inverse_of
end
def polymorphic_inverse_of(associated_class)
- if has_inverse?
- if inverse_relationship = associated_class._reflect_on_association(options[:inverse_of])
- inverse_relationship
- else
- raise InverseOfAssociationNotFoundError.new(self, associated_class)
- end
- end
+ return unless has_inverse?
@rafaelfranca
rafaelfranca Jul 17, 2014 Ruby on Rails member

Can we avoid the early return here? I really prefer the if.

@rafaelfranca rafaelfranca commented on an outdated diff Jul 17, 2014
activerecord/lib/active_record/reflection.rb
@@ -399,13 +399,10 @@ def inverse_of
end
def polymorphic_inverse_of(associated_class)
- if has_inverse?
- if inverse_relationship = associated_class._reflect_on_association(options[:inverse_of])
- inverse_relationship
- else
- raise InverseOfAssociationNotFoundError.new(self, associated_class)
- end
- end
+ return unless has_inverse?
+
+ inverse_relationship = associated_class._reflect_on_association(options[:inverse_of])
+ inverse_relationship || (raise InverseOfAssociationNotFoundError.new(self, associated_class))
@rafaelfranca
rafaelfranca Jul 17, 2014 Ruby on Rails member

We can use or and avoid the (

@rafaelfranca rafaelfranca commented on an outdated diff Jul 17, 2014
activerecord/lib/active_record/reflection.rb
@@ -500,24 +497,17 @@ def inverse_name
end
end
- # returns either nil or the inverse association name that it finds.
+ # Returns either false or the inverse association name.
@rafaelfranca
rafaelfranca Jul 17, 2014 Ruby on Rails member

We are changing the return value. It is better to keep returning nil here.

@rafaelfranca
rafaelfranca Jul 17, 2014 Ruby on Rails member

Oops, it already returned false before your change.

@rafaelfranca rafaelfranca commented on an outdated diff Jul 17, 2014
activerecord/lib/active_record/reflection.rb
def automatic_inverse_of
- if can_find_inverse_of_automatically?(self)
- inverse_name = ActiveSupport::Inflector.underscore(options[:as] || active_record.name).to_sym
-
- begin
- reflection = klass._reflect_on_association(inverse_name)
- rescue NameError
- # Give up: we couldn't compute the klass type so we won't be able
- # to find any associations either.
- reflection = false
- end
+ return false unless can_find_inverse_of_automatically?(self)
@rafaelfranca
rafaelfranca Jul 17, 2014 Ruby on Rails member

Can we use if/else?

@rafaelfranca rafaelfranca commented on an outdated diff Jul 17, 2014
activerecord/lib/active_record/reflection.rb
@@ -500,24 +497,17 @@ def inverse_name
end
end
- # returns either nil or the inverse association name that it finds.
+ # Returns either false or the inverse association name.
+ # If a NameError occurs, we couldn't compute the klass type
@rafaelfranca
rafaelfranca Jul 17, 2014 Ruby on Rails member

We don't need to talk about the NameError case here. In fact the before text were fine:

# Returns either false or the inverse association name that it finds.

If it doesn't find so false is returned.

@johnkelly johnkelly Reduce complexity in 2 _inverse_of methods
In polymoprhic_inverse_of:
Use an early return and an or instead of an if value return value else raise an error

In automatic_inverse_of:
Fix the comment to say false instead of nil
Reduce complexity by moving to a early return and a method level rescue instead of a begin..rescue..end
427c959
@johnkelly

@rafaelfranca Thanks for the feedback. I've made the suggested changes.

@egilburg egilburg commented on the diff Jul 18, 2014
activerecord/lib/active_record/reflection.rb
end
-
+ rescue NameError
@egilburg
egilburg Jul 18, 2014

👎, this change makes rescue NameError scope less precise (the whole method) instead of only the line it is expected at, making it harder to debug and higher chance of it rescuing when it shouldn't be

@pixeltrix
pixeltrix Jul 18, 2014 Ruby on Rails member

Have to agree with @egilburg here - we can't really merge it with this as it is

@matthewd matthewd commented on the diff Jul 18, 2014
activerecord/lib/active_record/reflection.rb
@@ -394,11 +394,8 @@ def inverse_of
def polymorphic_inverse_of(associated_class)
if has_inverse?
- if inverse_relationship = associated_class._reflect_on_association(options[:inverse_of])
- inverse_relationship
- else
- raise InverseOfAssociationNotFoundError.new(self, associated_class)
- end
+ inverse_relationship = associated_class._reflect_on_association(options[:inverse_of])
+ inverse_relationship or raise InverseOfAssociationNotFoundError.new(self, associated_class)
@matthewd
matthewd Jul 18, 2014 Ruby on Rails member

I prefer the if/else spelling, but if we're going to use or...

associated_class._reflect_on_association(options[:inverse_of]) or
  raise InverseOfAssociationNotFoundError.new(self, associated_class)
@sgrif sgrif was assigned by rails-bot Oct 20, 2015
@sgrif
Member
sgrif commented Oct 29, 2015

Closing due to inactivity. There's a lot of unresolved feedback

@sgrif sgrif closed this Oct 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment