5
\$\begingroup\$

We have a "Committee" class and I need to ensure that any new committee has start and end dates (don't know why they're CharFields instead of Dates or even Ints, but that's not my call) that are after any existing instances. This works, but I would appreciate any general review as well as specifically on my solution to my test problem: When no Committees exist, my tests broke as noted in the comment. My solution worked but feels hacky.

class Committee(models.Model):
    start_year = models.CharField(max_length=4)
    end_year = models.CharField(max_length=4)

    def save(self, *args, **kwargs):
        # new committees need to ensure start and end dates are in the future

        if not self.pk:
            committee_set = Committee.objects.all()
            latest_start_year = committee_set.aggregate(Max('start_year'))["start_year__max"]
            latest_end_year = committee_set.aggregate(Max('end_year'))["end_year__max"]

            # in the test database when there are no other committees yet, these evaluate to
            # None and break the tests. Doesn't really affect production code otherwise.
            try:
                latest_start_year = int(latest_start_year)
            except TypeError:
                latest_start_year = 0
            try:
                latest_end_year = int(latest_end_year)
            except TypeError:
                latest_end_year = 1

            if not (self.start_year > latest_start_year):
                raise ValidationError("Start year (%s) must be greater than most recent "
                                      "Committee start year (%s)" % (
                                          self.start_year, latest_start_year))

            if not (self.end_year > latest_end_year):
                raise ValidationError("End year (%s) must be greater than most recent "
                                      "Committee end year (%s)" % (self.end_year, latest_end_year))


        super(Committee, self).save(*args, **kwargs)
\$\endgroup\$
2
\$\begingroup\$

Instead of trying to convert to int and catching TypeError, it would be better to check for None values.

Instead of committee_set.aggregate(Max('start_year'))["start_year__max"], you could give a custom alias to the max value, making the code slightly more compact.

Instead of getting the latest start and end years in the beginning, it would be better to delay getting the latest end year until it's needed. That way you may be able to avoid one unnecessary query.

Instead of the style formatting strings with '%s' % (...), the modern way is to use '{}'.format(...).

Combining the above tips, I would write the main part like this:

query = Committee.objects

latest_start_year = query.aggregate(max=Max('start_year'))['max']
if latest_start_year and int(self.start_year) <= int(latest_start_year):
    raise ValidationError("Start year {} must be greater than most recent {}".format(self.start_year, latest_start_year))

latest_end_year = query.aggregate(max=Max('end_year'))['max']
if latest_end_year and int(self.end_year) <= int(latest_end_year):
    raise ValidationError("End year {} must be greater than most recent {}".format(self.end_year, latest_end_year))

If it's not very important to include in the error message the latest start and end years, a more compact and more efficient solution is possible:

query = Committee.objects

if query.filter(start_year__gt=self.start_year).exists():
    raise ValidationError("Start year must be greater than most recent")

if query.filter(end_year__gt=self.end_year).exists():
    raise ValidationError("End year must be greater than most recent")

And let's not forget about test cases. Django provides a rich framework for testing. Here are some examples:

class CommitteeTests(TestCase):
    def test_create_first(self):
        self.assertEqual(0, Committee.objects.count())
        Committee(start_year='1991', end_year='1992').save()
        self.assertEqual(1, Committee.objects.count())

    def test_create_second_valid(self):
        self.assertEqual(0, Committee.objects.count())

        Committee(start_year='1991', end_year='1995').save()
        self.assertEqual(1, Committee.objects.count())

        Committee(start_year='1992', end_year='1996').save()
        self.assertEqual(2, Committee.objects.count())

    def test_should_fail_if_start_is_before_latest_start(self):
        Committee(start_year='1991', end_year='1995').save()
        with self.assertRaises(ValidationError):
            Committee(start_year='1990', end_year='1995').save()

    def test_should_fail_if_end_is_before_latest_end(self):
        Committee(start_year='1991', end_year='1995').save()
        with self.assertRaises(ValidationError):
            Committee(start_year='1992', end_year='1994').save()

Lastly, you didn't mention about it, but the current code allows Committee instances having start date after end date. Probably it would be a good idea to prevent that too, not to mention valid numeric values. These validations should come first before the others querying the database, and there should be test cases for them too.

\$\endgroup\$
  • \$\begingroup\$ The filter is a great idea. I didn't just check for None values since I had to convert to an int to compare them anyway. I have lots of those tests already--I didn't include them in the question. Should I have? I was going for making y'all read as little as possible. \$\endgroup\$ – thumbtackthief May 30 '17 at 18:12
  • 1
    \$\begingroup\$ @thumbtackthief it's too late for this question (as you should not modify code after you received answers), but next it will be a good idea to include test cases, so that they get reviewed as well. \$\endgroup\$ – janos May 30 '17 at 18:16
  • \$\begingroup\$ Ok... I was pretty confident in those, though (they're almost identical to yours!) \$\endgroup\$ – thumbtackthief May 30 '17 at 18:22
1
\$\begingroup\$

In addition to @janos's answer.

I would recommend using a model form instead of overriding model's save method.

Advantages of using forms:

  1. Display an HTML form with automatically generated form widgets.
  2. Check submitted data against a set of validation rules.
  3. Redisplay a form in the case of validation errors.
  4. Convert submitted form data to the relevant Python data types.

Secondly split the validators to individual validator functions. That way you can also use them at other places if required.

Validators

import re


def validate_year(year):
    pattern = re.compile('^\d{4}$')

    if not pattern.match(year):
        raise ValidationError('{year} is not a valid year.'.format(year=year))


def validate_start_year(start_year):
    latest_start_year = Committee.objects.aggregate(Max('start_year'))["start_year__max"]
    latest_start_year = int(latest_start_year or 0)
    if start_year <= latest_start_year:
        raise ValidationError(
            "Start year {start_year} must be greater than most recent "
            "Committee start year {latest_start_year}".format(
                start_year=start_year, latest_start_year=latest_start_year))


def validate_end_year(end_year):
    latest_end_year = Committee.objects.aggregate(Max('end_year'))["end_year__max"]
    latest_end_year = int(latest_end_year or 1)
    if end_year <= latest_end_year:
        raise ValidationError(
            "End year {end_year} must be greater than most recent "
            "Committee end year {latest_end_year}".format(
                end_year=end_year, latest_end_year=latest_end_year))

Models

class Committee(models.Model):
    start_year = models.CharField(max_length=4, validators=[validate_year, validate_start_year])
    end_year = models.CharField(max_length=4, validators=[validate_year, validate_end_year])

Forms

class CommitteeForm(ModelForm):
    def clean_start_year(self):
        return int(self.cleaned_data['start_year'])

    def clean_end_year(self):
        return int(self.cleaned_data['end_year'])

    class Meta:
        fields = ['start_year', 'end_year']

Views

from django.http import JsonResponse


def add_committee(request):
    form = CommitteeForm(request.POST)
    if not form.is_valid():
        return JsonResponse(form.errors, status=404)
    ...
\$\endgroup\$
  • \$\begingroup\$ It's through an API, not a form \$\endgroup\$ – thumbtackthief May 30 '17 at 19:17
  • \$\begingroup\$ @thumbtackthief Check the views example, it would work with any sort of incoming data. The form errors can also be used by the consumer to display errors. \$\endgroup\$ – Ashwini Chaudhary May 30 '17 at 20:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.