views:

147

answers:

4

Let's consider python (3.x) scripts:

main.py:

from test.team import team
from test.user import user

if __name__ == '__main__':
    u = user()
    t = team()
    u.setTeam(t)
    t.setLeader(u)

test/user.py:

from test.team import team

class user:
    def setTeam(self, t):
        if issubclass(t, team.__class__):
            self.team = t

test/team.py:

from test.user import user

class team:
    def setLeader(self, u):
        if issubclass(u, user.__class__):
            self.leader = u

Now, of course, i've got circular import and splendid ImportError.

So, not being pythonista, I have three questions. First of all:

i. How can I make this thing work ?

And, knowing that someone will inevitably say "Circular imports always indicate a design problem", the second question comes:

ii. Why is this design bad?

And the finally, third one:

iii. What would be better alternative?

To be precise, type checking as above is only an example, there is also a index layer based on class, which permits ie. find all users being members of one team (user class has many subclasses, so index is doubled, for users in general and for each specific subclass) or all teams having given user as a member

Edit:

I hope that more detailed example will clarify what i try to achieve. Files omitted for readibility (but having one 300kb source file scares me somehow, so please assume that every class is in different file)

# ENTITY

class Entity:
    _id    = None
    _defs  = {}
    _data  = None

    def __init__(self, **kwargs):
        self._id   = uuid.uuid4() # for example. or randint(). or x+1.
        self._data = {}.update(kwargs)

    def __settattr__(self, name, value):
        if name in self._defs:
            if issubclass(value.__class__, self._defs[name]):
                self._data[name] = value

                # more stuff goes here, specially indexing dependencies, so we can 
                # do Index(some_class, name_of_property, some.object) to find all   
                # objects of some_class or its children where
                # given property == some.object

            else:
                raise Exception('Some misleading message')
        else:
            self.__dict__[name] = value    

    def __gettattr__(self, name):
        return self._data[name]

# USERS 

class User(Entity):
    _defs  = {'team':Team}

class DPLUser(User):
    _defs  = {'team':DPLTeam}

class PythonUser(DPLUser)
    pass

class PerlUser(DPLUser)
    pass

class FunctionalUser(User):
    _defs  = {'team':FunctionalTeam}

class HaskellUser(FunctionalUser)
    pass

class ErlangUser(FunctionalUser)
    pass

# TEAMS

class Team(Entity):
    _defs  = {'leader':User}

class DPLTeam(Team):
    _defs  = {'leader':DPLUser}

class FunctionalTeam(Team):
    _defs  = {'leader':FunctionalUser}

and now some usage:

t1 = FunctionalTeam()
t2 = DLPTeam()
t3 = Team()

u1 = HaskellUser()
u2 = PythonUser()

t1.leader = u1 # ok
t2.leader = u2 # ok
t1.leader = u2 # not ok, exception
t3.leader = u2 # ok

# now , index

print(Index(FunctionalTeam, 'leader', u2)) # -> [t2]
print(Index(Team, 'leader', u2)) # -> [t2,t3]

So, it works great (implementation details ommitted, but there is nothing complicated) besides of this unholy circular import thing.

A: 

i. To make it work, you can use a deferred import. One way would be to leave user.py alone and change team.py to:

class team:
    def setLeader(self, u):
        from test.user import user
        if issubclass(u, user.__class__):
            self.leader = u

iii. For an alternative, why not put the team and user classes in the same file?

ma3
ad. iii - I have 60+ such classes and putting them into one file is not really the option
ts
ad i. - isn't that hitting performance? anyone knows if python internally optimize that kind of imports?
ts
yes, it's optimized. see http://docs.python.org/library/sys.html#sys.modules
ma3
A: 

tets/team.py

from test.user import User

class Team:
    def __init__(self, leader=None):
        self._leader = leader 
        self._team_members = list()

    @property
    def leader(self):
        return self._leader

    @leader.setter
    def leader(self, user):
        if issubclass(u, User.__class__):
            self._leader = user

    def addmember(self, user_to_add):
        user.team = self
        self._team_members.append(user_to_add)

test/user.py:

class User:
    def __init__(self, team=None):
        self._team = None
    @property
    def team(self):
        return self._team 
    @team.setter 
    def team(self, user):
        self._team = user

main.py

from test.team import Team
from test.user import User

if __name__ == '__main__':
    user_1 = User()
    team_A = Team()
    team_A.addmember(user_1)
    print user_1.team
singularity
Your User class doesn't contain the issubclass check from the original question that leads to the circular import problem. What if someone wrote code like `user = User(); user.team=3`?
ma3
@ma3204: don't have to , because first of all i just call it from Team class and second of all "we are all adult here" , actually i don't see the use of doing each time isinstance , and ts said that the isinstance is just an example and for the others example that he said that he want to do , this desing is more suitable for him, by the way i forget to answer "Why is this design bad?" because it's not working :)
singularity
type checking is only example but it has a place indeed. And problem with team setting self as user._team, is that I am using _ _ setattr _ _ heavily (instead of properties), and all logic (type checking also) is done inside of _ _ setattr _ _
ts
@ts : like everyone have been telling you , you shouldn't put a lot of type cheking , think more python (more dynamic), and about using _ _setattr_ _ instead of propriety so what ? propriety is just an example you can do what ever you want , and if you want sincerely control the type of data , just as my example __"put what you want to control inside a controllable environmental"__ like in addmember() i set the user.team = self in it like that i know for sure that this will be a team instance, right ??
singularity
i added a little bit of code to my original post. I hope that will be more understandable
ts
@ts : so if i understand well ; in the class FunctionalTeam ; it said that __the FunctionalTeam doesn't accept any leader if it's not a FunctionalUser__ yes ? and i guess you have also __a user can be assign to only one Team__ so from the class FunctionalTeam i can also say that __FunctionalUser can be assign to only FunctionalTeam__ ; so can you tell me why you are repeating it in FunctionalUser ??? if you remove the _defs from the Users class is it a problem ? because i guess from you example that you can do it.
singularity
+2  A: 

Bad practice/smelly are the following things:

  • Probaly unnecessary type checking (see also here). Just use the objects you get as it were a user/team and raise an exception (or in most cases, one is raised without needing additional code) when it breaks. Leave this away, and you circular imports go away (at least for now). As long as the objects you get behave like a user / a team, they could be anything. (Duck Typing)
  • lower case classes (this is more or less a matter of taste, but the general accepted standard (PEP 8) does it differently
  • setter where not necessary: you just could say: my_team.leader=user_b and user_b.team=my_team
  • problems with data consistency: what if (my_team.leader.team!=my_team)?
knitti
In reality getters and setters were much simplified.Normally they take more responsability, i.e. checking data consistency (i am not posting whole code here). Lower case classes - ok, I am just reading PEP8 ;). Last but not least - type checking. The problem is that objects which are set here, are propagated and their use is often deferred. So, if the object has wrong type i should backtrace all propagation which can be quite impossible. Instead of that I am validating object before (yes, violating EAFP), and validation is based on object's class rather than properties. Any sane solution here?
ts
You could just add a member to your instances saying what class it is: Then your test would look like : `if not u.is_a == "user": ` which would then just be a sanity check using duck-typing.
Michael Anderson
@Michael: it looks that i have to deal with it in that way, although this doubles class inheritance structure
ts
http://stackoverflow.com/questions/510972/getting-the-class-name-of-an-instance-in-python has a slightly neater solution `u.__class__.__name__ == "user"`
Michael Anderson
+6  A: 

Circular imports are not inherently a bad thing. It's natural for the team code to rely on user whilst the user does something with team.

The worse practice here is from module import member. The team module is trying to get the user class at import-time, and the user module is trying to get the team class. But the team class doesn't exist yet because you're still at the first line of team.py when user.py is run.

Instead, import only modules. This results in clearer namespacing, makes later monkey-patching possible, and solves the import problem. Because you're only importing the module at import-time, you don't care than the class inside it isn't defined yet. By the time you get around to using the class, it will be.

So, test/users.py:

import test.teams

class User:
    def setTeam(self, t):
        if isinstance(t, test.teams.Team):
            self.team = t

test/teams.py:

import test.users

class Team:
    def setLeader(self, u):
        if isinstance(u, test.users.User):
            self.leader = u

from test import teams and then teams.Team is also OK, if you want to write test less. That's still importing a module, not a module member.

Also, if Team and User are relatively simple, put them in the same module. You don't need to follow the Java one-class-per-file idiom. The isinstance testing and set methods also scream unpythonic-Java-wart to me; depending on what you're doing you may very well be better off using a plain, non-type-checked @property.

bobince