- I kind of ignore line length as you might notice
modules/project/models.py
- space after every comma (see lines 11, 55, 64)
- Priority_Choices should be priority_choices (line 50), same with Status_Choices -> status_choices
- beginning spelt wrong (beggining_date), make sure you correct it before making a migration
- related_name on line 68,83,84, should be named like REVERSE of field
- Instead of task_handler = ForeignKey(Agent, related_name='task_handler'), it should be ForeignKey(Agent, related_name='tasks_that_this_agent_is_the_handler_for')
- E.g. if you have Person item type, you'd have mother = ForeignKey(Person, related_name="children"). Then when you do sally.children.filter(age__lt=10) you get a query set of sally's children that are under 10 years old. Otherwise you'd have to do Person.objects.filter(mother=sally, age__lt=10)
- On the foreign keys, try not to have null=True/blank=True.
- dependent_task = FixedForeignKey(Task, related_name='dependent_task', null=True, blank=True, default=None) becomes dependent_task = FixedForeignKey(Task, related_name='dependent_task')
- If you do want to allow null, don't need default=None
- Line 57, decimal_places = 2 should be decimal_places=2 (or at least keep it consistent)
- Missing introduced_abilities on Project. (edit and view due_date)
- You should make TaskDependency fields immutable (introduced_immutable_fields, see Membership for an example)
- You do have dyadic_relations, these are for "many to many" item types. See Membership in cms/models.py on line 1426, the dyadic_relations. You'll see this used in the Related Items pane of the side bar.
- Don't have excessive newlines. Keep all of the fields together with no newlines, status_choices and priority_choices can be in a separate block.
modules/project/views.py
- Don't have excessive newlines, within a method only have single newlines, between classes maybe two. Try and keep programmer from having to scroll, so they can absorb as much information in short period of time.
- I don't think you need a task dependency viewer. Let users use the default item viewer for this, most of the time they'll just be interacting with the ProjectViewer. You may need the viewer with the top two lines, but you don't need the show method
- Lines 23 and 24 can be combined: memberships.filter(active=True, item__active=True)
- We'll come back to permissions later
- I don't think you need line30 with cur_agent_in_collection
- Lines 44 and 45, you don't need the .pk
- I wouldn't name the lists `is_dependent` and `is_required` since that sounds like a boolean. Name it something like "tasks_we_depend_on"
- dependencies = TaskDependency.objects.all()
- self.context['is_dependent'] = dependencies.filter(dependent_task=self.item.pk)
- self.context['is_required'] = dependencies.filter(required_task=self.item.pk)
- self.context['is_dependent'] = TaskDependency.objects.filter(dependent_task=self.item.pk)
- self.context['is_required'] = TaskDependency.objects.filter(required_task=self.item.pk)
modules/templates/project/show.html
- In general, you should have spaces around {{ variable }} as opposed to {{variable}}
modules/templates/project/task_dependency.html
modules/templates/project/task.html
Project Tool Code Review Notes 7/14/2011