After the lively discussion at RailsConf regarding refactoring patches for Rails I'd like to raise awareness once more of my patch to fix the improper inheritance relationship between HMT and HOT association classes. The ticket is here: https://rails.lighthouseapp.com/projects/8994/tickets/1642-hasonethroughassociation-should-not-be-a-child-of-hasmanythroughassociation
For the click-averse among us, here is Pratik's most recent response to the ticket: > If people with a lot of extra time are looking for some work that might > actually add some real value, I'd be happy to help out. But I don't believe > this "refactoring" is making the code any easier to follow. Ignoring for the moment the unanimous support from commenters on the ticket, here is how this "refactoring" improves the code and makes it easier to understand: 1) Please take a look at activerecord/lib/active_record/ associations.rb, line 1236. This is the code that defies the association setter method for all associations. On line 1243 the code explicitly checks the type of the association_proxy_class and branches to different logic if it's a HasOneThroughAssociation. For all other association types the code makes a call to a polymorphic method without regard to object type, as it should. Changing implementation based on type interrogation in this way is a clear sign of code fragility, makes the method longer and less obvious, and causes the class abstraction to leak. My patch removes the explicit check and reduces six lines of code from 1243 to 1249 to two. At the same time it makes private the #create_through_record method in the HasOneThroughAssociation class, simplifying the public interface to the class. >From a more pragmatic point of view, this branching logic was a direct cause of one of the bugs I fixed in https://rails.lighthouseapp.com/projects/8994/tickets/895-has_one-through-errors . 2) Please look at activerecord/lib/active_record/associations/ has_many_through_association.rb. This is a long file, 257 lines. My patch pulls all of the Through-specific login into a module, which reduces the size of this file to 112 lines, and makes an obvious distinction between different aspects of the implementation. The patch also reduced the size of the #find_target method from twelve lines to two, delegating the scope construction to the shared #construct_scope method, rather than building the scope inline. 3) Please look at activerecord/lib/active_record/associations/ has_one_through_association.rb. As mentioned before this class exposes the #create_through_association method in the public interface, though it performs implementation that should be handled by the class within its abstraction. My patch makes this method private. The class also includes special-case overrides of #find, #find_target, and #reset_target! to mask the functionality inherited from HasManyThroughAssociation (the override of #reset_target! was the source of another bug I fixed in https://rails.lighthouseapp.com/projects/8994/tickets/895-has_one-through-errors). My patch removes the need for #find and #reset_target!, since the inherited behavior from HasOneAssociation is already correct, and changes the implementation of #find_target to mirror the implementation of #find_target on the HasManyThroughAssociation class, which is now a peer. In this case, it calls find(:first) on the reflections class, using the scope generated by ThroughAssociationScope#construct_scope. I believe this is more clear than sending #find_target to the HasManyThroughAssociation superclass with an explicit limit override. 4) This patch makes HasOneThroughAssociation a peer of HasManyThroughAssociation in the inheritance hierarchy, as it properly should be. HasOneAssociation is a peer of HasManyAssociation; doing the same with their Through counterparts is far more obvious than deepening the hierarchy only under HasManyAssociation. As well, the current inheritance tree imports all of the code from association_collection.rb and has_many_association.rb as well as hash_many_through_association.rb into has_one_through_association.rb. Moving HasOneThroughAssociation up the inheritance hierarchy to its proper place removes a lot of inherited cruft that can only cause complication (for instance, it made https://rails.lighthouseapp.com/projects/8994/tickets/1643-association-proxies-should-correctly-respond-to-method-defined-via-method_missing unnecessarily difficult) and confuse programmers who interrogate the HasOneThroughAssociation class for its capabilities. 5) All of this is in addition to my argument that the current implementation is a fundamental misuse of inheritance. This is not an esoteric argument, the concept of encapsulation over inheritance for sharing of implementation is well worn, baked into Liskov Substitutability, permeating Design Patterns, advocated regularly by the likes of Dave Thomas, etc. It's been proven enough times to matter that it makes code easier to understand, easier to change, and more resilient to defects. Ignore that at your peril, I suppose. I'm interested in any and all detailed, technical concerns regarding this patch. I've updated it to the current head of Rails master, with no test failures. I've done searches in GitHub and found no plugins that these changes might affect. And passive aggressive posturing implying that I'm submitting useless fluff because I have too much time on my hands isn't a particularly useful, or professional response. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
