Is it good to use reverse_merge! on the optional hash argument of a method?

[Eastknight]

I found an example in this guide

def obliterate(things, options = {})
  default_options = {
    :gently => true, # obliterate with soft-delete
    :except => [], # skip obliterating these things
    :at => Time.now, # don't obliterate them until later
  }
  options.reverse_merge!(default_options)

  ...
end

I'm wondering whether this is a good practice. When you use this method, the input options hash might be modified by reverse_merge! method, which, in my opinion, is very error-prone process in a big project.

[robotpistol]

I've been thinking about this for a while. and I think I agree with you. Though many options are passed in statically, it is very possible that they are constructed and reused elsewhere

In this case it is safer to do

  options = options.reverse_merge({
    :gently => true, # obliterate with soft-delete
    :except => [], # skip obliterating these things
    :at => Time.now, # don't obliterate them until later
  })

Which will set options to a copy of the original options hash, leaving the original in tact.

[ljharb]

+1, any methods that mutate imo should be avoided, to avoid this hazard.

Alternatively,

  options = {
    :gently => true, # obliterate with soft-delete
    :except => [], # skip obliterating these things
    :at => Time.now, # don't obliterate them until later
  }.merge(options)