I know an awesome software engineer. He’s very smart and a joy to work with. He’s platinum-grade material, and I’d work with him again in a femtosecond.
On rare occasions, this Pythonista among Pythonistas and Djangonaut among Djangonauts writes code that makes me scratch my head.
He’ll do this:
@classmethod
def _clean_field(cls, data, field):
"""Simple cleaning."""
return data.get(field, '').strip()
def clean_series_code(self):
"""Clean series code."""
return self._clean_field(self.cleaned_data, self.FORM_FIELDS.SERIES_CODE)
def clean_oan(self):
"""Clean OAN."""
return self._clean_field(self.cleaned_data, self.FORM_FIELDS.OAN).replace(',', '')
def clean_grant_number(self):
"""Clean grant number."""
return self._clean_field(self.cleaned_data, self.FORM_FIELDS.GRANT_NUMBER).replace(',', '')
def clean_publication_number(self):
"""Clean publication number."""
return self._clean_field(self.cleaned_data, self.FORM_FIELDS.PUBLICATION_NUMBER)
The one-line function that only does a strip() makes my eyeballs itch. I would’ve forgone the _clean_field method:
def clean_series_code(self):
"""Clean series code."""
return self.cleaned_data.get(self.FORM_FIELDS.SERIES_CODE, '').strip()
def clean_oan(self):
"""Clean OAN."""
return self.cleaned_data.get(self.FORM_FIELDS.OAN, '').replace(',', '').strip()
def clean_grant_number(self):
"""Clean grant number."""
return self.cleaned_data.get(self.FORM_FIELDS.GRANT_NUMBER, '').replace(',', '').strip()
def clean_publication_number(self):
"""Clean publication number."""
return self.cleaned_data.get(self.FORM_FIELDS.PUBLICATION_NUMBER, '').strip()
It’s understandable that he wanted to abstract how the data was cleaned. But I would argue that strip() is sufficiently innocuous and clear. And _clean_field is one more reason for my eyeballs to have to move on the page. If my eyeballs have to move, I’d like them to receive a better reward than finding a function that just calls a string method and returns.
(Updated 2/9 to make it clearer what I’m scratching my head about. I wish I were a better writer. I should have studied more in school.)
Seek Nuance 
Surely it’s easier to put that in the generic clean method:
def clean(self): for fld in ['publication_number', 'oan', 'grant_number', 'series_code']: self.cleaned_data[fld] = self.cleaned_data.get(fld,'').replace(',','').strip() ...Seems like a classic case of DRY
The code has changed from when we originally did it, but assuming I was right in the head when I did it, the reasons I usually have for picking multiple-per-field cleans (from https://docs.djangoproject.com/en/dev/ref/forms/validation/) are:
1. Clean order: clean_FIELDNAME comes before clean(), and this can impact / simplify an inheritance scenario, as well as give assurances of what state the data is at by the time you reach clean().
2. Semantically, clean() is for access to multiple fields that interact. (Kind of a punt, but that’s what the docs say).
3. Form validation errors: clean_FIELDNAME automatically attaches to the correct field just from throwing ValidationError. Any errors in clean() get stuck on __all__ unless you manually dig out the errors internal dictionary and set them yourself. (Although that’s not the case here, and we do do that elsewhere in the larger corpus of the code from which this example came).
All said, I don’t see a particular need for clean_FIELDNAME in the above scenario as it is now.
How about something like:
def clean(self): # call super here fields_cleans = { 'field_1': lambda x: x.strip(), 'field_2'; lambda x: x.replace(',' ' ') } for field in fields_cleans: self.cleaned_data[field] = fields_cleans[field](self.cleaned_data[field])A more declarative way:
class StrippedCharField(forms.CharField): def to_python(self, value): value = super(StrippedCharField, self).to_python(value) if value is not None: value = value.strip() return value class MyForm(forms.Form): series_code = StrippedCharField() grant_number = StrippedCharField()