Conditionally set attributes in MongoDB based on form fields

Question

I have code which gets data from a POST request (request.form dict), compares with corresponding attribute in object accident and does some action. The code looks needlessly verbose. How can I optimize it?

if accident.status == 1:
 if request.form.get('reason') \
 and request.form['reason'] != accident.reason:
 accident.set_reason(request.form['reason'], current_user)
 if request.form.get('note') \
 and request.form['note'] != accident.note:
 accident.note = request.form['note']
else:
 if request.form.get('priority') \
 and int(request.form['priority']) != accident.priority:
 accident.set_priority(current_user, int(request.form['priority']))
 if request.form.get('status') \
 and int(request.form['status']) != accident.status:
 accident.set_status(current_user, int(request.form['status']))
 if request.form.get('duration') \
 and int(request.form['duration']) != accident.duration:
 accident.duration = int(request.form['duration'])
 if request.form.get('service_status') \
 and int(request.form['service_status']) != accident.service_status:
 accident.set_service_status(int(request.form['service_status']))

Below is a stripped down version of Accident class, which is inconsistent because there are many fields, and sometimes setting a field requires many additional actions like logging or checking for user rights. The class derives from MongoEngine schema class.

class Accident(Document):
 priority = IntField(choices=PRIORITIES)
 status = IntField(choices=STATUSES, default=0)
 duration = IntField()
 region = StringField()
 service_status = IntField(choices=(0, 1, 2))
 reason = StringField(default=u"")
 note = StringField(default=u"")
 def set_priority(self, user=None, priority=None):
 self.priority = priority
 # Functions starting with `set_` do some activity that has
 # nothing to do with setting and validating a value. That's
 # why i don't use @property decorator. `to_history(user)` is
 # just an example.
 to_history(user)
 def set_status(self, user, status):
 self.status = status
 to_history(user)
 def set_reason(self, reason, user):
 self.reason = reason
 to_history(user)
 def set_service_status(self, ss):
 self.service_status = ss
 to_history(user)

If this is necessary, how can i improve class structure? It seems like I can't use @property decorator for setters, because my setters accept additional parameters. However I don't like the idea of writing a setter for all possible attributes. I use request.form.get(foo) instead of foo in request.form, because the dictionary may contain foo: None pair, and I don't want to accidentally set an attribute to None.

Answer 1

I'm not 100% sure there is a particularly elegant way to write this. If the code did not have a different body for each if statement, one could rewrite it like this

FORM_VALUES = {
 "priority": accident.priority,
 "status": accident.status,
 "duration": accident.duration,
 "service_status": accident.service_status
}
for key in FORM_VALUES:
 if request.form.get(key) and int(request.form[key]) != FORM_VALUES[key]:
 # As you use different functions for setting the values, this isn't possible
 request.set_value(key, request.form[key])

However, that is not the case, so don't get your hopes up. Regardless, there are still improvements to be made:

• You're not checking that the header values are valid integers. I'd recommend either checking they are by using string.isdigit() or otherwise wrapping your calls to int() in a try except block.
• You can use 'reason' in request.form instead of request.form.get('reason'); they have the same meaning in the places you've used them, but the first is easier to read.
• It might be good if you used an enumeration for the values to Accident.status. As it is, checking that accident.status == 1 doesn't tell me anything; using a meaningful identifier might solve this. You can do this quite simply, by just assigning values to each identifier: PENDING, IN_PROGRESS, FINISHED = STATUSES. I've used some random names, and assumed that there are 3 possible statuses, change this to be accurate. Then in your function, instead of if accident.status == 1, we can have if accident.status == PENDING, which is a lot easier to understand without needing to know the meaning of each value of status.
• In general, you should try and be consistent with the ordering of your arguments: your set_reason takes first a reason, then a user argument, while all of your other methods take the user argument first. The set_service_status doesn't take a user argument at all, which is fairly impressive, considering it uses a user variable in the function.

Overall not bad code. I don't agree with your comments on how @property would be inappropriate there, but it's far from the end of the world. In general, I'd try and be more careful to validate user input, but it may be that you do not need to worry about that.

Answer 2

I'd like to see this as my goal:¹

if accident.status == 1:
 transfer_from_form(request.form, accident, [
 ('reason', str),
 ('note', str),
 ])
else:
 transfer_from_form(request.form, accident, [
 ('priority', int),
 ('status', int),
 ('duration', int),
 ('service_status', int),
 ])

Let's see what it takes to get there! I imagine that transfer_from_form() could look like this:

def transfer_from_form(form, obj, fields):
 for field, conv in fields:
 if form.get(field) and form[field] != getattr(obj, field):
 setattr(obj, field, conv(form[field]))

Notionally, that would perform the equivalent of assignments like accident.service_status = int(request.form['service_status']) whenever the form field is set and the attribute on accident doesn't already contain that value.

Then there is the pesky issue of how to pass the user parameter. I would argue that the user should be stored in the session somewhere, and the Accident's setter method could pluck it out of the session instead of having it passed in explicitly. You didn't specify which web framework you are using, so I can't tell you exactly how to accomplish that.

The Accident class's setters appear to be wrappers around the MongoEngine fields' setters, with some audit logging inserted. Instead of defining methods like set_service_status(), you could hook in the audit logging by monkey patching the MongoEngine fields' setters. Not only would the code read more smoothly, you wouldn't be relying on the goodwill of the users of the Accident class to refrain from setting accident.service_status directly instead of going through the approved set_service_status() method.

---

¹ There is some redundancy here. Ideally, I shouldn't have to declare that 'reason' is a str and 'service_status' is an int, because the types should be deducible from the fact that they are a StringField and IntField, respectively. If you can figure out how to accomplish that deduction, that would be even better.

• sea-rob: I'm also thinking (broadly) about "closure" to handle that pesky user parameter.
• Michael Urman: Shouldn't obj.getattr(field) be getattr(obj, field) (similarly obj.setattr(...) vs setattr(obj, ...))? Anyway, totally agreed on refactoring that part out, although I would prefer multiple calls over passing a list of parameters.
• 200_success: @MichaelUrman Thanks for spotting the error. Corrected in Rev 2.

Answer 3

Don't forget that dict.get() can also take a default:

if request.form.get('reason', accident.reason) != accident.reason:
 accident.set_reason(request.form['reason'], current_user)

...that makes it a little shorter.

EDIT: or even

def needs_update(name):
 return request.form.get(name, getattr(accident, name)) != getattr(accident, name)
if needs_update('reason'):
 ...

Barring @BluePepper's elegant solution, you could probably build a change list and then iterate through the fields that need updating.

This is very cheesy, but you could break the value check and the update in two parts. I did this fast, sorry, but I hope the general idea is clear:

form = {'a': 1, 'b': 2, 'c': 3}
original = {'a': 1, 'b': 20, 'c': 30, 'd': 4}
# build the list of new values as a list of tuples
update = [(k, form.get(k)) for k in form.keys() if form.get(k) != original.get(k, None)]
print update
# make the updates, making special calls as necessary
for k, v in update:
 if k == 'c':
 print "DO SOMETHING SPECIAL"
 original['c'] = v
 else:
 original[k] = v
print original

Produces

[('c', 3), ('b', 2)]
DO SOMETHING SPECIAL
{'a': 1, 'c': 3, 'b': 2, 'd': 4}