Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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.

share|improve this question

3 Answers

up vote 2 down vote accepted

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.

    1. 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? ;)

    2. 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:

  1. 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

  2. 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.

  3. 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.

share|improve this answer
up vote 1 down vote

Regarding @Andrew's answer: Best Coffeescript practice would be to use is instead of ==. I.e., person.id is targetPerson.id. Coffeescript will substitute === in place of either AFAIK.

Regarding @Pickels' question: List comprehension is a great tool here. Consider this:

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

This compiles to:

var data, people;

people = (function() {
  var _i, _len, _ref, _results;
  _ref = database.people;
  _results = [];
  for (_i = 0, _len = _ref.length; _i < _len; _i++) {
    data = _ref[_i];
    _results.push(new Person(people, data));
  }
  return _results;
})();

which is, I believe, what you want.

A great new reference is the Coffeescript Style Guide.

share|improve this answer
up vote 0 down vote

Here's a pattern I use all the time; for model/collections as in your example, or even for managing views and arbitrary objects. This is also how Spine.JS models work.

You can declare static methods and properties on the class itself with @method. So calling Person.Fetch() here will instantiate new Person instances with the appropriate id (Array#push returns the new length) and add them to the collection array, a private static variable.

So to answer the original question, I do think it is appropriate for instances to add themselves to the collection. What if a Person was created client-side by the user rather than through a call to the database? Shouldn't it also be a part of the collection, say for syncing back to the server? The cool thing here is that it's encapsulated within the class definition, rather than establishing a dependency with another class.

class Person
  collection = []

  @Fetch: -> new Person data for data in database
  @Get: (id) -> if id? then collection[id] else collection

  constructor: (@data) -> @id = collection.push this

database = ["Barry", "Other Barry", "Sterling"]
Person.Fetch()
alert Person.Get(1).data     # alerts "Other Barry"
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.