views:

44

answers:

4

This happens in Ruby on Rails's View, where there is a hash for another partial. This hash has about 20 key/value pairs.

There is (in HAML)

- if (some_conditon)
  = render :partial => 'some_name', :locals => a_hash.merge({ :extra => true })
- else
  -# a lot more processing, including concatenating the partials and return as json
  - some_var.each do |item| 
    - result_html << (render :partial => 'some_name', :locals => a_hash )
    -# etc
  - response.content_type = "application/json"
  = result_html.to_json

So the question is, should the merge be written as merge! instead? Because it is no longer needed later, if a new hash is created, a lot of time will be spent for creating this new hash (20 items in hash). If an in-place modification is done, it can use the existing hash structure and add one item to it, which will be a lot faster?

+1  A: 

It might not be needed later in the function at the moment, but I wouldn't be surprised if you need to extend the view in the six months from now. And in six months time, you might be very surprised to find :extra => true stuffed into your hash after this if/else/end block.

So ask yourself which you would be more surprised by, in six months time: finding :extra => true in your hash, or not finding :extra => true in your hash. Without full details, it's hard to say which one I would prefer, I can imagine both paths making sense.

I wouldn't worry about speed unless your profiling has demonstrated that creating a new hash represents a measurable amount of processing.

sarnold
+2  A: 

This is a micro-optimization. As with all such matters, the question of "Should I…?" is answered by profiling and seeing whether A) the code in question is taking up a lot of time to begin with, and B) the change results in much of a speedup. If A isn't the case, don't proceed to B. If B isn't the case, the change isn't worth making the code less clean.

As for safety considerations, you don't only have to consider whether you're going to use that hash later in the method, but whether any other objects might have references to the hash. If you got this hash from another object and that object has it stored in an instance variable, adding keys to the hash will cause the other object to see the mutated version.

Chuck
+1  A: 

if you're worried about speed you could test this very easily, otherwise use merge()

jshen
+1  A: 

If you are certain that modifying the existing hash is not a problem, then you could certainly use merge!. However, I'm not sure if it will be "a lot" faster than simply using merge. copying a hash of 20 or so objects probably isn't an extremely time consuming operation. But, if it is a concern, you could benchmark the different implementations and see how much you will gain by doing one over the other.

Pete
hm... i am more concerned with a more correct way of doing things and at the same time, don't cause the code to slow down every where... so if no new object is created, that looks like a good way. But nowadays, CPU, RAM, and hard disk is all cheap. We are no longer in the 33MHz, 8MB RAM, 200MB hard disk era. Even TortoiseSVN is like 17MB nowadays... so we don't care so much about any cost these days. The realistic aquarium screensaver was only 1.2MB, with true to real life fish... but I think if CPU and RAM is not so expensive, then we can adapt to this environment too
動靜能量