I have several very similar subclasses that I (think I) want to be stored in the same database table. Most of the fields are identical, with each subclass adding 1 or 2 custom fields. My code is using the Django ORM, but I think the question is more general than that.
Problem specification
The app is meant to simplify the tracking and submission of corporate expenses for reimbursement. There are three primary user interactions:
- Inputting expenses: The user says "I just took a 25ドル cab to the airport for a client visit, billable to XXX client."
- Viewing expenses: The users says "Did I forget to add that dinner from last Tuesday? Let me see if this is the one I'm thinking of..."
- Generating reports: The user says "Compile these 15 expenses into the XML report I need to submit these to finance for reimbursement."
The report itself is pretty standard xml, with one node per expense, with each field given its own node (roughly, <trxs><trx><date>YYYY-MM-DD</date>...</trx><trx>...</trx><trxs>
). The key complication is that depending on the type of expense, there are 1 or 2 additional custom fields per transaction. E.g., airfare expenses require a ticket_number
, taxi expenses require destination
and purpose
fields, etc. These fields show up in the XML report roughly as
<custom>%%%destination%%%Airport%%%purpose%%%client travel%%%</custom>
The XML schema for all expense types is thus the same.
Current solution
There is a single Transaction
model, which mirrors the XML schema and has fields like item_date
, amount
, client_code
, employee_id
, etc. It also has a custom
field, which stores the custom fields, serialized the same way they are serialized in the XML report.
In order to keep the custom field seralization/deserialization code near the model, I've used Django proxy models to create subclasses that map to the same table but provide slightly different functionality.
from django.db import models
class Transaction(models.Model):
MEAL = '5'
# ...
TAXI = '2'
EXPENSE_TYPE_CHOICES = (
(MEAL, 'Meal'),
# ...
(TAXI, 'Taxi'),
)
date_created = models.DateTimeField(auto_now_add=True)
item_date = models.DateField()
expense_type = models.CharField(choices=EXPENSE_TYPE_CHOICES, max_length=4)
# ...
custom = models.CharField(max_length=100)
def as_subclass(self):
if self.expense_type == self.MEAL:
self.__class__ = MealTrx
elif self.expense_type == self.TAXI:
self.__class__ = TaxiTrx
# ...
else:
raise ValueError("Expense type not set!")
class MealTrx(Transaction):
objects = TypedManager({'expense_type': Transaction.MEAL})
# TypedManager adds .filter(**kwargs) to .get_queryset()
class Meta:
proxy = True
detail_re = re.compile("^%%%attendees%%%(.*)%%%purpose%%%(.*)$")
detail_str = "%%%attendees%%%{}%%%purpose%%%{}"
@property
def attendees(self): # Comma-separated list of attendees
match = self.detail_re.match(self.custom)
if match is None:
return []
return match.group(1).split(", ")
@attendees.setter
def attendees(self, value):
args = (", ".join(value), self.purpose)
self.custom = self.detail_str.format(*args)
@property
def purpose(self):
match = self.detail_re.match(self.custom)
if match is None:
return ""
return match.group(2)
@purpose.setter
def purpose(self, value):
args = (", ".join(self.attendees), value)
self.custom = self.detail_str.format(*args)
How the code gets called
1. Creating expenses
This takes place inside the new transaction form save()
method. The attributes are assigned and the model is saved. The property setters ensure that the fields are properly serialized:
def save(self):
trx = base_form.save(commit=False) # base_form is a ModelForm for Transaction
trx.as_subclass()
if trx.expense_type == models.Transaction.MEAL:
trx.attendees = self.meal_form.cleaned_data['attendees']
trx.purpose = self.meal_form.cleaned_data.get('purpose', "")
elif trx.expense_type == models.Transaction.TAXI:
# ...
trx.save()
return trx
2. Viewing expenses
This takes place inside template code, where the approach is to test for all of the possible custom fields:
def expense_detail(request, expense_id):
expense = get_object_or_404(models.Transaction, pk=expense_id)
expense.as_subclass()
if expense.user != request.user:
LOGGER.warning("%r tried to see details of Transaction %s",
request.user.username, expense_id)
raise http.Http404
return render(request, 'expenses/detail.html', {'expense': expense})
and then inside the Jinja2
template
{%- if expense.attendees -%}
<div class="table-row">
<span class="table-cell">Attendees</span>
<div class="table-cell">
<ul class="attendee-list">
{%- for attendee in expense.attendees -%}
<li>{{ attendee }}</li>
{%- endfor -%}
</ul>
</div>
</div>
{%- endif -%}
3. Generating reports
This is the most straightforward piece, since the database schema mirrors the XML schema. Roughly speaking:
class Report:
# snip
def to_string(self, encoding='utf-8'):
# build the document root, header info, etc.
for trx in self. transactions:
transactions.append(self.trx_as_element(trx))
return ET.tostring(root, encoding=encoding)
def trx_as_element(self, trx):
root = ET.Element('doc_trans')
items = [
('item_date', transaction.item_date.strftime("%d-%b-%Y")),
('expense_type', transaction.expense_type),
# ...
('custom', transaction.custom),
]
for key, val in items:
ET.SubElement(root, key).text = val
return root
Identified issues
- Adding a new expense type requires modifying a lot of places
- There's a lot of repetition in the definition of properties
- I'm not super-comfortable with the assignment of
__class__
inas_subclass()
. That feels too magic, and asking client code to remember to callas_subclass()
seems like a bad idea - Accessing and modifying the custom properties is expensive (I think), requiring 1 or 2 regex matches and string interpolation every time
Bottom-line: Is this code readable? Understandable? Easy to use? Would adding a new expense type, or creating a view to edit existing expenses be unduly painful?
-
\$\begingroup\$ Is this actual, working code, or hypothetical and stub code? I'm a little uncertain whether this is good for Code Review or not... \$\endgroup\$holroy– holroy2015年12月03日 01:40:53 +00:00Commented Dec 3, 2015 at 1:40
-
\$\begingroup\$ This is coming from deployed code. I tried slimming it down to focus on the key points. Happy to add more code if helpful \$\endgroup\$Felipe– Felipe2015年12月03日 01:45:37 +00:00Commented Dec 3, 2015 at 1:45
1 Answer 1
What you could do is a CustomField
parent class (with TaxiField
and MealField
as subclasses).
Then you use implement some getters and setter on your Transaction
Model to retrieve those models.
In your CustomField
you can then implement a __str__
method to simply call ('custom', str(transaction.custom))
when you need to render it.
In the CustomField
parent class, you can store the subfields as an OrderedDict
and create getters and setters like: set(field_name, field_value)
(to prevent method like attendees()
)
-
\$\begingroup\$ Thanks -- I think this is a good idea to encapsulate a lot of this repetitive behavior and potentially get rid of the
__class__
assignment. It might even help with the template code. I'll work on it a bit and post back \$\endgroup\$Felipe– Felipe2015年12月03日 19:31:14 +00:00Commented Dec 3, 2015 at 19:31 -
\$\begingroup\$ Sure, please comment on this answer when you have your new code, so that I can have the chance to look at it :-) \$\endgroup\$oliverpool– oliverpool2015年12月03日 21:16:34 +00:00Commented Dec 3, 2015 at 21:16
Explore related questions
See similar questions with these tags.