Krdytkyu

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.

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.