WIP Refactor xml conversion to hash #8471

Merged
merged 1 commit into from Dec 21, 2012

Conversation

Projects
None yet
6 participants
@kytrinyx
Contributor

kytrinyx commented Dec 9, 2012

Hash.from_xml was very complicated, and so we decided to take a stab at
refactoring it. This is a work in progress that we want to get feedback on.

Basically, instead of defining a huge method on Hash, we made a utility
object for actually doing the conversion. This let us break up the big
method into a bunch of smaller ones, so that the process overall is
easier to understand.

However, some of the names kinda suck. Still working on that. But we
wanted to get some feedback on if this kind of change is generally
considered to be better or not before putting more effort into
breaking it up. We think that a bunch of small methods are easier to understand
than one massive one.

Things to work on:

  • Names. {process,convert} is awkward. And there's also typecast/to.
  • Might be another utility object as well, as half of the class is for refining
    a Hash that's getting converted.
  • We may have uncovered either missing test cases or extraneous conditions
    in a few of the if statements. We've left some comments. Tests pass if
    you remove them, but we didn't want to before confirming that they
    aren't actually needed.
  • Changes to documentation, including # :nodoc: where appropriate

❤️

@rafaelfranca

View changes

activesupport/lib/active_support/core_ext/hash/conversions.rb
- when Hash
- Hash[params.map { |k,v| [k.to_s.tr('-', '_'), unrename_keys(v)] } ]
+ def typecast_to_hash(value)
+ send("convert_#{value.class.to_s.downcase}", value)

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 10, 2012

Member

I think that is better to use a case statement here instead of call send based in the class name.

@rafaelfranca

rafaelfranca Dec 10, 2012

Member

I think that is better to use a case statement here instead of call send based in the class name.

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 10, 2012

Member

Really? Hm. I guess it's not the worst, and there's only 4 of them...

@steveklabnik

steveklabnik Dec 10, 2012

Member

Really? Hm. I guess it's not the worst, and there's only 4 of them...

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 10, 2012

Member

I don't know, seems weird to me.

case value
when Array
  convert_array(value)
when Hash
  convert_hash(value)
else
  raise
end

This reads better to me. It is cleaner. It make clear what are the supported types of value without need to read the whole class.

@rafaelfranca

rafaelfranca Dec 10, 2012

Member

I don't know, seems weird to me.

case value
when Array
  convert_array(value)
when Hash
  convert_hash(value)
else
  raise
end

This reads better to me. It is cleaner. It make clear what are the supported types of value without need to read the whole class.

@rafaelfranca

View changes

activesupport/lib/active_support/core_ext/hash/conversions.rb
+ end
+
+ def convert_hash(value)
+ if array?(value)

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 10, 2012

Member

Those methods names seems weird. array?(value) means value === Array to me. But I can't think nothing better now

@rafaelfranca

rafaelfranca Dec 10, 2012

Member

Those methods names seems weird. array?(value) means value === Array to me. But I can't think nothing better now

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 10, 2012

Member

Yeah, so we tried to encapsulate what the conversion was trying to get at. So like #hash? for example is the thing that determines if the node is a hash.

Everything could kinda use better names.

@steveklabnik

steveklabnik Dec 10, 2012

Member

Yeah, so we tried to encapsulate what the conversion was trying to get at. So like #hash? for example is the thing that determines if the node is a hash.

Everything could kinda use better names.

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 10, 2012

Member

In this context it kinda seems ok, since it's checking the type attribute for array, I guess.

@carlosantoniodasilva

carlosantoniodasilva Dec 10, 2012

Member

In this context it kinda seems ok, since it's checking the type attribute for array, I guess.

@rafaelfranca

View changes

activesupport/lib/active_support/core_ext/hash/conversions.rb
+
+ def process_array(value)
+ _, entries = Array.wrap(value.detect { |k,v| not v.is_a?(String) })
+ if entries.nil? || (c = value['__content__'] && c.blank?)

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 10, 2012

Member
if entries.nil? || (value['__content__'].blank?)
@rafaelfranca

rafaelfranca Dec 10, 2012

Member
if entries.nil? || (value['__content__'].blank?)

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 10, 2012

Member

Totally. This is the original code, we just moved it around, so I've missed some easy stuff like this.

@steveklabnik

steveklabnik Dec 10, 2012

Member

Totally. This is the original code, we just moved it around, so I've missed some easy stuff like this.

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 21, 2012

Member

.blank? fails the tests, because the left half checks that it's not nil or false.

@steveklabnik

steveklabnik Dec 21, 2012

Member

.blank? fails the tests, because the left half checks that it's not nil or false.

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 21, 2012

Member

had to do .try(:empty?) to handle the case properly.

@steveklabnik

steveklabnik Dec 21, 2012

Member

had to do .try(:empty?) to handle the case properly.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 10, 2012

Member

I like the idea to have a specialized object to do the conversion. 👍 from my side

Member

rafaelfranca commented Dec 10, 2012

I like the idea to have a specialized object to do the conversion. 👍 from my side

@carlosantoniodasilva

View changes

activesupport/lib/active_support/core_ext/hash/conversions.rb
+ if entries.nil? || (c = value['__content__'] && c.blank?)
+ []
+ else
+ case entries # something weird with classes not matching here. maybe singleton methods breaking is_a?

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 10, 2012

Member

This comment could 🔥

@carlosantoniodasilva

carlosantoniodasilva Dec 10, 2012

Member

This comment could 🔥

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 10, 2012

Member

yeah it was originally there. I think that maybe the code was changed and not the comment.

@steveklabnik

steveklabnik Dec 10, 2012

Member

yeah it was originally there. I think that maybe the code was changed and not the comment.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 10, 2012

Member

Nice stuff @kytrinyx @steveklabnik 👍

Nice stuff @kytrinyx @steveklabnik 👍

+end
+
+module ActiveSupport
+ class XMLConverter

This comment has been minimized.

Show comment Hide comment
@frodsan

frodsan Dec 10, 2012

Contributor

# :nodoc:

@frodsan

frodsan Dec 10, 2012

Contributor

# :nodoc:

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 10, 2012

Member

Totes.

@steveklabnik

View changes

activesupport/lib/active_support/core_ext/hash/conversions.rb
+ end
+
+ def content?(value)
+ value['type'] == 'file' || value['__content__']

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 21, 2012

Member

Changing this is the line that broke the test. ha!

@steveklabnik

steveklabnik Dec 21, 2012

Member

Changing this is the line that broke the test. ha!

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 21, 2012

Member

We've updated this pull request to handle all of the feedback.

We re-checked that we didn't break anything when making these changes, and rolled back a few of the more aggressive changes we made previously.

We're thinking that, since most people don't actually use this code, in the future, it might be a good candidate to extract to a plugin.

Member

steveklabnik commented Dec 21, 2012

We've updated this pull request to handle all of the feedback.

We re-checked that we didn't break anything when making these changes, and rolled back a few of the more aggressive changes we made previously.

We're thinking that, since most people don't actually use this code, in the future, it might be a good candidate to extract to a plugin.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Dec 21, 2012

Member

:shipit:

Member

rafaelfranca commented Dec 21, 2012

:shipit:

@kytrinyx

This comment has been minimized.

Show comment Hide comment
@kytrinyx

kytrinyx Dec 21, 2012

Contributor

Should we put the XMLConverter class in its own file?

Contributor

kytrinyx commented Dec 21, 2012

Should we put the XMLConverter class in its own file?

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Dec 21, 2012

Member

Since it's probably not going to be used on its own, this is fine. Having it in its own file would be nice if it needed to be autoloaded, but it won't.

I'm going to squash and then :shipit:.

Member

steveklabnik commented Dec 21, 2012

Since it's probably not going to be used on its own, this is fine. Having it in its own file would be nice if it needed to be autoloaded, but it won't.

I'm going to squash and then :shipit:.

Steve Klabnik + Katrina Owen
Refactor Hash.from_xml.
Three basic refactors in this PR:

* We extracted the logic into a method object. We now don't define a tone of extraneous methods on Hash, even if they were private.
* Extracted blocks of the case statement into methods that do the work. This makes the logic more clear.
* Extracted complicated if clauses into their own query methods. They often have two or three terms, this makes it much easier to see what they _do_.

We took care not to refactor too much as to not break anything, and put comments where we suspect tests are missing.

We think ActiveSupport::XMLMini might be a good candidate to move to a plugin in the future.

steveklabnik added a commit that referenced this pull request Dec 21, 2012

@steveklabnik steveklabnik merged commit 10c0a3b into rails:master Dec 21, 2012

@kytrinyx kytrinyx deleted the kytrinyx:refactor-xml-to-hash branch Dec 21, 2012

@rizwanreza

This comment has been minimized.

Show comment Hide comment
@rizwanreza

rizwanreza Dec 25, 2012

Contributor

Love this.

Contributor

rizwanreza commented on b02ebe7 Dec 25, 2012

Love this.

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