4 ms·

The problem with if (page.hasAttribute(“Test”)) {… is when you then decide to change boolean attribute Test to an enum attribute Environment with values Dev

The problem with if (page.hasAttribute(“Test”)) {… is when you then decide to change boolean attribute Test to an enum attribute Environment with values Development, Test and Production. Because now you have to hunt down all the page.hasAttribute(“Test”) in your code and replace them with something different. And if your code is creepy enough you will miss one or two and something will break. So you have to look very carefully whether you repeat the same checks. If you repeat a check in different places and it has some meaning, you should abstract it away.

engtech 14y ago
The bigger problem I had with that snippet is the exposed string. I haven't seen the original video this is from, but I can't wonder but removing this hardcoded string was the bigger reason. Encoded strings like this are a maintenance problem lying in wait. How do you grep/search for it? These are all equivalent: if (page.hasAttribute("Test")) {… if (page.hasAttribute('Test')) {… string test = "Test"; if (page.hasAttribute(test)) {… string cheese = "Test"; if (page.hasAttribute(cheese)) {…
hapless 14y ago
The core problem is that it moves part of your OO interface into data. With "isTestPage()" you have a method with well-specified outputs. With "page.hasAttribute('Test')," a load of things become part of the permanent interface of the class: hasAttribute(), the datatype of 'Test,' the content of the field in the constructed argument etc...
anonymous 14y ago
That's simple - change the method so all those invocations are invalid now. If you're using a compiled language, the compiler will tell you all the lines that need changing. If you're using a dynamic language, you should have test coverage for all those methods and your tests will blow up with "no such method". Or maybe, you could special-case the 'hasAttribute(name)' method so it logs a debug warning and checks the enum attribute. Really, there are enough ways to deal with that and you're far from the first person to have had that problem.
- guard-of-terra 14y ago
  "you could special-case the 'hasAttribute(name)' method" Please don't do this never.
  - jtheory 14y ago
    I believe "temporarily" was implied.
    - guard-of-terra 14y ago
      And when are you going to take out that? If you leave, who will take care of this hack?
      - jtheory 14y ago
        This isn't my argument -- I've never taken this approach -- but if the need came up, it would never go into source control; it'd only be there long enough to run test suites locally and find the broken code that needed fixing.
h2s 14y ago
This is exactly what I thought. The purpose of the "isTestPage" method is to hide the implementation. It's a basic OO principle, not some controversy stirred up by Robert C Martin. I hated seeing somebody argue against it because we'd be better off as developers if more people understood this and made a habit of hiding implementation to this extent.

engtech 14y ago

The bigger problem I had with that snippet is the exposed string. I haven't seen the original video this is from, but I can't wonder but removing this hardcoded string was the bigger reason. Encoded strings like this are a maintenance problem lying in wait. How do you grep/search for it? These are all equivalent: if (page.hasAttribute("Test")) {… if (page.hasAttribute('Test')) {… string test = "Test"; if (page.hasAttribute(test)) {… string cheese = "Test"; if (page.hasAttribute(cheese)) {…

hapless 14y ago

The core problem is that it moves part of your OO interface into data. With "isTestPage()" you have a method with well-specified outputs. With "page.hasAttribute('Test')," a load of things become part of the permanent interface of the class: hasAttribute(), the datatype of 'Test,' the content of the field in the constructed argument etc...

anonymous 14y ago

That's simple - change the method so all those invocations are invalid now. If you're using a compiled language, the compiler will tell you all the lines that need changing. If you're using a dynamic language, you should have test coverage for all those methods and your tests will blow up with "no such method". Or maybe, you could special-case the 'hasAttribute(name)' method so it logs a debug warning and checks the enum attribute. Really, there are enough ways to deal with that and you're far from the first person to have had that problem.

guard-of-terra 14y ago
"you could special-case the 'hasAttribute(name)' method" Please don't do this never.
- jtheory 14y ago
  I believe "temporarily" was implied.
  - guard-of-terra 14y ago
    And when are you going to take out that? If you leave, who will take care of this hack?
    - jtheory 14y ago
      This isn't my argument -- I've never taken this approach -- but if the need came up, it would never go into source control; it'd only be there long enough to run test suites locally and find the broken code that needed fixing.

h2s 14y ago

This is exactly what I thought. The purpose of the "isTestPage" method is to hide the implementation. It's a basic OO principle, not some controversy stirred up by Robert C Martin. I hated seeing somebody argue against it because we'd be better off as developers if more people understood this and made a habit of hiding implementation to this extent.