5 ms·

Ok, to take a real world example instead: if (url.startsWith('http://')) { vs. if (isAbsoluteUrl(url)) { If the next developer comes by in 2 months

by janpot 7y ago

Ok, to take a real world example instead: if (url.startsWith('http://')) { vs. if (isAbsoluteUrl(url)) { If the next developer comes by in 2 months to fix the case for https:// https:// urls (and protocol relative ones in 6 months), they'll immediately be able to spot the intention of the code and can easily fix it in the abstraction layer that's already in place. Moreover, the fix will be applied every other place this faulty assumption about their input was made.

gmueckl 7y ago
Your example works because it avoids duplication a magic string constant. If the string wasn't a constant, but another variable, thw whole argument changes.
- janpot 7y ago
  My example is literally the real world version of the example in the article, only thing I did was change string values and identifier names. If you say my example works as an abstraction, it means that it successfully refutes the argument that the article tries to make.
GuB-42 7y ago
url.startsWith('http://' http://') may be better, especially if used only once. For example, what is "isAbsoluteUrl()" supposed to do? Does it match anything that starts with "word:", maybe "word://" maybe it is limited to supported protocols, what about validity, encoding, etc... For example "mailto:foo@bar.com" is an absolute URL, and a correct one that will work in your browser, but if your intention is to download something, it will fail. In fact, just by looking at isAbsoluteUrl(url), I don't have any idea about when the condition will be true. Maybe you want something more explicit like isAbsoluteDownloadableUrl(url). But still, it doesn't tell you everything, in particular, it doesn't tell you what kind of checking it does. Is it a full validation or just a quick check? And sure, you can add documentation and all that but in the end, that's typically how you add in 50 lines of code for no good reason. url.startsWith('http://' http://') may not be generic but what it does is crystal clear. And maybe, when https support will be added there will be a bug. But if isAbsoluteUrl() is just "return url.startsWith('http://')" http://')", it is still be a bug. I am not saying that isAbsoluteUrl(url) is bad, just that the article also applies to your "real world" example. And as mentioned in the end of the article "functions are still a good idea ... there is some balance to find here".
kixiQu 7y ago
Seems to me that this is a great example of how, if I'm the next developer, I will have to go dereference isAbsoluteUrl in order to see that the https:// https:// case hasn't already been handled--whereas it would have been obvious with the inlined version. Maybe isAbsoluteUrl is imported from a module I trust, and I won't think to go look at it for the problem. And "every other place" is a bit of a strawman, since everyone commenting seems to think if you have three cases of a thing, then the abstraction is warranted... so it'd have to be "in the other place".
- janpot 7y ago
  I never said you wouldn't have to dereference and check the implementation. That's debugging. I said that it would make my intent clear. That I wrote the code below that line with the assumption that url is absolute and not specifically only the 'http://' http://' case. The abstraction here is about encapsulating and communicating my intentions.

by janpot 7y ago

gmueckl 7y ago

Your example works because it avoids duplication a magic string constant. If the string wasn't a constant, but another variable, thw whole argument changes.

janpot 7y ago
My example is literally the real world version of the example in the article, only thing I did was change string values and identifier names. If you say my example works as an abstraction, it means that it successfully refutes the argument that the article tries to make.

GuB-42 7y ago

url.startsWith('http://' http://') may be better, especially if used only once. For example, what is "isAbsoluteUrl()" supposed to do? Does it match anything that starts with "word:", maybe "word://" maybe it is limited to supported protocols, what about validity, encoding, etc... For example "mailto:foo@bar.com" is an absolute URL, and a correct one that will work in your browser, but if your intention is to download something, it will fail. In fact, just by looking at isAbsoluteUrl(url), I don't have any idea about when the condition will be true. Maybe you want something more explicit like isAbsoluteDownloadableUrl(url). But still, it doesn't tell you everything, in particular, it doesn't tell you what kind of checking it does. Is it a full validation or just a quick check? And sure, you can add documentation and all that but in the end, that's typically how you add in 50 lines of code for no good reason. url.startsWith('http://' http://') may not be generic but what it does is crystal clear. And maybe, when https support will be added there will be a bug. But if isAbsoluteUrl() is just "return url.startsWith('http://')" http://')", it is still be a bug. I am not saying that isAbsoluteUrl(url) is bad, just that the article also applies to your "real world" example. And as mentioned in the end of the article "functions are still a good idea ... there is some balance to find here".

kixiQu 7y ago

Seems to me that this is a great example of how, if I'm the next developer, I will have to go dereference isAbsoluteUrl in order to see that the https:// https:// case hasn't already been handled--whereas it would have been obvious with the inlined version. Maybe isAbsoluteUrl is imported from a module I trust, and I won't think to go look at it for the problem. And "every other place" is a bit of a strawman, since everyone commenting seems to think if you have three cases of a thing, then the abstraction is warranted... so it'd have to be "in the other place".

janpot 7y ago
I never said you wouldn't have to dereference and check the implementation. That's debugging. I said that it would make my intent clear. That I wrote the code below that line with the assumption that url is absolute and not specifically only the 'http://' http://' case. The abstraction here is about encapsulating and communicating my intentions.