Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added Options class to Mapper #6574

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

dyba commented May 31, 2012

The options hash has a lot of non-similar data stored in it. It makes it difficult to keep track of what exactly is stored inside. Perhaps an alternative to using a hash is to store options in a class and make the information accessible via methods. @jonleighton expressed the Mapper class was in need of refactoring. I'm starting with what I consider low-hanging fruit given my skill level.

@dyba dyba Added Options class to Mapper
The options hash has a lot of non-similar data stored in it.
It makes it difficult to keep track of what exactly is stored inside.
Perhaps another alternative is to store options in a class and make the
information accessible via methods.
d4d642f

@carlosantoniodasilva carlosantoniodasilva commented on the diff May 31, 2012

actionpack/lib/action_dispatch/routing/mapper.rb
+ end
+
+ def constraints=(constraints)
+ @opts[:constraints] = constraints
+ end
+
+ def delete(option)
+ @opts.delete(option)
+ end
+
+ def defaults
+ @opts[:defaults] ||= {}
+ end
+
+ alias :all :opts
+ end
@carlosantoniodasilva

carlosantoniodasilva May 31, 2012

Owner

Wrong indent here.

@dyba

dyba Jun 1, 2012

Contributor

I'm not sure why the indent didn't show up correctly here. If you take a look at the file, the indent is present there.

@carlosantoniodasilva

carlosantoniodasilva Jun 1, 2012

Owner

Are you using tabs instead of spaces? Might be the case, otherwise I have no idea :D

@dyba

dyba Jun 1, 2012

Contributor

It's strange. I'm using spaces. In vim I use :set softtabstop=2 and :set shiftwidth=2.

@carlosantoniodasilva

carlosantoniodasilva Jun 1, 2012

Owner

Well, no idea, I have this same config :D

I'm sorry but I particularly don't see a real gain with this refactor, since the Options class is only used by the scope method to wrap some method calls, instead of using the options hash directly, and afterwards the hash is used again to set scope[:options]. At the end, seems that just using the options hash is clear enough.

Anyway, thanks for your patch :)

Contributor

dyba commented Jun 1, 2012

Fair enough. I'll find a more interesting way to refactor the Mapper class.

Owner

pixeltrix commented Jun 1, 2012

I'm in agreement with @carlosantoniodasilva - all this is doing is wrapping a class around the options hash. To make it worthwhile it needs to be abstracting and/or reducing repetition in behaviour. The scope instance variable is probably the best one to look at since that's a really big hash that could be broken up.

@pixeltrix pixeltrix closed this Jun 1, 2012

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