Overkill use of regex #13

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@blixt

blixt commented Oct 19, 2010

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

This comment has been minimized.

Show comment
Hide comment
@cowboy

cowboy Oct 19, 2010

Owner

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.
Owner

cowboy commented Oct 19, 2010

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

This comment has been minimized.

Show comment
Hide comment
@blixt

blixt Oct 19, 2010

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.

blixt commented Oct 19, 2010

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

This comment has been minimized.

Show comment
Hide comment
@cowboy

cowboy Oct 19, 2010

Owner

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!

Owner

cowboy commented Oct 19, 2010

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!

Andreas Blixt
Changed regex to a simple string operation. Should be more readable a…
…nd performant. Also updated comment to be consistent with what is returned (behavior has not been changed.)
@blixt

This comment has been minimized.

Show comment
Hide comment
@blixt

blixt Oct 19, 2010

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 commented Oct 19, 2010

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

This comment has been minimized.

Show comment
Hide comment

blixt commented Jun 17, 2011

Ping :)

@cowboy

This comment has been minimized.

Show comment
Hide comment
@cowboy

cowboy Jun 17, 2011

Owner

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!

Owner

cowboy commented Jun 17, 2011

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!

@blixt blixt closed this Apr 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment