Journal Friends Archive Profile Memories Chalain May. 23rd, 2008 10:26 am Is this Monkeypatch Good or Evil? Okay, firstoff, if you think all monkeypatches are inherently evil, you're invited to withdraw from this discussion, and even more so if you think they are all inherently good.



A coworker wanted to find the index of an element in an array. Not a problem, we have Array#index sitting right there. Ah, but he wanted to find the element using block syntax, and index doesn't support that.



Now, before we get outta control, this really is just two statements, innit: users.index(users.find {|u| u.name='Bob'}) It does access users twice, though, which is a tiny bit poogy--I've been on a no-locality kick lately. My coworker wrote a method called find_index, and it works pretty much as you'd expect. It encapsulates that line of code.



But then I thought, the real problem here is that index doesn't take block syntax, and it probably oughta. It took me about 30 seconds to whip out this monkeypatch: class Array alias_method :orig_index, :index def index(val=nil, &block) if block_given? orig_index(find(&block)) else orig_index(val) end end end

And it works exactly as expected: users.index {|u| u.name=='Bob'} Now, I don't want to get bogged down in whether or not we should be monkeypatching here. Let's take for a given right now that our small team has looked at the code and is willing to add this method to the bits of code they have to know exist, and that we all (on my team) agree that the cleaner syntax is a reward in itself.



The question is, however, should it be a monkeypatch... or should it be a separate method. The very specific reason for this question is that my monkeypatch now takes an argument or a block... but it COULD take an argument AND a block. In that case, it ignores the argument, but there's nothing in there to indicate that you can only use one or the other.



Ruby doesn't support different method signatures like C or Java would let us do, so if we want to make these arguments exclusive, we have to make a separate function.



Discuss.



P.S. Oh, duh: raise ArgumentError if !val.nil? && block_given? Is this the answer?16 comments - Leave a comment (no subject) - (Anonymous) From: chalain Date: May 23rd, 2008 06:30 pm (UTC) Re: speaking of ArgumentErrors (Link) Are you perhaps in the "all monkepatches are evil" camp? :-) Reply ) ( Parent ) ( Thread From: msmercenary Date: May 23rd, 2008 06:49 pm (UTC) Re: speaking of ArgumentErrors (Link) The more monkeypatches I see, the closer I get to that camp...



And I don't even code Ruby. Reply ) ( Parent ) ( Thread (no subject) - (Anonymous) From: chalain Date: May 27th, 2008 02:31 pm (UTC) Re: speaking of ArgumentErrors (Link) But I think it should be something of (almost) last resort, not a tool to be used lightly or with weak justifications... I would frown nine out of ten times



Okay, just for the record: you're in the "all monkeypatches are evil" camp. :-)



Actually, I find the attitude behind this statement interesting. My team has a very strong ethic of "do whatever it takes to improve readability and maintainability", and that means "you should ONLY, but you should also ALWAYS, make a monkeypatch if a monkeypatch is the appropriate solution."



Most of the Array methods have arg-or-block syntax. Patching Array



Most of your arguments circulate around a fear of potential maintenance issues that you have never faced. I know you've never faced them, because we have, and they are neither as common nor as disastrous as you fear. Not by an order of magnitude... on either count.



Take a look at all of your concerns, and ask yourself this question: does leaving this monkeypatch out fix any of those problems? Novice coders will still make mistakes, new versions of Ruby might still be incompatible with a small amount of your code, and every single line of code you ever write might have unintended ramifications that you will have to debug. You could argue that writing a monkeypatch might worsen these conditions. I'm saying I have actual experience that shows it does not. Not any more than any other line of code, and what we do know about these lines of code is that they improve readability and maintainability.



Sorry if I'm disagreeing overmuch. I largely wanted to avoid the "monkeypatches are inherently good/evil" argument because I am very passionate about the topic of readability. The general populace comes down blindly and hard against monkeypatches--you yourself think "nine out of ten times against" is a fair middle ground!--and it's hard to argue against that without sounding like I'm in the "monkeypatches are inherently good" camp. I am definitely not in that camp. A bad monkeypatch can ruin your whole day.



What I do feel is that monkeypatches are extremely powerful, and that means that when misused they are extremely dangerous. But that power can and should be leveraged whenever appropriate. I would agree with you on the "nine out of ten" rule if only one monkeypatch in ten were more useful than dangerous, but the reality is, once you've seen a few good monkeypatches it becomes very easy to tell them apart, and your sample set becomes much closer to "five out of ten".



This particular monkeypatch rides the very edge of power vs. danger. It only extends functionality, which is good, but it does so inside an existing method which, as you point out, gives the reader pause. And it should. Ultimately, I think we'll keep it as a patch on index, though, because that's where this functionality belongs.



Anyway, I hope I'm not coming across as dismissive. I appreciate your comments, and we considered them at length in debating the utility of this monkeypatch. Thank you! Okay, just for the record: you're in the "all monkeypatches are evil" camp. :-)Actually, I find the attitude behind this statement interesting. My team has a very strong ethic of "do whatever it takes to improve readability and maintainability", and that means "you should ONLY, but you should also ALWAYS, make a monkeypatch if a monkeypatch is the appropriate solution."Most of the Array methods have arg-or-block syntax. Patching Array #index in this way is clean, reliable, and readable. And, believe it or not, this really does improve maintainability. We use a key set of patches, and all programmers on our project are expected to know them.Most of your arguments circulate around a fear of potential maintenance issues that you have never faced. I know you've never faced them, because, and they are neither as common nor as disastrous as you fear. Not by an order of magnitude... on either count.Take a look at all of your concerns, and ask yourself this question: does leaving this monkeypatch out fix any of those problems? Novice coders will still make mistakes, new versions of Ruby might still be incompatible with a small amount of your code, and every single line of code you ever write might have unintended ramifications that you will have to debug. You could argue that writing a monkeypatchworsen these conditions. I'm saying I have actual experience that shows it does not. Not any more than any other line of code, and what we do know about these lines of code is that they improve readability and maintainability.Sorry if I'm disagreeing overmuch. I largely wanted to avoid the "monkeypatches are inherently good/evil" argument because I am very passionate about the topic of readability. The general populace comes down blindly and hard against monkeypatches--you yourself think "nine out of ten times against" is a fair middle ground!--and it's hard to argue against that without sounding like I'm in the "monkeypatches are inherently good" camp. I amnot in that camp. A bad monkeypatch can ruin your whole day.What I do feel is that monkeypatches are, and that means that when misused they are extremely dangerous. But that power can and should be leveraged whenever appropriate. I would agree with you on the "nine out of ten" rule if only one monkeypatch in ten were more useful than dangerous, but the reality is, once you've seen a few good monkeypatches it becomes very easy to tell them apart, and your sample set becomes much closer to "five out of ten".This particular monkeypatch rides the very edge of power vs. danger. It only extends functionality, which is good, but it does so inside an existing method which, as you point out, gives the reader pause. And it should. Ultimately, I think we'll keep it as a patch on index, though, becauseAnyway, I hope I'm not coming across as dismissive. I appreciate your comments, and we considered them at length in debating the utility of this monkeypatch. Thank you! Reply ) ( Parent ) ( Thread (no subject) - (Anonymous) From: chalain Date: May 27th, 2008 05:09 pm (UTC) Re: speaking of ArgumentErrors (Link) Oh, oh, OH! I just realized I may have inadvertently baited you. I have presented this here with all the safety guards removed for the sake of brevity. The actual code:



- is implemented with alias_method_chain



- timidly checks to see that the alias hasn't already been chained, and refuses to re-chain if so



- has rdoc (!!!)



- has a sanity check that passes calls ary.index {...} without the patch, expecting silent failure. If that condition ever changes (e.g. Ruby changes), the unit test will immediately fail.



Yeah, I just realized that if you assume the patch I presented was presented in its entirety (i.e. NO safety guards), it's a bit on the "swinging from the chandelier with a cutlass in my teeth" side.



Thanks again. Reply ) ( Parent ) ( Thread From: ext_101324 Date: May 23rd, 2008 07:02 pm (UTC) Re: speaking of ArgumentErrors (Link) I'm definitely not in the monkeypatching-is-evil camp -- it's saved my butt on more than one occasion -- but I have to agree with grandparent. The risk:gain ratio is too high on this one. You're tweaking a widely-used method for, let's face it, pretty minor syntactic convenience. The inherent risk of this particular change is pretty low, but multiplied by all the places that you do and don't know of that call index, it should give one pause.



If you were tweaking this to implement, say, an algorithm 2 orders of magnitude faster for your problem set that will have benefits throughout your application, and you've already used the normal index method everywhere, that's a different story. The risk is still there (probably greater) but the benefits are proportional.



Reply ) ( Parent ) ( Thread From: ext_101328 Date: May 23rd, 2008 07:21 pm (UTC) Re: speaking of ArgumentErrors (Link)

> always implies !(some_array.include?(val)). Or that subsequently does something with val if the

> Array



Nothing changes. Those assumptions would still hold.



The only change comes when block_given? I suppose someone somewhere might be passing a block to index, expecting it to be ignored, and now it wouldn't be. They probably deserve what they get. :)



///ark > It *looks* safe, but there might be some code, for example, that assumes some_array.index(val).nil?> always implies !(some_array.include?(val)). Or that subsequently does something with val if the> Array #index call doesn't return nil.Nothing changes. Those assumptions would still hold.The only change comes when block_given? I suppose someone somewhere might be passing a block to index, expecting it to be ignored, and now it wouldn't be. They probably deserve what they get. :)///ark Reply ) ( Parent ) ( Thread (no subject) - (Anonymous) From: ext_101328 Date: May 23rd, 2008 09:40 pm (UTC) Re: speaking of ArgumentErrors (Link) Oh, I see - you were talking about when someone is intentionally using the patch. In that case, you're right, and a case could be made for patching include? to ignore a passed block just to keep the symmetry. I don't know if anyone would find that useful, however.



I thought you were saying the patch was dangerous because it could trip up someone who didn't know about the patch.



///ark Reply ) ( Parent ) ( Thread From: chalain Date: May 23rd, 2008 10:39 pm (UTC) Re: speaking of ArgumentErrors (Link)



irb(main):001:0> [1,2,3].any? {|i| i==2} => true

Hmm, I've never thought of include? and index as being paired functions. THe block form of include? already exists: Reply ) ( Parent ) ( Thread From: ext_101348 Date: May 23rd, 2008 10:04 pm (UTC) I dig it (Link) Maybe I'm a little loose with my code but I dig it. Since index does not have a behavior defined when a block is given it seems ok to me to provide that behavior. Especially when the behavior defined is logical and expected (i.e. this could easily be something that might later get introduced in a future version of Ruby).



And your automated testing should let you know if there is any obscure reason to not do this. So I say go for it. Reply ) ( Thread From: ext_88325 Date: May 25th, 2008 03:24 pm (UTC) Re: I dig it (Link)



About this example in particular: the problem of what happens when you pass both an argument and a block bugs me. Probably for this particular case I'd use a different method, as I can't recall anywhere in the ruby stdlib where you have a block-taking method that ignores the arguments if a block is passed (of course, I woke up 20min ago, and haven't had coffee yet :P) So, for consistency, and POLS, I'd say 'Array



But, I'm often one to monkeypatch to add stuff without thinking it twice, specially if what I add is orthogonal with the original code and won't do any damage. About monkeypatching to add functionality: If you're adding to the method, and not changing anything, then what's to fear? The only problem would be for people who were already passing a block to Array #index . But as someone commented above: they had it coming.About this example in particular: the problem of what happens when you pass both an argument and a block bugs me. Probably for this particular case I'd use a different method, as I can't recall anywhere in the ruby stdlib where you have a block-taking method that ignores the arguments if a block is passed (of course, I woke up 20min ago, and haven't had coffee yet :P) So, for consistency, and POLS, I'd say 'Array #find_index ' does the job.But, I'm often one to monkeypatch to add stuff without thinking it twice, specially if what I add is orthogonal with the original code and won't do any damage. Reply ) ( Parent ) ( Thread From: ext_101423 Date: May 24th, 2008 12:43 pm (UTC) (Link) How easy is it to grep the code to find all incidences of using index with a block?



I'd use a different method name. Reply ) ( Thread From: jes5199 Date: May 25th, 2008 08:26 pm (UTC) (Link) it looks to me that it cannot possibly break existing code. which is a pretty good criterion. Reply ) ( Thread (no subject) - (Anonymous) From: jes5199 Date: May 27th, 2008 07:52 am (UTC) (Link)





I suppose code would break if there were two libraries trying to replace Array but your carefully written unit tests aren't going to test third-party libraries (ie rubygems) that break mysteriously when run alongside this hack.I suppose code would break if there were two libraries trying to replace Array #index in incompatible ways. Unfortunate that there's not really a way to detect that (is there?). Reply ) ( Parent ) ( Thread From: ext_101647 Date: May 26th, 2008 02:56 am (UTC) (Link)



Case in point - both rails and the classifier gem monkey patch Array



So I wouldn't release this in a library, but if it 'works for you' in your app code then go nuts. Monkey patching methods that are likely to be patched by other libraries is dangerous because one of the patches will get cloberred.Case in point - both rails and the classifier gem monkey patch Array #sum , so you get obscure errors trying to use them together.So I wouldn't release this in a library, but if it 'works for you' in your app code then go nuts. Reply ) ( Thread From: (Anonymous) Date: May 27th, 2008 09:01 am (UTC) Not a ruby programmer, but... (Link) .. can you have an array of blocks in ruby?

If so, aren't you clobbering the ability to find the index of a given block in that array?

Maybe not something you might be doing yourself, but are you sure no library code you rely on is going to do that? Reply ) ( Thread From: chalain Date: May 27th, 2008 12:41 pm (UTC) Re: Not a ruby programmer, but... (Link) You can't, at least not implicitly.



Although you can have arrays of procs, equivalence for them is undefined, so {|i| i==3} won't be equivalent to anything except itself, since you're implicitly creating a new proc there.



Finally, even if all that could work, the syntax would be arr.index( {|i| i==3 }) ; ... note the {} is inside the (). Reply ) ( Parent ) ( Thread