Ruby recursive map of a hash of objects

Question

I have a hash that has an unknown collection and mixture of nested arrays, hashes, arrays of hashes and strings. This is the result of JSON.parse. The structure of the data must be the same as it started with. The end goal is to convert strings to Fixnums that could be Fixnums.

The following works just fine, but I was wondering if it could be shortened. Note how I need the key and the value in the clean method as not all strings that can be Fixnums should be. Any ideas?

def clean_node(node)
    if node.class == String
      clean(node)
    elsif node.class == Array
      node.each_with_index do |obj, i|
        if obj.class == String
          node[i] = clean(node[i], obj)
        else
          clean_node(obj)
        end
      end
    elsif node.class == Hash
      node.each_pair do |key, value|
        if value.class == String
          node[key] = clean(key, value)
        else
          clean_node(value)
        end
      end
    end
end

def clean(key, value)
    FIXNUM_KEYS.include?(key)? value.to_i : value
end

Are you generating the JSON yourself? It looks like the JSON contains numbers as strings. I would rather try to fix the code that generates the JSON. — Patrick Oscity
– Patrick Oscity, Commented Nov 13, 2013 at 7:38
In line 7, on the right side of the assignment, node[i] is the same as obj. Why are you writing clean(node[i], obj)? — sawa
– sawa, Commented Nov 13, 2013 at 7:46
notwithstanding the strange logic noted by sawa, this kind of code is pretty typical of processing heterogenous data structures - I don't think it really simplifies.... — GreenAsJade
– GreenAsJade, Commented Nov 13, 2013 at 7:52
I don't generate the JSON otherwise of course, i would fix it there. And trying to fix it once I get it before parsing it to a hash results in a horrible amount of side effects to deal with. — Marc
– Marc, Commented Nov 13, 2013 at 21:45
@sawa because clean actually needs the key and the value, clean returns a string. I wrote clean so I didn't have to write that same line in clean twice, once for the hash and once for the array cases. — Marc
– Marc, Commented Nov 13, 2013 at 21:47

Neil Slater · Accepted Answer · 2013-11-13 22:57:33Z

3

There is no need to split up the string processing, you can do it all in one recursive routine, if you return the value of node at the end of the routine. This has the side effect of removing some parameters, and you can use in-place .map! for handling arrays.

Using case makes the selection by type slightly easier to read.

Adding the ability to deal with arrays of strings could be done in lots of ways. I've chosen to add a state variable (a common pattern in recursion, for instance to count depth). Then in the case of arrays, the recursion inherits the state from the current level, whilst the hash case determines new state (to convert or not) based on lookup in the list of keys to apply conversions.

def clean_node( node, convert_item = false )

    case node

    when String
      node = node.to_i if convert_item

    when Array
      node.map! do |obj| 
        clean_node( obj, convert_item )
      end

    when Hash
      node.each_pair do |key, value|
        node[key] = clean_node( value, FIXNUM_KEYS.include?(key) )
      end
    end

    node
end

edited Nov 13, 2013 at 22:57

answered Nov 13, 2013 at 8:33

Neil Slater

27.4k6 gold badges80 silver badges98 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Marc Over a year ago

I like it, but you don't check the FIXNUM_KEYS and do a .to_i with any of the array objects?

Neil Slater Over a year ago

@Marc: Your original code did not apparently deal with that (and having the declarative nature of FIXNUM_KEYS means there is some un-written intent in how it should be used). In practice I think it is cleaner to add back in the handling of String values and convert one recursion level deeper.

hirolau · Accepted Answer · 2013-11-13 08:15:58Z

2

Although I have not looked into the recursion, I must comment on the fact that you are writing if statements that would be easier to read in a case statement:

def clean_node(node)
  case node
    when String then clean(node)
    when Array
      node.each_with_index do |obj, i|
        case obj
          when String
            node[i] = clean(node[i], obj)
          else
            clean_node(obj)
        end
      end
    when Hash....

answered Nov 13, 2013 at 8:15

hirolau

14k9 gold badges39 silver badges51 bronze badges

2 Comments

Neil Slater Over a year ago

I also think that the case for String is redundant. All the strings will be altered inside the containers, and clean(node) does not alter the string in place. The code could be cleaned up even further, but it all starts looking like codereview.stackexchange.com - I think moving to case statements is a good core piece of advice for readability.

Marc Over a year ago

I never knew of codereview.stackexchange.com, thanks, and yes, that was what I was going after. The casing suggestion is great. Going to wait for @NeilSlater response to my question to his answer before I mark anyone as answer.

Collectives™ on Stack Overflow

Ruby recursive map of a hash of objects

2 Answers 2

2 Comments

2 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related