Practical guidelines for checking the correct form of Django?

Question

I have a form containing 5 pairs of places and descriptions. I have three sets of checks that need to be performed

• You must enter at least one location.
• for the first location you must have a description
• for each remaining pair of locations and descriptions

After reading the Django documentation, I applied the following code to perform these custom checks

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 very ugly. I am looking for a more "Pythonic" and "Djangoist" (?) A way to do this. Thanks in advance.

Answer 1

The first thing you can do is simplify testing for cases where you want to see if only one of the two fields is filled. You can implement a logical xor as follows:

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

or as follows:

 if bool(description2) ^ bool(location2): 

I also think that it would be more clear if you implemented a clean method for each field separately, as described in. This ensures that the error appears in the right field and allows you to simply raise forms.ValidationError , and not directly access the _errors object.

For instance:

 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') 

To get behavior that requires location1 , just use required=True for this field and it will be processed automatically.

Answer 2

At least you can reduce the 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 ...