Adding push to array inside the class that is being pushed

I used to mostly do this:

class Person
  constructor : (@parent, @data)->
    @index = @parent.length

class People
  constructor : ->
    people = []

    for data in database.people
      people.push new Person(people, data)

Lately I have been trying the following:

class Person
  constructor : (@data)->

  pushTo : (list)->
    this.index = list.length
    list.push this

class People
  constructor : ->
    people = []

    for data in database.people
      person = new Person(data)
      person.pushTo people

So was wondering if there is any cons about doing it the second way. The reason I am asking is that it might not be the responsibility of Person to add himself to the list.

The reason I am asking is that it might not be the responsibility of Person to add himself to the list.

I think you should ask youself a question "Why am I doing this?" Why do you need to keep the @index inside a Person?

-
Is it just an id? If so why don't you use _.uniqueId('person') from Underscore JS? That would be much cleaner.

-
Do you use it for ordering so you could for example sort an array of people by it? If so uniqueId would still suit your needs well.

-
Do you use it to search for person by it's index in the list later on? Basically I can envision two kinds of data processing.

-
You iterate over a list of people. In this case you'll be fine with Underscore's each method:

_.each people, (person, index) ->
    // your code here     ^----- see that? ;)

-
You got a person data from some external source and you want to reuse the associated Person object. In this case you should still use an ID and search for your object with find or select methods.

Some notes:

-
Both your solutions are totally fine if you certain that they don't complicate the code of your application. They also apply in cases when your collection is very large and find takes too much time. BUT you can speed up selection on later invocations:

indexFor = (targetPerson) ->
  unless person.index // we search for index once and then store it for later use
    _.find people, (person, index) ->
      person.index = index if found = (person.id == targetPerson.id)
      found
  targetPerson.index

-
uniqueId uses internal counter so the ID is always increasing. It cannot be reset which may not be always convenient. For example if you need to save your collection and reload it later you won't be able to tell the generator to increase the counter accordingly. In that case I would still go with uniqueId for presentation code but use some other ID generation scheme for persistence. I would go with UUIDs: but you can also try timestamps though I wouldn't recommend it since generally time is way too difficult to get right.

-
You should definitely check out other methods in Underscore library. Whenever you encounter a kind of low-level task you should ask if Underscore does it for you already. If it doesn't check jQuery or whatever library you use for UI. If you're still out of luck ask Google or StackOverflow. I'm sure you're great coder and you can solve many of those tasks yourself but why should you spend your time and effort reinventing stuff instead of creating something new? Know your libraries and use them. It's like a new language or a new text editor - at first it's painful but it pays off later on.

StackExchange Code Review Q#4776, answer score: 2