madmurphy / cookies.js

Simple cookie framework with full Unicode support
GNU General Public License v3.0
265 stars 54 forks source link

[fix] remove redundant escape characters #15

Closed rvalitov closed 6 years ago

rvalitov commented 6 years ago

Making the code cleaner.

rvalitov commented 6 years ago

A next PR is ready that provides automatic generation of .min.js and .min.map files.

madmurphy commented 6 years ago

@rvalitov

Thank you for your pull request. Do you by coincidence use eslint? I already had a discussion about this topic once.

I don't know why some debuggers decide that the escape character in regular expressions must be used only when there would be ambiguity otherwise. In my opinion 1) A character that can represent a metacharacter in regular expressions must be always escaped, in order to avoid bugs difficult to find when the regular expression gets modified by the programmer; 2) From the perspective of human readability /[\-.+*]/g is much more ambiguous to guess than /[\-\.\+\*]/g: even if there is no formal ambiguity in the first case, a human eye would probably first see \-., and then the square brackets that surround the characters, making readability at first sight definitely less natural.

--madmurphy

rvalitov commented 6 years ago

Thank you for the comments! Sometimes I use eslint, but not this time. This time it was a built-in code inspection by PHPStorm. I think that nowadays this type of code optimization (warning) is used in all IDEs. I understand your opinion. I think metacharacter should be escaped only if necessary. I'm a perfectionist: I don't like unnecessary or useless code đŸ˜„ Besides, regexps are quite easy to understand if you deal with them, so I think there's no benefits in escaping.

Please, let me know about your final decision, if you accept this PR or not. Because my next PR depends on this one: I will pull it anyway, but some parts of the code depend if you accept this PR or not. So I need to make it compatible.

madmurphy commented 6 years ago

I think metacharacter should be escaped only if necessary. I'm a perfectionist: I don't like unnecessary or useless code smile

I guess you never put a semicolon at the end of a statement in JavaScript then. It's definitely unnecessary for JS engines…

I think I will skip this one. But I will be happy to review your next PR!

--madmurphy

rvalitov commented 6 years ago

I guess you never put a semicolon at the end of a statement in JavaScript then. It's definitely unnecessary for JS engines…

Haha, no, because semicolon is required by all JS code style standards:)) I like to use tools like that to check code quality, find any warnings or errors automatically, etc. The next PR is coming....

rvalitov commented 6 years ago

One of the benefits of not using escaping. We have a line of code:

/^(?:expires|max\-age|path|domain|secure)$/i

In this case above we can't use "search or replace" in the editor for max-age string. We should keep in mind that we actually may have max\-age syntax. That makes the coding more complicated and may cause new errors in the future when we will forget about that one day. Besides, now many IDEs have refactoring tools and you can rename a variable or constant in one click, while the IDE does all the work for you to find the occurrence of the original name in different parts of the code, comments, etc. When you use extra escaping, then IDE may fail to find all the occurrences. Of course, this explanation may not be very important for this project which is quite small and has a few lines of code that is easy to read. But for bigger projects it can make the work more comfortable. As I said in the previous message, it's just a coding style - so, we may or may not follow that. But I think following has more benefits than not.

rvalitov commented 6 years ago

I checked again what's new in PHP 7.3 and there we need to escape some characters that we didn't have to escape untill PHP 7.3. The reason is that PHP before 7.3 use PCRE syntax while PHP 7.3 uses PCRE 2. As a result, it's better to close this PR.