Add data of unknown type to a hash

Question

I don't see any way to refactor this, but something tells me I'm missing something.

if val.instance_of?(Hash) then
    @fields.has_key?(key) ? @fields[key].merge!(val) : @fields.merge!({key => val})
elsif val.instance_of?(Array) then
    @fields.has_key?(key) ? @fields[key].push(val) : @fields.merge!({key => Array.new.push(val)})
else
    @fields.has_key?(key) ? @fields[key] << val : @fields.merge!({key => val})
end

The point of the method is to take a hash and insert a value of different types if the key doesn't exist:

{:key => "value}, {:key = ["value"]}, or {:key => {:k => "value"}}

or append if it does:

{:key=> "value1value2"}, {:key => [["value1"], ["value2"]], {:key => {:k1 => "value1", :k2 => "value2"}}

it does work as intended, so my issue is thinking that there is likely an easier way to do it.

Answer 1

There are several options for refactoring your code. In any case we must get value for given key.

1. If value not exist (same as nil by default in a hash) we must set value as result value ( standart behavior for hash).
2. If value exist we should do something based on existing or given value class. IMHO "case" operator looking very good for this. In "when" option we can set one or more classes to redefine standart behavior.

Code wrapped into class for easy explore it in irb. It just sample for presentation

class MyHash < Hash
  def []=(key,val)
    super(key, 
          case (old = self[key]).class
          when Array, String
            old + val
          when Hash, MyHash
            old.merge(val)
          else
            val 
          end
         )
  end
end

Ok, now some testing in irb:

Strings

 >   mh = MyHash.new()
 => {}

 > mh[:k1] = "str1"
 => "str1"

 > mh
 => {:k1=>"str1"}

 > mh[:k1] = "str2"
 => "str2"

 > mh
 => {:k1=>"str1str2"}

Arrays

> mh[:k2] = [1]
 => [1] 

 > mh
 => {:k1=>"str1str2", :k2=>[1]} 

 > mh[:k2] = [2]
 => [2] 

 > mh
 => {:k1=>"str1str2", :k2=>[1, 2]} 

Hashes

 > mh[:k3] = {1=>:a}
 => {1=>:a} 

 > mh
 => {:k1=>"str1str2", :k2=>[1, 2], :k3=>{1=>:a}} 

 > mh[:k3] = {2=>:b}
 => {2=>:b} 

 > mh
 => {:k1=>"str1str2", :k2=>[1, 2], :k3=>{1=>:a, 2=>:b}} 

This code have many issues, behavior when old and new value have different class, Arrays result should be array of array etc. but as stated above it is just sample.

Answer 2

There are some obvious targets for refactoring, such as Array.new.push(val) instead of [val], but to me this entire code block stinks. The most obvious smell is the type checking necessitated by a lack of duck typing.

Can you explain more about the tensions that lead to this design? I suspect the best solution is further up the context stack than what you have provided.