4 ms·

Our current principal engineer has implemented the visitor pattern for a bunch of things in our current project. It's a quite ugly implementation we shot down i

Our current principal engineer has implemented the visitor pattern for a bunch of things in our current project. It's a quite ugly implementation we shot down in code review and then we had an entire meeting where he defended it and eventually it got let in. He's used the visitor pattern in a few other areas as well. I'm not a big fan.

KirinDave 8y ago
This technique improves readability, improves abstraction, improves testability, and improves performance. Please don't confuse your superstitions or local culture's distaste for properly researched architecture for a more general sense of code quality. Simply saying, "I saw the word visitor and got mad," not only is disrespectful to the author, but it's disrespectful to your entire profession.
- fenwick67 8y ago
  You and I weren't there so without details it's hard to say whether it's sensible or over-engineered
  - KirinDave 8y ago
    Given the fact that this is a combination of well-known techniques, trivially offers performance benefits, and makes the transformation part of the code testable in isolation from the process or recursion, it's a smart first pass. I think most experienced OO engineers could sit down and write this in one pass. Calling it overengineered is pretty ridiculous IMO. "Over-engineered" doesn't mean, "Because software training is often inadequate someone might encounter this for the first time in industry rather than in school."
- phyzome 8y ago
  Visitor pattern definitely does not automatically improve readability or clarity. (In the general case! Summing values would be an exception.) In fact, I'd see it as a performance/readability tradeoff in any language where recursive tree processing is ergonomic. Not even knowing the language involved, there's really not much to say.
  - KirinDave 8y ago
    I'm not referring to the Visitor pattern in Abstract, I'm referring to the specific solution the author of the article we are discussing employed. There is no such thing as a pattern that always increases clarity. That's not what patterns are for, so that's unsurprising. It's the case though that this specific application has pretty much only benefits.

KirinDave 8y ago

This technique improves readability, improves abstraction, improves testability, and improves performance. Please don't confuse your superstitions or local culture's distaste for properly researched architecture for a more general sense of code quality. Simply saying, "I saw the word visitor and got mad," not only is disrespectful to the author, but it's disrespectful to your entire profession.

fenwick67 8y ago
You and I weren't there so without details it's hard to say whether it's sensible or over-engineered
- KirinDave 8y ago
  Given the fact that this is a combination of well-known techniques, trivially offers performance benefits, and makes the transformation part of the code testable in isolation from the process or recursion, it's a smart first pass. I think most experienced OO engineers could sit down and write this in one pass. Calling it overengineered is pretty ridiculous IMO. "Over-engineered" doesn't mean, "Because software training is often inadequate someone might encounter this for the first time in industry rather than in school."
phyzome 8y ago
Visitor pattern definitely does not automatically improve readability or clarity. (In the general case! Summing values would be an exception.) In fact, I'd see it as a performance/readability tradeoff in any language where recursive tree processing is ergonomic. Not even knowing the language involved, there's really not much to say.
- KirinDave 8y ago
  I'm not referring to the Visitor pattern in Abstract, I'm referring to the specific solution the author of the article we are discussing employed. There is no such thing as a pattern that always increases clarity. That's not what patterns are for, so that's unsurprising. It's the case though that this specific application has pretty much only benefits.