2
\$\begingroup\$

So this isn't EXACTLY my code but it is close enough to show what I have. Also the naming is only for this site so if its not really clear forgive me. Its much clearer in the actual code.

class Member(models.Model):
    team = models.IntegerField(default=0)
    match = models.ForeignKey(Match, null=True, blank=True)

    def engage(self, match_instance, team_number):
        self.match = match_instance
        self.team = team_number
        self.save()

class NPCGeneric(models.Model):
    speed = models.IntegerField(default=1)
    difficulty = models.IntegerField(default=0)

class NPCInstance(Member):
    generic = models.ForiegnKey(NPCGeneric);
    status = models.CharField(max_length=50)

    def engage(self, match_instance, team_number):
        new_instance = NPCInstance(generic=self)
        new_instance.save()       
        return new_instance.engage(match_instance, team_number)

class Player(Member):
    name = models.CharField(max_length=100)


class Match(models.Model):
    event_members = []

    def __init__(self, *args, **kwargs):        
        retval = super(Match, self).__init__(*args, **kwargs)
        if(args[0] and hasattr(args[0], '__iter__')):
            teams = args[0]
            args = args[1:]
            self.__init_new__(teams)
        elif(self.id):
            self.__init_existing__()
        return retval

    def __init_new__(self, teams):
        new_teams = []
        for team in teams:
            new_team = []
            new_teams.append(new_team)
            team_number = new_teams.index(new_team)
            for event_member in team:
                team_member = event_member .engage(self, team_number)
                team_member .save()
                new_team.append(team_member)
        self.event_members = new_teams

    def __init_existing__(self):
        new_teams = []
        def add_list(list, member, team):
            while(list.__len__() < (team + 1)):
                list.append([])
            list[team].append(member)

        for event_member in self.member_set.all():
            try:
                member = event_member.npcinstance
            except:
                member = event_member.player
            add_list(new_teams, member, member.team)
        self.event_members= new_teams

So this is where I start thinking that there has to be a better cleaner way. Unfortunately my brain just keeps getting stuck on what I already have.

I'd love to know if anyone can suggest a cleaner/Pythonic way for holding the Members in the Match class. specifically:

  • The initialising just seems very odd and clumsy. I'm afraid I will introduce errors this way.
  • a better way of representing teams other than a list of lists event_members
\$\endgroup\$
  • \$\begingroup\$ Can you clarify what are you trying to achieve? Such an unusual Match initialization might be "odd and clumsy", but without knowing the intent of the code it's hard to tell if it is so and whether there are alternatives. It would be also useful if you comment the code for clarification (especially the "odd" places). \$\endgroup\$ – Anton Strogonoff Jul 6 '11 at 16:05
  • \$\begingroup\$ @Anton I django they have this member_set but i can't really access it per team. and if a team gets knocked out of the match then there might be a team missing (e.g. teams 1, 2, 4 and 5) might be still in the match but not team 3). The Intent of the code is to hold Members grouped by team in an object called Match. \$\endgroup\$ – James Khoury Jul 9 '11 at 1:10
  • \$\begingroup\$ I still don't get it... You can access members per team through member_set: member_set.filter(team=<some_team_number>). Also, why not add members to matches via Team objects? You'll be able to access members on a match like that: team_set.get(number=<some_team_number>).member_set. And if team gets knocked out, you can just set a flag on Team object (or clear its ForeignKey, which will remove it from match's team_set). \$\endgroup\$ – Anton Strogonoff Jul 9 '11 at 1:34
  • \$\begingroup\$ I had thought about Team objects but I was hoping not to get too complex. i can't access the filtering in the templates either. \$\endgroup\$ – James Khoury Jul 9 '11 at 1:43
  • \$\begingroup\$ I think it's time to move discussion to answer thread. Too long for a comment... \$\endgroup\$ – Anton Strogonoff Jul 9 '11 at 2:10
3
\$\begingroup\$

Sorry, it just seems to me that issues lie outside of the piece of code that you're suggesting to review. The code itself is more or less fine, but some assumptions behind it seem not right.

I had thought about Team objects but I was hoping not to get too complex. i can't access the filtering in the templates either.

Well, you still have teams anyway, you're just passing them around "implicitly", by their numbers. IMO it actually makes it more complex, and is less "pythonic" (remember, explicit is better than implicit).

And, although I don't know the exact requirements, but maybe what you're trying to do in templates could be achieved another way?

Update in response to comment:

Also I want to point out that the code works. It just doesn't "mesh" in my head well.

The code may work, but it looks very… unconventional. And in Python conventions mean a lot. And in Django they mean even more: it's a framework, and, as any other framework, it has lots of design decisions already made for you. It allows you to focus on your data structures and your business logic, but it does that by taking from you some freedom.

So when you're starting to write such complex initialization methods, then probably something is going wrong. I'd say this, and "implicit" teams, are cases when you're beginning to "fight the framework". It's usually the losing battle. The conventional way is to just have a Team class. It will also make code more maintainable, which is very important. (What if the requirements change and you'll need to store more information about the team, not only its number?)

i am also trying to keep the number of different objects in the DB to a minimum but its not THAT important

Seems like premature optimization. I doubt that could be a performance bottleneck. Although it depends on your requirements.

\$\endgroup\$
  • \$\begingroup\$ No I don't need to worry about the team on the template. That's just another piece of data. It means I then have to access the members by match > teams > members instead of just match > members. I do like the link for explicit/implicit. That hadn't occured to me. Also I want to point out that the code works. It just doesn't "mesh" in my head well. I thought i should add i am also trying to keep the number of different objects in the DB to a minimum but its not THAT important \$\endgroup\$ – James Khoury Jul 10 '11 at 23:35
  • \$\begingroup\$ I've updated the answer. \$\endgroup\$ – Anton Strogonoff Jul 11 '11 at 2:30
  • \$\begingroup\$ Thats a great Answer! It wasn't a performance issue so much as an I'm-not-really-a-db-person issue. Thanks for the response and effort!. \$\endgroup\$ – James Khoury Jul 11 '11 at 3:54
  • 1
    \$\begingroup\$ You're welcome! I'm not a DB person either—but Django's ORM abstraction advocates thinking about information in terms of objects / classes instead of tables, so I'm trying to follow that approach where possible and not to worry about how many tables I'll end up with. And I should say that having a good number of tables never caused problems with maintenance for me so far. \$\endgroup\$ – Anton Strogonoff Jul 11 '11 at 4:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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