From: Douglas Seifert <doug@...>
Date: 2009-09-19T09:48:39+09:00
Subject: Re: Getting to one attribute of my marshalled array of objects

--000e0cd5c7c81cb3440473e38895
Content-Type: text/plain; charset=ISO-8859-1

I won't comment on your design as it seems straightforward enough.  It could
use some rethinking, but with time and experience you will come up with
better designs.  If you want some pointers let me know ...

Some issues with the implementation though, my comments below ...


> class Car
>
>  attr_reader :vin, :year, :make, :model, :color, :price
>  attr_writer :vin, :year, :make, :model, :color, :price
>
> The above can be shorted to one line:

   attr_accessor :vin, :year, :make, :model, :color, :price


> end
>
> class CarInventory
>
>  @@inventoryFile = "cars.data"
>
>  def initialize
>    if File.exists?(@@inventoryFile)
>      File.open(@@inventoryFile) do |file|
>        @carArray = Marshal.load(file)
>      end
>    else
>        @carArray = Array.new
>        @carArray[0] = Car.new(nil, nil, nil, nil, nil, nil)
>    end
>  end
>
>  def save
>    File.open(@@inventoryFile, "w+") do |file|
>      Marshal.dump(self, file)
>

Here you are dumping the CarInventory (self) object to the file.  What you
really want to do is dump the @carArray to the file.  This is because above
in the initialize method you are trying to read the @carArray from the
file.  This is the source of the undefined method `[]' error you are getting
when you run the program after saving.


>    end
>  end
>
>  def addCar(car)
>    if car.vin == nil
>      @carArray[0] = car
>    else
>      @carArray << car
>    end
>
Not sure what you are trying to do with this implementation.  I would change
it to simply

@carArray << car


>  end
>
>  def updateCar(car)
>    updateIndex = @carArray.find {|vinMatch| car.vin == vinMatch}
>    if updateIndex
>      @carArray[updateIndex]  = car
>    end
>
This will not have the effect you want.  I think you will have to define ==
on the Car class and implement it use vin as the equality check:

class Car
  ...
  def ==(other)
     self.vin == other.vin
  end
  ...
end

Then, the updateCar method of the CarInventory class could be something
like:

def updateCar(car)
   i = @carArray.index(car)
   if i
     @carArray[i] = car
   else
      # TODO: Print an error message? Raise an exception?
   end
 end

 end
>
>  def delCar(car)
>    @carArray = @carArray - car
>

Here, you need
@carArray = @carArray - [car]

and it will only work if you define Car#== as above.  The Array#- method
only works on two Array objects.  You can't use it to remove an object.
Alternatively, you could use Array#delete:

def delCar(car)
  @carArray.delete(car)
end


>
> puts "Car Inventory 1.0\n"
>
> cars = CarInventory.new
> firstCar = cars.carArray
> firstCar = firstCar[0]
>

I would change the above two lines to simply
firstCar = cars.carArray.first


> if !firstCar.vin
>

Then instead of looking for a nil vin, just check if firstCar itself is nil:

  if !firstCar


>  puts "\nNo car inventory found, please enter the first car:"
>  puts "\nVIN:"
>  vin = gets.chomp
>  puts "\nYear:"
>  year = gets.chomp
>  puts "\nMake:"
>  make = gets.chomp
>  puts "\nModel:"
>  model = gets.chomp
>  puts "\nColor:"
>  color = gets.chomp
>  puts "\nPrice:"
>  price = gets.chomp
>  newCar = Car.new(vin, year, make, model, color, price)
>  cars.addCar(newCar)
> else
>  cars.sort("V")
>  cars.filter(nil)
> end
>

But really, I would just get rid of the entire if/else statment above and go
directly into the loop below it.  The initial logic is not DRY and doesn't
add anything the loop doesn't already give you.

Hope that helps,
Doug Seifert

--000e0cd5c7c81cb3440473e38895--