4 ms·

That's a huge improvement but damn, the fixed code didn't look any better in my eyes. Going from HANDLERS = [ Text, List, ListItem, Code,

That's a huge improvement but damn, the fixed code didn't look any better in my eyes. Going from HANDLERS = [ Text, List, ListItem, Code, # ... ].freeze to HANDLERS_BY_NODE_NAMES = [ Text, List, ListItem, Code, # ... ].each_with_object({}) do |handler, result| handler::NODE_NAMES.each { |node_name| result[node_name] = handler } end.freeze

viraptor 2y ago
I'd go with this since it's not performance critical code. Not sure if it's that more readable, but I like it better: BY_NODE_NAMES = HANDLERS.map {|h| h::NODE_NAMES.map {|n| [n, h]} }.flatten(1).to_h
- ngcazz 2y ago
  I think we can go harder on the std lib :) HANDLERS.flat_map { _1.node_names.index_with(_1) }.inject(&:merge) (nb: assuming there exists a `.node_names` to expose the constant... just because I like always using method calls)
masklinn 2y ago
A bigger question for me would be why the handlers don’t register themselves. It should be a very small amount of meta-programmation, and would avoid having to repeat the handlers to register them.
- IshKebab 2y ago
  Self-registration is usually an anti-pattern in my experience because it introduces globals. Sometimes you can't avoid it, but if you only have a few things to register it's usually better to just list them explicitly.
  - masklinn 2y ago
    > Self-registration is usually an anti-pattern in my experience because it introduces globals. You mean unlike explicit registration introducing the HANDLERS_BY_NODE_NAMES global?
    - IshKebab 2y ago
      Well I guess it isn't quite as simple as I implied. The handlers don't know anything about HtmlHandler, so you are free to make OtherHtmlHandler or whatever. The dependency direction is correct, whereas with self-registration your handlers now depend on some single unique global registry. HANDLERS_BY_NODE_NAMES doesn't affect any other code that might interact with the handlers (tests is normally the big one). barrkei gave some very good reasons to avoid self-registration.
  - barrkel 2y ago
    Self-registration has a lot of problems. Global state, like you say - you can't merge two apps that use the same registration mechanism but use incompatible registered sets. Lack of discoverability in maintenance - it's a kind of COME FROM but for data. A barried to optimization: it's not clear what will break when you remove a dependency, and you have to eagerly run all initializers everywhere - if you try to be clever and lazy, you have to pick and choose, and you're back to explicit but indirect registration. Initialization order problems: if you have code you run during init which depends on the stuff you register at init time, you're going to have to manually manage your initialization in error-prone ways. Adding or removing dependencies may change initialization order.
deleted 2y ago
[deleted]

viraptor 2y ago

I'd go with this since it's not performance critical code. Not sure if it's that more readable, but I like it better: BY_NODE_NAMES = HANDLERS.map {|h| h::NODE_NAMES.map {|n| [n, h]} }.flatten(1).to_h

ngcazz 2y ago
I think we can go harder on the std lib :) HANDLERS.flat_map { _1.node_names.index_with(_1) }.inject(&:merge) (nb: assuming there exists a `.node_names` to expose the constant... just because I like always using method calls)

masklinn 2y ago

A bigger question for me would be why the handlers don’t register themselves. It should be a very small amount of meta-programmation, and would avoid having to repeat the handlers to register them.

IshKebab 2y ago
Self-registration is usually an anti-pattern in my experience because it introduces globals. Sometimes you can't avoid it, but if you only have a few things to register it's usually better to just list them explicitly.
- masklinn 2y ago
  > Self-registration is usually an anti-pattern in my experience because it introduces globals. You mean unlike explicit registration introducing the HANDLERS_BY_NODE_NAMES global?
  - IshKebab 2y ago
    Well I guess it isn't quite as simple as I implied. The handlers don't know anything about HtmlHandler, so you are free to make OtherHtmlHandler or whatever. The dependency direction is correct, whereas with self-registration your handlers now depend on some single unique global registry. HANDLERS_BY_NODE_NAMES doesn't affect any other code that might interact with the handlers (tests is normally the big one). barrkei gave some very good reasons to avoid self-registration.
- barrkel 2y ago
  Self-registration has a lot of problems. Global state, like you say - you can't merge two apps that use the same registration mechanism but use incompatible registered sets. Lack of discoverability in maintenance - it's a kind of COME FROM but for data. A barried to optimization: it's not clear what will break when you remove a dependency, and you have to eagerly run all initializers everywhere - if you try to be clever and lazy, you have to pick and choose, and you're back to explicit but indirect registration. Initialization order problems: if you have code you run during init which depends on the stuff you register at init time, you're going to have to manually manage your initialization in error-prone ways. Adding or removing dependencies may change initialization order.

deleted 2y ago

[deleted]