views:

1009

answers:

4

Hi,

Is there a more 'DRY' way to do the following in ruby?

#!/usr/bin/env ruby

class Volume
    attr_accessor :name, :size, :type, :owner, :date_created, :date_modified, :iscsi_target, :iscsi_portal

    SYSTEM = 0
    DATA = 1

    def initialize(args={:type => SYSTEM})
      @name = args[:name]
      @size = args[:size]
      @type = args[:type]
      @owner = args[:owner]
      @iscsi_target = args[:iscsi_target]
      @iscsi_portal = args[:iscsi_portal]
    end

    def inspect
      return {:name => @name,
              :size => @size,
              :type => @type,
              :owner => @owner,
              :date_created => @date_created,
              :date_modified => @date_modified,
              :iscsi_target => @iscsi_target,
              :iscsi_portal => @iscsi_portal }
    end

    def to_json
      self.inspect.to_json
    end

end

+8  A: 

Whenever you see a long list of things like that, usually you can roll it all up into a singular Array:

class Volume
  ATTRIBUTES = [
    :name, :size, :type, :owner, :date_created, :date_modified,
    :iscsi_target, :iscsi_portal
  ].freeze

  ATTRIBUTES.each do |attr|
    attr_accessor attr
  end

  SYSTEM = 0
  DATA = 1

  DEFAULTS = {
    :type => SYSTEM
  }.freeze

  def initialize(args = nil)
    # EDIT
    # args = args ? DEFAULTS : DEFAULTS.merge(args) # Original
    args = args ? DEFAULTS.merge(args) : DEFAULTS

    ATTRIBUTES.each do |attr|
      if (args.key?(attr))
        instance_variable_set("@#{attr}", args[attr])
      end
    end
  end

  def inspect
    ATTRIBUTES.inject({ }) do |h, attr|
      h[attr] = instance_variable_get("@#{attr}")
      h
    end
  end

  def to_json
    self.inspect.to_json
  end
end

Manipulating instance variables is pretty straightforward after that.

tadman
Awesome! I had to change one line in the initialize functionargs = args ? DEFAULTS : DEFAULTS.merge(args)becomesargs = (args == nil) ? DEFAULTS : DEFAULTS.merge(args)Otherwise it will return DEFAULTS and won't set anything.
Trevoro
The one downside here is that if I pass a hash with a default value, like `Hash.new(4)` or `Hash.new { |h,k| h[k] = k.to_s }`, it would never get used (and it would in the original code). To get around this, I would drop the args.key? if clause, and use the slightly klunky `args = args ? args.merge(DEFAULTS).merge(args) : DEFAULTS` which will let args override the DEFAULTS but keep its default behaviour.
rampion
Ah, looks like I got my ternary backwards on the args check. That's what I get for posting untested code!Posting a fix to avoid confusion.
tadman
+6  A: 
 class Volume

  FIELDS = %w( name size type owner iscsi_target iscsi_portal date_create date_modified)
  SYSTEM = 0
  DATA = 1
  attr_accessor *FIELDS

  def initialize( args= { :type => SYSTEM } )
    args.each_pair do | key, value |
      self.send("#{key}=", value) if self.respond_to?("#{key}=")
    end
  end

  def inspect
    FIELDS.inject({}) do | hash, field |
      hash.merge( field.to_sym => self.send(field) )
    end.inspect
  end

 end
Subba Rao
-1 inspect doesn't return the attribute values in the hash
rampion
you are right. i fixed it. above code should working now. thanks.
Subba Rao
A: 

Riffing off of tadman's answer

I would have #inspect return a String (like most #inspect methods), and maybe factor out your conversion to a hash method into a #to_hash method instead.

The args.merge(DEFAULTS).merge(args) nonsense lets the args override the DEFAULTS, but keeps any default behaviour for args (say if args == Hash.new(3) or args == Hash.new { |h,k| h[k] = h.to_s.length }

class Volume
  ATTRIBUTES = %w{
    name size type owner date_created date_modified
    iscsi_target iscsi_portal
  }.map! { |s| s.to_sym }.freeze

  attr_accessor *ATTRIBUTES

  SYSTEM = 0
  DATA = 1

  DEFAULTS = { :type => SYSTEM }.freeze

  def initialize(args = nil)
    args = args ? args.merge(DEFAULTS).merge(args) : DEFAULTS

    ATTRIBUTES.each do |attr|
      instance_variable_set("@#{attr}", args[attr])
    end
  end

  def to_hash
    Hash[ *ATTRIBUTES.map { |attr| [ attr, instance_variable_get("@#{attr}") ] }.flatten ]
  end

  def inspect
    to_hash.inspect
  end

  def to_json
    self.to_hash.to_json
  end
end
rampion
+1  A: 

Here's a slightly different spin on the answer by tadman:

class Volume
  ATTRIBUTES = [
    :name, :size, :type, :owner, :date_created, :date_modified,
    :iscsi_target, :iscsi_portal
  ].freeze

  ATTRIBUTES.each do |attr|
    attr_accessor attr
  end

  SYSTEM = 0
  DATA = 1

  DEFAULTS = {
    :type => SYSTEM
  }.freeze

  def initialize(&block)
    ATTRIBUTES.each do |attr|
      self.__send__ "#{attr}=", DEFAULTS[attr]
    end
    yield(self)
  end

  def inspect
    ATTRIBUTES.inject({}) { |h,attr| h[attr] = self.__send__ attr; h }
  end
  def to_json
    self.inspect.to_json
  end
end

This allows to do this:

vol = Volume.new do |v|
  v.name = 'myVolume'
end

I like this because it has the advantage of giving errors right away if someone has made a typo on an attribute.

Also, unlike his answer, it initializes the defaults when they're not supplied.

or if you end up doing it a lot and really need DRY:

module Attributable
  @@ATTRIBUTES = []
  @@DEFAULTS = {}

  def initialize(&block)
    @@ATTRIBUTES.each do |attr|
      self.class.__send__ :attr_accessor, attr
      self.__send__ "#{attr}=", @@DEFAULTS[attr]
    end
    yield(self)
  end
end

class Volume
  include Attributable
  @@ATTRIBUTES = [ :name, :size, :type, :owner ]
  @@DEFAULTS = { :type => 0 }
end

couldn't work out how to do it with constants so I did it with class variables instead.

Daniel