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.

• kingsfoil: All good points. Thanks for your feedback.
• Kashif Umair Liaqat: @Graipher thanks for the changes. They look good to me

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.

• David Aldridge: Bit off topic, but maybe replace contents.map { |item| item.weight }.sum with contents.sum(&:weight). I'd be tempted to replace new_contents.any? { |e| !e.is_a? Item } with new_contents.all? { |e| e.is_a? Item } also.