Timeline for Versatile string validation
Current License: CC BY-SA 4.0
21 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Jan 31, 2019 at 17:42 | comment | added | Adriano Repetti | I agree but only if you have to validate one single property. | |
| Jan 31, 2019 at 16:16 | comment | added | jpmc26 | @AdrianoRepetti Okay, fine, but that's only reinforcing my point. Abstract when the complexity grows and not before. Simple is better when it's possible. | |
| Jan 31, 2019 at 16:02 | comment | added | Adriano Repetti | I agree that OP took it the wrong way, that's why in the comment I wrote (emphasis added now): "...Having reusable... NOT the way OP did...". | |
| Jan 31, 2019 at 15:48 | comment | added | jpmc26 | @AdrianoRepetti And in fact, it would be harder to fix these problems with the OP's code because it's designed to process one character at a time. You'd have to change that fact to deal with combining characters, which means scrapping every line of code he has. Would you rather scrap 7 lines of code like my method or scrap 50 like his? These problems demonstrate that the time spent on all the OP's boilerplate was not a good investment. | |
| Jan 31, 2019 at 15:46 | comment | added | jpmc26 | @AdrianoRepetti Your objections boil down to "Unicode is hard," and that is 100% true, especially when you get into the realm of combining characters. But deal with those problems with this method is not any harder. The OP's code notably suffer these same problems, and it's even harder to detect them because the logic is so scattered. If dealing with those cases led to a level of complexity where inlining each check became unwieldy, I'd obviously factor the checks out into a separate static method. But until it does become that complex, there's no reason to insist on separating them. | |
| Jan 31, 2019 at 11:13 | comment | added | Adriano Repetti | If you are thinking that "𠀑" is not a letter (!) and it's rightfully rejected then you should try with "ā" (LATIN SMALL LETTER A MACRON): it's accepted. If you think "ā" is OK but "𠀑" is not) then you should try with "ā" (LATIN SMALL LETTER A + COMBINING MACRON): it's rejected. As you can see to write some more code to handle (and test) this correctly once and for all is a VERY good investment. | |
| Jan 31, 2019 at 11:05 | comment | added | Adriano Repetti |
Let me add an example. Unfortunately requirements are vague and open to interpretation so let's assume - for the sake of discussion - that letter has the meaning an average Joe would assume (not a programmer who might think that it's a synonym for a Latin US-ASCII character). In this case this simple string "𠀑𠀑𠀑" passes your length validation test (because each 𠀑 is encoded as two UTF-16 code units then for your code its length is 6) and it's wrongfully rejected by Char.IsLetterOrDigit(). You obviously don't want to repeat this kind of tests each time you have length validation.
|
|
| Jan 31, 2019 at 10:58 | comment | added | Adriano Repetti | I partially agree with t3, I don't think this is "not testable" but I think that (assuming you will have similar requirements for different properties) writing ad-hoc code like this will make testing MUCH harder. Don't take me wrong, in your tests you will need to go through all the requirements (for each property), this isn't changed, but you want (must) test corner cases. And corner cases when handling text are A LOT and not so corner. Having reusable functions (NOT the way OP did but more general, even if - at first - you have to write more code) lets you test all those cases just ONCE. | |
| Jan 29, 2019 at 15:20 | comment | added | jpmc26 | @t3chb0t How is this not testable? You literally just have the test call the method and check the output without any boilerplate setup; that's maximum testability. Every line of code is an additional possibility for a mistake and therefore a bug. Every line of code is another line a reader has to understand. How can you look at this code and think it's harder to understand than 50 lines of extra methods? It's literally just a short list of conditions to check, just like the requirements are. What are the practical disadvantages of this code you're concerned about? | |
| Jan 29, 2019 at 12:44 | comment | added | Tvde1 | @t3chb0t This is testable... | |
| Jan 29, 2019 at 6:23 | comment | added | t3chb0t | Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes. - not that has to be tested but one that can be tested. Your code isn't testable and is so easy to make a mistake there that you wouldn't notice until something goes wrong for the client. This isn't an improvement. It's a great degradation of both readability and testability. | |
| Jan 29, 2019 at 6:21 | comment | added | t3chb0t | I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. - this is so not true. I wouldn't let this code pass to production. | |
| Jan 29, 2019 at 1:36 | comment | added | jpmc26 | @AdrianoRepetti Don't get me wrong. I love descriptive names. I made this one pretty descriptive, if not perfectly so. I just don't think it's possible to capture the entire description in the name without making it take up nearly the whole line. | |
| Jan 28, 2019 at 23:56 | comment | added | Adriano Repetti | Honestly I like it even less (and I wouldn't replace a good descriptive name with a comment) but as I said it's really just my POV | |
| Jan 28, 2019 at 23:43 | comment | added | jpmc26 | @AdrianoRepetti I added documentation instead of making the name too complicated. | |
| Jan 28, 2019 at 23:39 | history | edited | jpmc26 | CC BY-SA 4.0 |
added 145 characters in body
|
| Jan 28, 2019 at 23:38 | comment | added | Adriano Repetti |
Hmmmmm POV, I guess. To me a well written regex can express clearly the requirement while UnicodeLettersNumsHyphen has the disadvantages of regexes (less easy to write and read) without its advantages (concise, fast and complete). Nitpick: I'd change UnicodeLettersNumsHyphen to a meaningful name because you can't understand what the regex is for without fully decoding it.
|
|
| Jan 28, 2019 at 23:33 | comment | added | jpmc26 | @AdrianoRepetti Yes, but then it's not simple anymore. Putting most of these checks outside the regex vastly improves readability for very little cost. | |
| Jan 28, 2019 at 23:30 | history | edited | jpmc26 | CC BY-SA 4.0 |
added 145 characters in body
|
| Jan 28, 2019 at 23:30 | comment | added | Adriano Repetti | I'd tend to agree to use a single regex but in that case everything can be expressed with a single regex without any other surrounding code (including the length and the first/last character) | |
| Jan 28, 2019 at 23:25 | history | answered | jpmc26 | CC BY-SA 4.0 |