3
\$\begingroup\$

I have a Rails helper that swaps out some text for certain icons.

Example:

Helper Sample

...
            '+1' => '<i class="ms ms-loyalty-up ms-loyalty-1"></i>',
            '+2' => '<i class="ms ms-loyalty-up ms-loyalty-2"></i>',
            '−1' => '<i class="ms ms-loyalty-down ms-loyalty-1"></i>',
            '−2' => '<i class="ms ms-loyalty-down ms-loyalty-2"></i>',
            '−10' => '<i class="ms ms-loyalty-down ms-loyalty-10"></i>',
            '{T}' => '<i class="ms ms-tap"></i>',
            '{P}' => '<i class="ms ms-tap"></i>',
            '{C}' => '<i class="ms ms-c"></i>'
            }
card.gsub(/\{.+?\}|\+\d+|\−\d+|\{[T]}|X/){|k| hash[k] || k }.html_safe 

There is about 30 lines above that, but I'm about to have to duplicate this +1, +2, -1, -2 one etc to include every number between 1 and 20. This seems -extremely- poor practice and I'm wondering if there is a better way to do it? Below is my entire helper, and I might be able to cross apply it to text too, but it seems very silly to me to have to do it for all numbers, both positive and negative.

(As you can tell ms-loyalty-down is for negative numbers and ms-loyalty-up is for positive.) I was thinking about a 'case' and 'when case +' loyalty-up, but I wasn't getting the number appended to the end, nor was it continuing through the cycle.

Entire Helper

module CardsHelper
    def card_text_swap card
        if card.nil ?
            return
        else
            hash = {
                '{hw}' => '<span class="ms-half"> <i class = "ms ms-w ms-cost"> </i> </span>', 
                '{W}' => '<i class="ms ms-w ms-cost ms-shadow"></i>',
                '{R}' => '<i class="ms ms-r ms-cost ms-shadow"></i>',
                '{U}' => '<i class="ms ms-u ms-cost ms-shadow"></i>',
                '{G}' => '<i class="ms ms-g ms-cost ms-shadow"></i>',
                '{B}' => '<i class="ms ms-b ms-cost ms-shadow"></i>',
                '{S}' => '<i class="ms ms-s ms-cost ms-shadow"></i>',
                '{X}' => '<i class="ms ms-x ms-cost ms-shadow"></i>',
                'X' => '<i class="ms ms-x ms-cost ms-shadow"></i>',
                '{W/U}' => '<i class="ms ms-wu ms-split ms-cost"></i>',
                '{W/B}' => '<i class="ms ms-wb ms-split ms-cost"></i>',
                '{W/P}' => '<i class="ms ms-wp ms-split ms-cost"></i>',
                '{2/W}' => '<i class="ms ms-2w ms-split ms-cost"></i>',
                '{U/B}' => '<i class="ms ms-ub ms-split ms-cost"></i>',
                '{U/R}' => '<i class="ms ms-ur ms-split ms-cost"></i>',
                '{U/P}' => '<i class="ms ms-up ms-split ms-cost"></i>',
                '{2/U}' => '<i class="ms ms-2u ms-split ms-cost"></i>',
                '{B/R}' => '<i class="ms ms-br ms-split ms-cost"></i>',
                '{B/G}' => '<i class="ms ms-bg ms-split ms-cost"></i>',
                '{B/P}' => '<i class="ms ms-bp ms-split ms-cost"></i>',
                '{2/B}' => '<i class="ms ms-2b ms-split ms-cost"></i>',
                '{R/G}' => '<i class="ms ms-rg ms-split ms-cost"></i>',
                '{R/P}' => '<i class="ms ms-rp ms-split ms-cost"></i>',
                '{R/W}' => '<i class="ms ms-rw ms-split ms-cost"></i>',
                '{2/R}' => '<i class="ms ms-2r ms-split ms-cost"></i>',
                '{G/W}' => '<i class="ms ms-gw ms-split ms-cost"></i>',
                '{G/B}' => '<i class="ms ms-gb ms-split ms-cost"></i>',
                '{G/P}' => '<i class="ms ms-gp ms-split ms-cost"></i>',
                '{2/G}' => '<i class="ms ms-2g ms-split ms-cost"></i>',
                '{0}' => '<i class="ms ms-0 ms-cost ms-shadow"></i>',
                '{1}' => '<i class="ms ms-1 ms-cost ms-shadow"></i>',
                '{2}' => '<i class="ms ms-2 ms-cost ms-shadow"></i>',
                '{3}' => '<i class="ms ms-3 ms-cost ms-shadow"></i>',
                '{4}' => '<i class="ms ms-4 ms-cost ms-shadow"></i>',
                '{5}' => '<i class="ms ms-5 ms-cost ms-shadow"></i>',
                '{6}' => '<i class="ms ms-6 ms-cost ms-shadow"></i>',
                '{7}' => '<i class="ms ms-7 ms-cost ms-shadow"></i>',
                '{8}' => '<i class="ms ms-8 ms-cost ms-shadow"></i>',
                '{9}' => '<i class="ms ms-9 ms-cost ms-shadow"></i>',
                '{10}' => '<i class="ms ms-10 ms-cost ms-shadow"></i>',
                '+1' => '<i class="ms ms-loyalty-up ms-loyalty-1"></i>',
                '+2' => '<i class="ms ms-loyalty-up ms-loyalty-2"></i>',
                '−1' => '<i class="ms ms-loyalty-down ms-loyalty-1"></i>',
                '−2' => '<i class="ms ms-loyalty-down ms-loyalty-2"></i>',
                '−10' => '<i class="ms ms-loyalty-down ms-loyalty-10"></i>',
                '{T}' => '<i class="ms ms-tap"></i>',
                '{P}' => '<i class="ms ms-tap"></i>',
                '{C}' => '<i class="ms ms-c"></i>'
            }
            card.gsub(/\{.+?\}|\+\d+|\−\d+|\{[T]}|X/) { | k | hash[k] || k
            }.html_safe
        end
    end
end
\$\endgroup\$
  • \$\begingroup\$ In the refactored version, you need hash = hash.merge(repetitive_cost_entries) or use the mutating version (i don't recommend this) hash.merge!(repetitive_cost_entries). also, use a guard clause at the top instead of a big if... else: return if card.nil?. finally, just a naming nitpick, i'd remove the repetitive_ prefix from the vars at the top. in my example i'd only included it so that, conceptually, it would be clear what i was suggesting. \$\endgroup\$ – Jonah Nov 30 '16 at 20:43
  • \$\begingroup\$ @Jonah You're right. I forget about that, such a simple fix. I appreciate the nit-picking! Naming conventions is a failure of mine. \$\endgroup\$ – DNorthrup Nov 30 '16 at 20:51
  • \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg Nov 30 '16 at 20:54
3
\$\begingroup\$

You can create the entries programmatically:

repetitive_entries = (1..10).map do |i|
  ["+#{i}", "<i class=\"ms ms-loyalty-up ms-loyalty-#{i}\"></i>"]
end.to_h

So then your overall strategy would be:

  1. Create the one-off entries in card as you have now
  2. Create the repetitive entries that differ only by a number
  3. Repeat step 2 as many times as needed, eg, for the "up" and "down" versions.
  4. Use array merge to combine the hashes from the previous steps into one large hash.

This allows you to keep your current approach, which I like for its simplicity, and avoid repeating yourself.

\$\endgroup\$
  • \$\begingroup\$ I actually like this. Still seems a bit repetitive, but isn't -nearly- as bad as my current usage. I'll try it out. \$\endgroup\$ – DNorthrup Nov 30 '16 at 19:35
  • \$\begingroup\$ you could certainly go one level of abstraction higher and avoid all the repetition. my approach would be to start with the rewrite above, then re-evaluate after seeing the new code, and decide if the gain in compactness would be worth the cognitive overhead of the new abstraction. it's a good general principle when refactoring \$\endgroup\$ – Jonah Nov 30 '16 at 20:01
  • \$\begingroup\$ Do you see anything wrong with my refactored version? I actually checked in console and it's giving me the results I would expect, but with the gsub I'm no longer getting proper expectations (AKA The number is still being output, not the <I>) \$\endgroup\$ – DNorthrup Nov 30 '16 at 20:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.