DRY for Ruby variable loops

Question

I have a def in ruby like below. Can I do something to make it DRY? like:

[e,t,l,te,le,le].each |xxx| do
  if xxx
end

Which means do the loops for "Variables", not only "Enumerator".

code:

def findLogs (obj)
  if e=obj['E']
    e=obj['E']['pkg'] ? "@E = #{obj['E']['pkg']},":nil
  else nil
  end
  if t=obj['T']
    t=obj['T']['pkg'] ? "@T = #{obj['T']['pkg']},":nil
  else nil
  end
  if l=obj['L']
    l=obj['L']['pkg'] ? "@L = #{obj['L']['pkg']},":nil
  else nil
  end
  if te=obj['Te']
    te=obj['Te']['pkg'] ? "@Te = #{obj['Te']['pkg']},":nil
  else nil
  end
  if le=obj['Le']
    le=obj['Le']['pkg'] ? "@Le = #{obj['Le']['pkg']},":nil
  else nil
  end
end

possible if you remove your variables and make them keys in a hash, or something. Hash keys are much easier to iterate. so te = obj[...] becomes my_hash['te'] = obj[...] — Sergio Tulentsev
– Sergio Tulentsev, Commented Dec 15, 2016 at 9:40
What is your code supposed to do? The local variables are pointless if you don't use them. — Stefan
– Stefan, Commented Dec 15, 2016 at 9:45
there is something to do with these variables in the def that I didn't paste here :) — L_G
– L_G, Commented Dec 16, 2016 at 2:46

Aleksei Matiushkin · Accepted Answer · 2016-12-15 09:58:13Z

4

e, t, l, te, le = %w|E T L Te Le|.map do |s|
  obj[s] && obj[s]['pkg'] ? "@#{s} = #{obj[s]['pkg']}," : nil
end

For Sergio Tulentsev:

b = binding
%w|E T L Te Le|.each do |s|
  b.local_variable_set(
    s.downcase.to_sym,
    obj[s] && obj[s]['pkg'] ? "@#{s} = #{obj[s]['pkg']}," : nil
  )
end

edited Dec 15, 2016 at 9:58

answered Dec 15, 2016 at 9:42

Aleksei Matiushkin

121k12 gold badges109 silver badges173 bronze badges

Sign up to request clarification or add additional context in comments.

9 Comments

Sergio Tulentsev Over a year ago

Not exactly DRY, is it? :) but yeah, have a +1

Aleksei Matiushkin Over a year ago

@SergioTulentsev it is DRY, why? Besides obj[s] && obj[s]['pkg'] there is no one repetition. It’s not a sample of good code, neither a robust production-quality snippet. But it is DRYed.

Sergio Tulentsev Over a year ago

If you want to add another element, you'll have to type it twice.

Sergio Tulentsev Over a year ago

I mean DRY as in "every piece of knowledge should have one source of truth. If it changes, you have only one place to change"

Aleksei Matiushkin Over a year ago

@SergioTulentsev you are welcome, please see an update.

|

Eric Duminil · Accepted Answer · 2016-12-15 11:08:35Z

2

As Stefan mentioned in a comment, defining local variables in findLogs is pointless if you don't use them. Even if you define them twice ;).

It's not clear from your code. If you want to define instance variables by writing Ruby code inside a String, and using eval afterwards : please don't!

obj = {
  'E'  => nil,
  'T'  => { 'pkg' => 't_pkg' },
  'L'  => { 'not_pkg' => 'x' },
  'Te' => { 'pkg' => 'te_pkg' }
}

%w(E T L Te Le).each do |var_name|
  instance_variable_set("@#{var_name}", obj.dig(var_name, 'pkg'))
end

p [@E, @T, @L, @Te, @Le] # => [nil, "t_pkg", nil, "te_pkg", nil]

answered Dec 15, 2016 at 11:08

Eric Duminil

54.6k10 gold badges80 silver badges134 bronze badges

Comments

Aldo 'xoen' Giambelluca · Accepted Answer · 2016-12-15 11:19:48Z

this looks more like a JS function than Ruby :)

Few things to notice:

The variables are not used so you can rid of them
There are a lot of else nil which can be removed as a Ruby method would return nil by default
You're using a ternary operator just to return nil, again, you can avoid it
This method is doing too many things: It's checking what kind of log it is and also display/formatting it
Style note: In Ruby we use snake_case, so the method should be called find_logs

You can avoid the repetition as already said and replace it with something like:

def find_logs(obj)
  log_type = ["E", "T", "L", "Te", "Le"].detect do |type|
    obj[type] && obj[type]["pkg"]
  end

  "@#{log_type} = #{obj[log_type]['pkg']}," if log_type
end

Now, this is not particularly pretty and it's still doing two different things but it may be good enough, and it removed the duplication.

I don't know the problem you're trying to solve but as we're in Ruby it would probably better to solve it with a class, e.g. something like this (not tested so it may not work):

class LogFormatter
  LOG_TYPES = ["E", "T", "L", "Te", "Le"]

  def initialize(obj:)
    @obj = obj
  end

  def format
    "@#{log_type} = #{pkg}," if log_type
  end

  private

  attr_reader :obj

  def pkg
    @pkg ||= obj[log_type]["pkg"]
  end

  def log_type
    @log_type ||= LOG_TYPES.detect { |type| obj[type] && obj[type]["pkg"] }
  end

end

Now, bear in mind that depending what you're trying to do, it may be a bit overkill - but personally, I prefer to have a small class rather than having this "complicated" method in some other class with a completely unrelated responsibility.

Collectives™ on Stack Overflow

DRY for Ruby variable loops

3 Answers 3

9 Comments

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

9 Comments

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related