Reduce duplication with Processor::API::Responder #7
Conversation
| # @api private | ||
| def define_abstract_helper_method(host) | ||
| host.class_eval do | ||
| define_method(:respond_with) do |_klass, _output| |
There was a problem hiding this comment.
You might be able to use abstract_type to remove some of this.
There was a problem hiding this comment.
@dkubb not really, as i also include that module into non-abstract classes which provide an implementation of #respond_with (Substation::Request)
|
I had to use |
|
I don't like that. Lots of unnecessary meta-programming. You also didn't remove duplication, you just replaced 2 similar lines of code with much more complex code. Not worth it IMHO. |
|
as discussed in irc, i tend to agree with @solnic ... |
|
Actually, thinking about it from that pov I agree with @solnic. I was more or less just thinking that maybe this pattern was repeated in lots more places, or would be, then I wouldn't have an issue with it. If all this work is just to remove two lines of code then maybe it should be skipped. |
|
@dkubb yeah, as i mentioned in irc, initially i thought it would be usable in those 2 places PLUS be useful to substation clients too. Turns out i was wrong with that latter assumption. While it could be used by substation clients, there's already a nicer, more straightforward way of doing something to the same effect (if you're interested, I'm talking about this: https://github.com/snusnu/substation/blob/master/spec/app.rb#L112-L118) Also, there's another argument against this. The code actually dynamically defines public api methods, which robs me of the possibility to properly document those. That's fairly severe I guess. I'll wait for @mbj to chip in, but currently, I lean towards nuking that branch and happily move forward using current master. EDIT: typo |
|
I just want to drop one more (mostly useless) comment. Metaprogramming is just programming after all. The code still does nothing more than define three methods, but it does it once now. The "ceremony" needed for the meta part in metaprogramming can be considered complex, but i guess there are other ways to look at that too. |
|
Yeah, I don't really make too many distinctions between programming and metaprogramming. I can read and understand what both examples do without too much effort. In terms of complexity the original code is probably simpler. Still, it's not like it's doing anything we haven't seen before.. this is a fairly common pattern in some of the support libs we're using. I think the original is probably clearer in terms of intent though. And there are likely other ways to DRY it up that may end up being simpler. |
|
It's just not worth it .... |
@dkubb @mbj @solnic
I'm opening a PR for this because I wanted to get your input. Do you consider this a worthy change? It feels like reducing duplication like this, results in more complexity. Even though flog doesn't show an increase, I do consider the code added in
Processor::API::Responderto be more complex than the previously present duplication. Also, flay count didn't decrease either.I somewhat think that I still like the code in this PR more than the previous version, but I wanted to get your thoughts on a decision like this.
Thx!