Django custom form validation best practices?

Question

I have a form that contains 5 pairs of locations and descriptions. I have three sets of validations that need to be done

• you need to enter at least one location
• for the first location, you must have a description
• for each remaining pair of locations and description

After reading the Django documentation, I came up with the following code to do these custom validations

def clean(self):
    cleaned_data = self.cleaned_data
    location1 = cleaned_data.get('location1')
    location2 = cleaned_data.get('location2')
    location3 = cleaned_data.get('location3')
    location4 = cleaned_data.get('location4')
    location5 = cleaned_data.get('location5')
    description1 = cleaned_data.get('description1')
    description2 = cleaned_data.get('description2')
    description3 = cleaned_data.get('description3')
    description4 = cleaned_data.get('description4')
    description5 = cleaned_data.get('description5')
    invalid_pairs_msg = u"You must specify a location and description"

    # We need to make sure that we have pairs of locations and descriptions
    if not location1:
        self._errors['location1'] = ErrorList([u"At least one location is required"])

    if location1 and not description1:
        self._errors['description1'] = ErrorList([u"Description for this location required"])

    if (description2 and not location2) or (location2 and not description2):
        self._errors['description2'] = ErrorList([invalid_pairs_msg])

    if (description3 and not location3) or (location3 and not description3):
        self._errors['description3'] = ErrorList([invalid_pairs_msg])

    if (description4 and not location4) or (location4 and not description4):
        self._errors['description4'] = ErrorList([invalid_pairs_msg])

    if (description5 and not location5) or (location5 and not description5):
        self._errors['description5'] = ErrorList([invalid_pairs_msg])

    return cleaned_data     

Now, it works but it looks really ugly. I'm looking for a more "Pythonic" and "Djangoist"(?) way to do this. Thanks in advance.

Answer 1

At least you can reduce some code. Have 'location1' and 'description1' Required=True (as TM and stefanw pointed out). Then,

def clean(self):
   n=5
   data = self.cleaned_data
   l_d = [(data.get('location'+i),data.get('description'+i)) for i in xrange(1,n+1)]
   invalid_pairs_msg = u"You must specify a location and description"

   for i in xrange(1,n+1):
      if (l_d[i][1] and not l_d[i][0]) or (l_d[i][0] and not l_d[i][1]):
         self._errors['description'+i] = ErrorList([invalid_pairs_msg])
   return data

still ugly though...

Answer 2

First thing you can do is simplify your testing for those cases where you want to see if only one of the two fields is populated. You can implement logical xor this way:

if bool(description2) != bool(location2): 

or this way:

if bool(description2) ^ bool(location2):

I also think this would be more clear if you implemented a clean method for each field separately, as explained in the docs. This makes sure the error will show up on the right field and lets you just raise a forms.ValidationError rather than accessing the _errors object directly.

For example:

def _require_together(self, field1, field2):
    a = self.cleaned_data.get(field1)
    b = self.cleaned_data.get(field2)
    if bool(a) ^ bool(b):
        raise forms.ValidationError(u'You must specify a location and description')
    return a

# use clean_description1 rather than clean_location1 since
# we want the error to be on description1
def clean_description1(self):
    return _require_together('description1', 'location1')

def clean_description2(self):
    return _require_together('description2', 'location2')

def clean_description3(self):
    return _require_together('description3', 'location3')

def clean_description4(self):
    return _require_together('description4', 'location4')

def clean_description5(self):
    return _require_together('description5', 'location5')

In order to get the behavior where location1 is required, just use required=True for that field and it'll be handled automatically.