Overkill use of regex

[blixt]

I just came across a regex when looking through the source code, and it looked so horrible, I had to fix it :) See attached commit.

[cowboy]

Andreas, I've got to be honest here, while I love community contributions, this is a perfect example of how not to make a pull request.

• You don't offer any objective reasoning why your code is better than the existing code or mention a specific issue that your change addresses. Following a snide remark with a smiley doesn't make your statement cute, it just makes you seem like an asshole.
• Your code isn't formatted in the same style as the surrounding code (extraneous parens, == instead of ===, crowded expressions), which makes it harder (albeit in this case, only slightly) to integrate your changes. While I haven't written an explicit styleguide, it should be fairly clear what the style expectations are just by looking at the rest of the code.
• Your code is actually broken, because while you reference url || location.href the first time, you reference just location.href the second time. Setting url = url || location.href; initially (like the original code did) would probably have been smart.

[blixt]

Okay sorry, I guess I should have just submitted a diff and not a pull request, but I found it to be much more convenient. And my comment was not meant to be snide, just playing on a very common topic (regexes being horrible.)

I tried to follow the code syntax in the rest of the code (I found ambiguous use of parens spacing and extraneous parens for clarity, so that's probably why there was some confusion.)

And yes, you're right, the broken code was a stupid mistake by me.

I'll close with an objective reasoning: Anyone with an understanding of basic string operations will understand code that finds a hash character and gets the substring after it. However, the regex was written in such a way that it might even be confusing to someone who understands them. If you were to use a regex at all, it would be much clearer if you were to use the following:

var match = url.match(/#.*/);
return match ? match[0] : '#';

I still recommend you to implement this change, and not throw it away in spite.

[cowboy]

Andreas, I appreciate the pull request, and I encourage you to fix it (so that it's not broken, at the bare minimum) and re-submit it (I'll re-open this issue).

I'm a busy guy, so it's especially challenging for me to spend time right now on contributions that need a lot of back-and-forth, but that being said, I do appreciate them. I hope it's clear that my main issues were simply that your pull request comment wasn't particularly constructive, and that your code was broken.

Either way, I'd love to replace that regexp with code that is more clear and/or performant. If you notice any other areas for improvement, please let me know. Thanks!

[blixt]

Yup, I'm busy too, hence the little time spent on my submission, making it messy. But it was in a way disrespectful of me, so I took the time now to do it properly. I removed the old commit and added a new one that should be better. :)

[blixt]

Ping :)

[cowboy]

I'm totally aware this issue exists, but it's a bit low priority for me right now. I've been refactoring a lot of code lately, and this is just another jQuery plugin on the (very long) list!