Better way to refer to common elements in a sequence of objects?

I just posted this code in SO and one person said my code sucked, and that I should post it here. So here it is (with fix I asked for included, so you don't have to worry about that).

The main concerns that I have with it myself is how I iterate through a sequence of objects that the object itself is in. Apparently id is bad, because it's like id(). Someone also mentioned that I shouldn't make members of a collection overly aware of their own collection.

import random

    class Car:
        def __init__ (self, company, doors, id):
            self.company = company
            self.doors = doors
            self.id = id

        def printDoors(self, id):
            print 'Car ' + `self.id` + ' has ' + `self.doors` + ' doors.'

        def findSameDoors(self, id):
            for i in self.company.cars:
                if self.id != i.id and self.doors == i.doors:
                    print 'Car ' + `i.id` + ' does too!'

    class Company:
        def __init__ (self, types):
            self.types = types
            self.cars = []

        def typesToNum(self):
            result = []
            for i in self.types:
                if i == 'sedan':
                    result.append(4)
                elif i == 'convertible':
                    result.append(2)
                else:
                    result.append(0)
            return result

    porsche = Company(['sedan', 'convertible'])
    honda = Company(['sedan', 'convertible', 'motorcycle'])

    for i in range(10):
        porsche.cars.append(Car(porsche, random.choice(porsche.typesToNum()), i))

    for i in range(10):
        honda.cars.append(Car(honda, random.choice(honda.typesToNum()), i))

    porsche.cars[0].printDoors(0)
    porsche.cars[0].findSameDoors(0)

Thanks for any pointers.

It is, as you say, slightly odd that a method in Car iterates over the container that Car is in. It really ought to be in Company, and take an argument for how many doors to match:

class Company:
    ...
    def searchByDoors(self, doors):
    ''' Get an iterator of all the cars in self that have the 
        specified number of doors
    '''

    for car in self.cars:
         if car.doors == doors:
             yield car

This also brings up another point: you probably want to avoid printing from inside your class (except for debugging). Even if whatever you're writing now wants it printed, you should write your classes so they can be used in any program . So, a search function should give the matches back whichever part of your program called it, and it can print them if it wants to. Similarly, Car.printDoors can go - your program can get that information and print it. If there is a canonical way that you want represent a Car for human consumption, define it this way:

class Car:
     ...
     def __str__(self):
         return "A {} {} with {} doors".format(self.company, self.type, self.doors)

And your program can just print(a_car) to print something like A porche sedan with 4 doors.

Note that, notwithstanding what the person on SO said about not making objects aware of their containers, it probably does make sense for a Car to know which Company made it.

There are two issues with using id as an attribute name:

• The local variable in the constructor shadows the built-in id() function,

• More importantly, car.id is different to id(car), even though they're named similarly and have similar purposes.

Consider whether id(car) is good enough for what you would use car.id for - it identifies a unique python object, and is called for you when you do things like a is b. If id needs to reflect something external to your program, consider renaming it. car_id might work, but if it represents something concrete in your problem domain (eg, an official registration number), naming it after that is even better.

There is also one issue with the way you're generating ids, that using Python's id() avoids: your ids are only unique within a company (and even that isn't guaranteed), but you could reasonably expect it to be unique across all cars. So, overall, I would drop id entirely.

typesToNum doesn't belong in Company, and this is really begging for a dictionary rather than a method. I'd make it a module-level private constant:

# Mapping of car type (eg, sedan) to number of doors 
 _DOORS_PER_TYPE = {'sedan': 4, 'convertible': 2, 'motorcycle': 0}

A car should really know what type of car it is, and with this mapping it would make more sense to take this at construction than the number of doors:

class Car:
      def __init__(self, company, type):
            assert type in company.types
            self.type = type

      @property
      def doors(self):
          return _DOORS_PER_TYPE[self.type]

The assert makes sure that the type of car is one that that company does make. It is optional, its just a sanity check in case something impossible happens. Its a good idea, because that is more common than it sounds. Likewise, you can do:

class Company:
       def __init__(self, types):

The constraint that a convertible always has 2 doors, and a sedan always has 4 is a little odd. If it might make sense in your program to have a 2 or 4 door sedan, but not, say, a single-door sedan, you could do this:

_DOORS_PER_TYPE = {'sedan': (2, 4)}

 class Car:
      def __init__(self, company, type, doors):
          assert type in company.types
          assert doors in _DOORS_PER_TYPE[type]

          self.company = company
          self.type = type
          self.doors = doors

This also makes doors an attribute rather than a property since its no longer linked 1-to-1 with type, but the rest of your program doesn't need to care about that difference.

You can also change the valid car types and number of doors to vary between each company, but only if that makes sense for your model. If the number of doors is always 1-to-1 with the type, and that is always going to be independent of the company who built it, then it is better to have your code say so.

StackExchange Code Review Q#8871, answer score: 2