Refactored code

[dcrec1]

Hi, I noticed the code had some duplications and some conditionals that could be avoided and decided to refactor it.

The only thing that changed in the behavior is the modification of 'return false' to 'event.preventDefault()'. I couldn't test this because the tests were being included before rails.js, so I couldn't bind an event to an element after the ujs events to test if they were being called, but I can try to find a way.

[mislav]

There are good parts, there are bad (meaning: unwanted) parts.

Parts that I like: attr('data-') changed to data(), val() invocations with a function (didn't know about that), splitting of handleRemote function. Switching to preventDefault instead of return false is OK also, I guess, that lets the event bubble up so user code can know it happened. This last thing, definitely make sure you do it in its own commit (because it's a feature change).

Parts that I don't like: indentation changes (indentation is currently one tab; I might change it to 2 spaces later on), extending $.fn (rails.js is not a jQuery plugin, it doesn't publish any public APIs), and naming of more than a few methods, mostly ones you use near the end of your changes (related to your $.fn extensions).

So, what I want you to do is redo (or rebase) your changes, grouping them one change at a time (try not to have "dryed some code" commits), starting with changes I liked, avoiding changing indentation, then you can end with changes I didn't like ($.fn extensions and non-descriptive function names) so I can review them again and think about how I can improve on what you already did.

Thanks for taking time to improve this code. Sorry if you think I'm being overly critical—I'm very sensitive about code style, naming, and grouping changes in a logical way (I sometimes spend even up to several hours rebasing my own feature branches in larger projects)

[dcrec1]

Sorry for the indentation changes, I'll adjust them to follow the JS standard.

About the jQuery prototype extensions, I thought the code was more readable like this but I can convert this calls to local functions if you think the extensions can cause any problem.

About the method names, are you talking about register and isMissing? If yes, I think that by themselves they're not too expressive but I think they fit well in the code:

.filter(isMissing).
.ifAllowedOn('click', register);

Anyway, just please confirm me what you didn't like so I can fix them.

About the commits, I tried already to commit one change at time. Can you please told me which one(s) do you thought it's too big so I can improve it?

Thanks for the feedback and don't feel sorry, I'm exactly like you :)

[neerajsingh0101]

I like some of individual commits but the pull request as a whole has a lot of changes. I would suggest lets move slowly.

Commit like dcrec1@ce01aad should be merged into some other commit.

Anyway let's start over. How about sending a pull request with just these two commits:

dcrec1@4ce77d8

dcrec1@0748cac

Hopefully I am not being too critical. I am just trying to ensure that all your hard work gets merged. However in order to get code merged it is easier to look at individual pieces.

Let me know if you have any questions.

[neerajsingh0101]

I am closing this as part of clean up.

@diego if you upto then please make the changes incrementally and please send different pull requests for different thing. Thanks

[dcrec1]

Ok Neeraj, will pick some time to do it, thanks.