0
\$\begingroup\$

Given a map of items to their attributes:

    OPTIONS = {
      paper: %w[A4_paper countable universal],
      pencil: %w[trackable universal trackable],
      chalk: %w[stationery countable trackable A4_paper],
    }.freeze

I need to return the items that have a particular attribute. For example, fetch_type_values('countable') should return ["paper", "chalk"].

Here's my working implementation:

def fetch_type_values(option)
 OPTIONS.map { |key, value| key if value.include?(option.to_s) }.compact.map(&:to_s)
end

I feel that this is a bit difficult to read; How can I refactor/simplify it to make it more readable?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I changed the question so that it describes the purpose, to the best of my understanding. Please check that I haven't misunderstood, and correct it if I have. Thanks! \$\endgroup\$ Commented Dec 22, 2022 at 10:10
  • \$\begingroup\$ Looks good to me. My only complaint is you're calling .to_s() more times than necessary -- but that's a concise readability versus CPU cycles tradeoff. For "small" option arguments, the code as written is better than assigning a temp var. \$\endgroup\$ Commented Dec 23, 2022 at 16:15

1 Answer 1

1
\$\begingroup\$

I would use select since that is what you are really doing

def fetch_type_values(option)
 OPTIONS.select { |_key, value| value.include?(option.to_s) }.map(&:to_s)
end

I also wouldn't stringify the result. I would keep the options as symbols since that is your internal representation. I would even make the attributes symbols. i.e.

    OPTIONS = {
      paper: %i[A4_paper countable universal],
      pencil: %i[trackable universal trackable],
      chalk: %i[stationery countable trackable A4_paper],
    }.freeze
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.