1

I am creating a project where I have scraped data from a webpage for product info. My scraping method returns a nested array with a collection of product_names, urls, and prices. I'm trying to use the nested array to create instances of my class Supplies, with attributes of a name, url, and price. The nested array has the same number of elements in each of the 3 array.

I want to @@all to return an array of instances of all products in the collection with their attributes set.

name_url_price = [[],[],[]]

class Catalog::Supplies
  attr_accessor :name, :price, :url

  @@all = []

  def initialize(name_url_price)
    count = 0
    while count <= name_url_price[0].length
    self.name = name_url_price[0][count]
    self.url = name_url_price[1][count]
    self.price = name_url_price[2][count]
    @@all << self
    count += 1
  end

end

1 Answer 1

1

There's a lot going wrong here but nothing that can't be fixed:

  1. Storing all the instances in a class variable is a little strange. Creating the instances and having the caller track them would be more common and less confusing.
  2. Your while count <= ... loop isn't terminated with an end.
  3. Your loop condition looks wrong, name_url_price[0] is the first element of name_url_price so name_url_price[0].length will, presumably, always be three. If name_url_price looks more like [ [names], [urls], [prices] ] then the condition is right but that's a strange and confusing way to store your data.
  4. while loops for iteration are very rare in Ruby, you'd normally use name_url_place.each do ... end or something from Enumerable.
  5. Your array indexing is possibly backwards, you want to say name_url_price[count][0], name_url_price[count][1], ... But see (3) if I'm misunderstanding how your data is structured.
  6. Your @@all << self is simply appending the same object (self) to @@all over and over again. @@all will end up with multiple references to the same object and that object's attributes will match the last iteration of the while loop.
  7. The initialize method is meant to initialize a single instance, having it create a bunch of instances is very strange and confusing.

It would be more common and generally understandable for your class to look like this:

class Catalog::Supplies
  attr_accessor :name, :price, :url

  def initialize(name, url, price)
    self.name  = name
    self.url   = url
    self.price = price
  end
end

And then your name_url_price array would look more like this:

name_url_price = [
  [ 'name1', 'url1', 1 ],
  [ 'name2', 'url2', 2 ],
  [ 'name3', 'url3', 3 ],
  [ 'name4', 'url4', 4 ]
]

and to get the supplies as objects, whatever wanted the list would say:

supplies = name_url_price.map { |a| Catalog::Supplies.new(*a) }

You could also use hashes in name_url_price:

name_url_price = [
  { name: 'name1', url: 'url1', price: 1 },
  { name: 'name2', url: 'url2', price: 2 },
  { name: 'name3', url: 'url3', price: 3 },
  { name: 'name4', url: 'url4', price: 4 }
]

and then create your instances like this:

supplies = name_url_price.map do |h|
  Catalog::Supplies.new(
    h[:name],
    h[:url],
    h[:price]
  )
end

or like this:

supplies = name_url_price.map { |h| Catalog::Supplies.new(*h.values_at(:name, :url, :price)) }
Sign up to request clarification or add additional context in comments.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.