Knapsack class in Ruby

Question

This is a simple implementation of a knapsack written in Ruby. I've included all the relevant code here, but my question is concerning the contents setter .contents=

## item.rb
class Item
  attr_accessor :weight
  attr_accessor :data

  def initialize(weight:, data: nil)
    @weight = weight
    @data   = data
  end

end




 ## knapsack.rb
class KnapsackCapacityExceededError < StandardError; end
class KnapsackWeightExceededError < StandardError; end
class KnapsackContentError < StandardError; end

require_relative('./item')

class Knapsack
  attr_reader :capacity
  attr_reader :weight
  attr_reader :contents

  def initialize(capacity:, weight:)
    @capacity = capacity
    @weight   = weight
    @contents = Array.new(@capacity) { nil }
  end

  def contents=(new_contents)
    raise KnapsackCapacityExceededError if self.exceeds_capacity? new_contents
    raise KnapsackWeightExceededError   if self.exceeds_weight? new_contents
    raise KnapsackContentError          unless new_contents.all? { |e| e.is_a? Item }

    @contents = new_contents
  end

  def fit?(contents)
    return false if self.exceeds_weight?(contents) || self.exceeds_capacity?(contents)
    true
  end
  alias_method :fits?, :fit?

  def fits_weight?(contents)
    new_weight = contents.map { |item| item.weight }.sum
    return true if new_weight <= self.weight
    false
  end

  def exceeds_weight?(contents)
    return true if !fits_weight? contents
    false
  end

  def fits_capacity?(contents)
    return true if contents.length <= self.capacity
    false
  end

  def exceeds_capacity?(contents)
    return true if !fits_capacity? contents
    false
  end

end

# k = Knapsack.new(capacity: 10, weight: 50)

In this method there are three conditions where an exception is raised, is this a code smell?

Any other feedback also appreciated.

Answer 1

So there are multiple things. First of all I'll share the code refactoring related suggestions and then we can look at the performance improvements of code.

Refactoring

1. You don't really need to pass a block to Array.new to initialize the array with nil values.

   def initialize(capacity:, weight:)
     @capacity = capacity
     @weight   = weight
     @contents = Array.new(@capacity) { nil }
   end
   

   can be refactored to:

   def initialize(capacity:, weight:)
     @capacity = capacity
     @weight   = weight
     @contents = Array.new(@capacity)
   end
   

2. def contents=(new_contents)
     raise KnapsackCapacityExceededError if self.exceeds_capacity? new_contents
     raise KnapsackWeightExceededError   if self.exceeds_weight? new_contents
     raise KnapsackContentError          unless new_contents.all? { |e| e.is_a? Item }
   
     @contents = new_contents
   end
   

   can be refactored to:

   def contents=(new_contents)
     raise KnapsackCapacityExceededError if exceeds_capacity? new_contents
     raise KnapsackWeightExceededError   if exceeds_weight? new_contents
     raise KnapsackContentError          if has_non_items? new_contents
   
     @contents = new_contents
   end
   
   def has_non_items?(contents)
     contents.any? { |content| !content.is_a?(Item) }
   end
   

   -> Notice that usage of self keyword has been removed.

   -> Moved the logic of checking any non-items in contents to a separate method. That makes the if conditions more readable and it replaces the unless with if to make it consistent with rest of the conditions.

3. Method fit? can be refactored to:

   def fit?(contents)
     fits_weight?(contents) && fits_capacity?(contents)
   end
   

4. Method fits_weight? can be refactored as well.

   def fits_weight?(contents)
     contents.map(&:weight).sum <= weight
   end
   

Performance Tuning

You don't need to initialize the @contents array with nil elements. You can just write @contents = [] to save some extra used memory.

Answer 2

First of all return true if ... else false is not necessary. Just return the original condition. For example:

  def fit?(contents)
    fits_weight?(contents) && fits_capacity?(contents)
  end

---

Next, self. is not necessary in most cases, including every case it is used in this program. self.weight should be replaced with @weight.

---

knapsack.rb should look more like:

class KnapsackCapacityExceededError < StandardError; end
class KnapsackWeightExceededError < StandardError; end
class KnapsackContentError < StandardError; end

require_relative('./item')

class Knapsack
  attr_reader :capacity
  attr_reader :weight
  attr_reader :contents

  def initialize(capacity:, weight:)
    @capacity = capacity
    @weight   = weight
    @contents = Array.new(@capacity) { nil }
  end

  def contents=(new_contents)
    raise KnapsackCapacityExceededError if exceeds_capacity? new_contents
    raise KnapsackWeightExceededError   if exceeds_weight? new_contents
    raise KnapsackContentError          if new_contents.any? { |e| !e.is_a? Item }

    @contents = new_contents
  end

  def fit?(contents)
    fits_weight?(contents) && fits_capacity?(contents)
  end
  alias_method :fits?, :fit?

  def fits_weight?(contents)
    new_weight = contents.map { |item| item.weight }.sum
    new_weight <= @weight
  end

  def exceeds_weight?(contents)
    !fits_weight? contents
  end

  def fits_capacity?(contents)
    contents.length <= @capacity
  end

  def exceeds_capacity?(contents)
    !fits_capacity? contents
  end

end

---

I do not think that three raise conditions are a problem. That code is clear and simple. The three exception classes may be a bit much.