Refactor Journey::Routes

[goshacmd]

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

[rafaelfranca]

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

[goshacmd]

@rafaelfranca ok, reverted it.

[rafaelfranca]

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.

[goshacmd]

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]

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.

[goshacmd]

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?

[rafaelfranca]

Seems good

[goshacmd]

Committed that & rebased.

[goshacmd]

Thanks.