Conditionally set attributes in MongoDB based on form fields

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 val

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.

StackExchange Code Review Q#42259, answer score: 8