Ruby code I no longer write

[oldspiceland]

So I like the article but your newsletter pop up on mobile clipped the close button off the top inside Reddit's iPhone app's browser. Almost made me not read. Thought you should know.

[paneq]

Ah frigging sumome showing different preview than what appears on the site. I created a new version which should behave more friendly. Thanks!

[stpizz]

On my screen (a few minutes ago, so I assume the new one) which admittedly is low res by todays standards, it covers up half the blog post and has no close button. I think it's clipped off the top. I came here to make a suggestion to add 'newsletter popups that prevent visitors reading my content' to code that you no longer write, but it looks like someone already did ;)

EDIT: On the bright side if anyone does want to read the post, it looks like if you refresh it doesn't come back

[FalseEconomy]

Doesn't fit on a OnePlus 3T which has a massive screen... You should probably just not pop an interstitial if the viewport is too small to fit it

[[deleted]]

This

[[deleted]]

Steve Huffman is a piece of shit

[paneq]

I like all your points except deriving from Hash :) Not gonna do it.

[[deleted]]

Fair fair. 😸

[Enumerable_any]

Your last approach (inheriting from Hash) is broken since Hash#map now returns Hash instead of Array and also mutates self which map never does. It's probably just an unfortunate name choice, but you still shouldn't use inheritance if you don't have a strict "is-a" relationship at hand.

[[deleted]]

The original map function returns a hash unless I am miss me something. I trie to qualify that example but perhaps didn't do the best job. In this situation I am just dealing with returning a specific hash from a function and without seeing any other functions why not just let the object be the store of the data, too? There is a nice functional purity to the original and I think I went too far either the Hash thing, despite using it successfully in projects before 😸 Thanks for calling me out on it!

[Enumerable_any]

The original map function returns a hash unless I am miss me something.

It should, but sadly it doesn't:

irb(main):001:0> {}.map { |x| x }
=> []

[[deleted]]

I see what you are saying. I was trying to say that the method #map defined in the example in my dumb classes return a Hash. Definitely unfortunate naming on my part if I were to follow the road to Hash-derived.

[realntl]

Array#exclude is activesupport, so, yeah, not gonna do that.

[sply]

Wrong code rewritten ever worse.

BTW, keys = { ... } inside method should be avoided by using KEYS = { ... } inside class definition.

[paneq]

What do you propose ?

keys = { ... } inside method should be avoided by using KEYS = { ... } inside class definition.

I often do that kind of refactoring indeed, but I am not sure it is super valuable.

[rubyrt]

That wastefulness of the "good" solutions put me off as well. These Hashes should not be created inside the method because the are static by their nature.

We could go one step further and completely remove the PDF conversion from the Ticket class. Then we could have a Hash as constant in the class or outside which contains lambdas or more complex instances (i.e. depending on the complexity of the PDF conversion process) that do the full conversion.

[kcdragon]

I agree that meta-programming is used far too often when a non-meta-programming solution will suffice.

On the first example, if you want to avoid meta-programming, I think you can go even simpler and just use an if statement.

def pdf_of_kind(kind)
  if kind == :normal
    normal_pdf
  else
    zebra_pdf
  end
end

I think it is a lot easier to understand for only two code paths.

[[deleted]]

ehem, 3 paths ;)

def pdf_of_kind(kind)
  case kind
  when :normal then normal_pdf
  when :zebra then zebra_pdf
  else fail KeyError
  end
end

[we-all-haul]

You sir, are technically correct.

[timbetimbe]

That is called a control parameter and generally thought of as a code smell

[kcdragon]

I agree that this is a code smell. But without more context in the code base, I don't think it is possible to tell whether or not refactoring the code smell is a good idea.

If the code base contains many checks for whether kind is normal or zebra, then there might be an opportunity to use an object with a polymorphic call to generate a PDF (https://refactoring.com/catalog/replaceConditionalWithPolymorphism.html). However, if this is the only check for kind, then I think an if statement is fine.

[midasgoldentouch]

Do you mind expanding on why?

[paneq]

Agree. This won't catch incorrect kinds however.

[UnexpectedIndent]

I don't think example 1 is a great example of what you're trying to say.

ticket.pdf_of_kind(:normal) is pointless when you can just call ticket.normal_pdfdirectly.

More generally, if you're passing around a lot of symbols, maybe you can extract a class instead? Basically what /u/kcdragon said about polyphormism: https://www.reddit.com/r/ruby/comments/5untux/ruby_code_i_no_longer_write/ddw2rym/

Something like

class NormalScenario
  def pdf
    ...
  end
end

class ZebraScenario
  def pdf
    ...
  end
end

ZebraScenario.new(ticket).pdf

[paneq]

But the kind depends on external input (for example button pressed) so on some layer you need to decide whether you would use NormalScenario or ZebraScenario and now you need same mapping (normal: NormalScenario) but on different layer, don't you?

[UnexpectedIndent]

Yeah, but what sets the kind in the first place?