Hacker News Reader

Comment by guard-of-terra | Hacker News Reader

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.

View on HN

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)) {…

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.

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.

"you could special-case the 'hasAttribute(name)' method"

Please don't do this never.

I believe "temporarily" was implied.

And when are you going to take out that? If you leave, who will take care of this hack?

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.

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

Reply on news.ycombinator.com