Tukukkk

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.

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.

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.