[Tutor] please critique my OOP

[Roeland Rengelink]

Hi Timothy,

I'll give it a shot. 

There are a lot of points, but I think your basic design is solid, and
you have a working program. You should consider any improvements I
suggest here as minor tweaks.

Timothy Wilson wrote:
> 
> Hey everyone,
> 
> I've been doing some reading on OOA, OOD, and OOP lately and I've gone back
> to the random student group assignment program that I wrote a while back. I
> reworked it a bit and I think it's quite a bit clearer. I'd appreciate it if
> some of the more experienced persons on this list would give it a look and
> offer critiques. I'm especially interested in OOP issues here.
> 
> My previous effort had all of the group assignment logic in one of the
> Teacher class's methods. I've removed that in this version and it's quite a
> bit simpler as a result. Have I sinned? :-)
> 

Some general remarks:

The assignment logic is now in main(), I don't think it belongs there.
My first guess is it does indeed belong in Teacher.

Teacher addStudent takes fn,ln,gender as argument, I think it should
take a student as argument.

Teacher has an addStudent method, but not an addGroup method

The loadFromFile looks awkward. I would probably make this a separate
function returning
a list of student. I would certainly create the Student instances within
this function

de loadFromFile(filename):
   result = []
   for line in open(filename).readlines():
	fn, ln, gender = line.split(',')
        # the strip()s take care of all whitespace
        student = Student(fn.strip(), ln.strip(), gender.strip())
	result.append(student)
   return result

A wild idea. Shouldn't there be Class class. A class (in the schoool
sense) HAS_A teacher and HAS_A list of students, and HAS_A list of
groups. Also: consider deriving group from UserList

Some additional comments in the code

> Once I feel like this is a reasonably good example of OOP I promise to write
> up a How-To describing the thought processes that led me do it this way. I
> know this is a very simple program, but as an OOP beginner, I haven't found
> a lot of examples of simple OO programs that describe how to get started.
> 

It's what I hate about all the OOA and OOD books, they go to great
lengths explaining how the OO analysis and design will really help
making programming easier, but then they rarely bother to actually code
up complete examples that show this.

> Here's the code and a sample run:
> 
> --snip--
> #!/usr/bin/env python
> 
> ################################################################
> #                                                              #
> # mkgrp.py by Tim Wilson <wilson@visi.com>                     #
> #   Assign a class of students randomly to a series of groups. #
> #
> #                                                              #
> # July, 2001                                                   #
> #                                                              #
> ################################################################
> 
> import string, random
> 
> class Teacher:
>     def __init__(self, name):
>         self.name = name
>         self.roster = []
>         self.unassigned = []

self.unassigned is part of the assignment logic. In your design it
doesn't belong here.

>         self.groupList = []
> 
>     def __str__(self):
>         return "%s has %s students." % (self.name, len(self.roster))
> 
>     def addStudent(self, ln, fn, gender):
>         newStudent = Student(ln, fn, gender)
>         self.roster.append(newStudent)
>         self.unassigned.append(newStudent)
> 
>     def loadFromFile(self, filename):
>         """
>         File format has one student per line in the form:
> 
>         lastname, firstname, gender
> 
>         For example:
> 
>         van Rossum,Guido,M
>         """
> 
>         file = open(filename, 'r')
>         studentList = file.readlines()
>         for i in studentList:
>             studentRecord = string.split(i, ',')
>             self.addStudent(studentRecord[0], \
>             studentRecord[1], studentRecord[2][:-1]) #remove '\012'
> 
>     def printStudentRoster(self):
>         for student in self.roster:
>             print student
> 
>     def printGroupList(self):
>         for group in self.groupList:
>             print group.name
	      group.printGroup()

> 
> 
> class Student:
>     def __init__(self, ln, fn, gender):
>         self.ln = ln
>         self.fn = fn
>         self.gender = gender

Minor nit: why not lastname and firstname, in stead of ln, fn?

> 
>     def __str__(self):
>         return "%s %s" % (self.fn, self.ln)
> 
> class Group:
>     def __init__(self, name):
>         self.name = name
>         self.groupMembers = []
> 
>     def printGroup(self):
>         for student in self.groupMembers:
>             print student
> 
>     def addStudent(self, student):
>         self.groupMembers.append(student)
> 
> if __name__ == '__main__':
>     teacherName = raw_input('Your name? ')
>     teacher = Teacher(teacherName)
>     classList = raw_input('Class file? ')
>     teacher.loadFromFile(classList)
>     print teacher

>     numGroups = raw_input('How many groups? ')
>     for i in range(int(numGroups)):
>         group = Group(i+1)
>         teacher.groupList.append(group)
      teacher.addGroup(group)

>     while len(teacher.unassigned) != 0:
>         for group in teacher.groupList:
>             if len(teacher.unassigned) == 0:
>                 break
>             else:
>                 selectedStudent = random.choice(teacher.unassigned)
>                 group.addStudent(selectedStudent)
>                 teacher.unassigned.remove(selectedStudent)
      You could remove the while loop, the break takes care of empty
unassigned lists.

>     print
>     for group in teacher.groupList:
>         print "Group %s" % group.name
>         print "======" + len(str(group.name))*'='
>         group.printGroup()
>         print
      If this is how you print a Group list why isn't this in
Teacher.prinGroupList

Hope this helps,

Roeland
-- 
r.b.rigilink@chello.nl

"Half of what I say is nonsense. Unfortunately I don't know which half"