Refactor Journey::Routes #8783

Merged
merged 1 commit into from Jan 6, 2013

Conversation

Projects
None yet
2 participants
Contributor

goshakkk commented Jan 6, 2013

  • prefer do-end for multiline blocks
  • prefer or-equals over returns with checks
Owner

rafaelfranca commented Jan 6, 2013

I really don't like tap it doesn't make anything better in that case.

Contributor

goshakkk commented Jan 6, 2013

@rafaelfranca ok, reverted it.

end
def ast
- return @ast if @ast
@rafaelfranca

rafaelfranca Jan 6, 2013

Owner

This change the behavior since if @ast is present it still will check if partitioned_routes.first is blank. The before code doesn't behave this way.

@goshakkk

goshakkk Jan 6, 2013

Contributor

I don't see if @ast is set anywhere besides this method. Therefore, if partitioned_routes.first.empty? ever returns true, @ast won't get assigned. I think it is safe to make this change. Also, tests do pass.

@rafaelfranca

rafaelfranca Jan 6, 2013

Owner

I think you didn't get. Before this change in the first call all the method body is called, and in the second call only the first line. So partitioned_routes.first.empty? is only called on the first call.

After this change partitioned_routes.first.empty? is called in every call of this method.

@goshakkk

goshakkk Jan 6, 2013

Contributor

Then, I think it can be refactored as follows:

@ast ||= begin
  asts = partitioned_routes.first.map(&:ast)
  Nodes::Or.new(asts) unless asts.empty?
end

So that, if partitioned_routes.first is empty, partitioned_routes.first.map(&:ast) would also be empty. Therefore Nodes::Or.new(asts) unless asts.empty? will set @ast to nil in case it's empty, and to Nodes::Or otherwise. That behavior is completely identical to the original one.

What do you think?

@goshakkk

goshakkk Jan 6, 2013

Contributor

Committed that & rebased.

refactor Journey::Routes
* prefer do-end for multiline blocks
* prefer or-equals over returns with checks

rafaelfranca added a commit that referenced this pull request Jan 6, 2013

@rafaelfranca rafaelfranca merged commit 4bfcae0 into rails:master Jan 6, 2013

@goshakkk goshakkk deleted the goshakkk:refactor-journey-routes branch Jan 6, 2013

Contributor

goshakkk commented Jan 6, 2013

Thanks.

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