Как упростить код проверок?

Question

[Twelfth Doctor]

Доброго времени суток. У меня в контроллере есть код с проверками:

if not Validation.is_int(data['vhid']):
            return json.dumps({'status': 'error', 'error': "Incorrect id of virtual host"})
        try:
            Websites.get(Websites.unid == vhid)
        except Websites.DoesNotExist:
            return json.dumps({'status': 'error', 'error': "Website doesn't exist"})
        usertoken = data['usertoken']
        try:
            user = Users.get(Users.api_token == usertoken)
        except Users.DoesNotExist:
            return json.dumps({'status': 'error', 'error': "User doesn't exist"})
        if not validators.email(data['email']):
            return json.dumps({'status': 'error', 'error': "Invalid e-mail"})
        if not validators.domain(data['mainname']):
            return json.dumps({'status': 'error', 'error': "Incorrect domain name"})
        try:
            Domains.get(Domains.fullname == Websites.mainname)
        except Domains.DoesNotExist:
            return json.dumps({'status': 'error', 'error': "Domain doesn't exist"})
        if user.role == 'customer' and user.node != nodeid:
            return json.dumps({'status': 'error', 'error': "It seems you try to edit something on a foreign node"})

Как его можно улучшить (упростить)?

Answer 1

[Stanislav Pugachev]

ну если рябит в глазах от try-catch то всегда можно сделать чтото типа такого

try:
    if not Validation.is_int(data['vhid']):
        raise ValidationError("Incorrect id of virtual host")

    Websites.get(Websites.unid == vhid)
    usertoken = data['usertoken']
    user = Users.get(Users.api_token == usertoken)

    if not validators.email(data['email']):
        raise ValidationError("Invalid e-mail")
    if not validators.domain(data['mainname']):
        raise ValidationError("Incorrect domain name")
    
    Domains.get(Domains.fullname == Websites.mainname)
    
    if user.role == 'customer' and user.node != nodeid:
        raise ValidationError("It seems you try to edit something on a foreign node")
except Websites.DoesNotExist:
    return json.dumps({'status': 'error', 'error': "Website doesn't exist"})
except Users.DoesNotExist:
    return json.dumps({'status': 'error', 'error': "User doesn't exist"})
except Domains.DoesNotExist:
    return json.dumps({'status': 'error', 'error': "Domain doesn't exist"})
except ValidationError as e:
    return json.dumps({'status': 'error', 'error': e.message})

сказать что это лучше или правильней - нельзя
это скорей от ваших предпочтений зависит

что мне определенно не нравится:
вы выбираете модель из базы только для того что бы проверить существует ли он там или нет
то есть поидее можно избавиться от 2х веток
Websites.DoesNotExist:
Domains.DoesNotExist:
используя https://docs.djangoproject.com/en/1.8/ref/models/q...

то есть код может выглядеть примерно так

try:
    if not Validation.is_int(data['vhid']):
        raise ValidationError("Incorrect id of virtual host")

    if not Websites.filter(Websites.unid == vhid).exists():
        raise ValidationError("Website doesn't exist")

    usertoken = data['usertoken']
    user = Users.get(Users.api_token == usertoken)

    if not validators.email(data['email']):
        raise ValidationError("Invalid e-mail")
    if not validators.domain(data['mainname']):
        raise ValidationError("Incorrect domain name")
    
    if not Domains.filter(Domains.fullname == Websites.mainname).exists():
        raise ValidationError("Domain doesn't exist")
    
    if user.role == 'customer' and user.node != nodeid:
        raise ValidationError("It seems you try to edit something on a foreign node")
except Users.DoesNotExist:
    return json.dumps({'status': 'error', 'error': "User doesn't exist"})
except ValidationError as e:
    return json.dumps({'status': 'error', 'error': e.message})

код я не запускал ессно, но думаю идею вы поняли

и еще одно замечание:
старайтесь в первую очередь проверять то что не требует обращений к базе данных
это позволит сэкономить запросы и вцелом будет performance improvement.

Comments

[Twelfth Doctor]

Я не использую Django. У меня Peewee ORM.

[Stanislav Pugachev]

пардон
ну я думаю там тоже есть что-то типа exist()?

Answer 2

[Boris Köln]

Упростить - вряд ли. А вот сделать более читаемым и с возможностью re-use - можно.
У объекта указать массив проверок: имя свойства, валидатор (имя стороннего класса-валидатора или метода-валидатора этого объекта), дополнительные параметры. В цикле перебирать этот массив и вызывать валидаторы.

Answer 3

[Twelfth Doctor]

В результате упростил код таким образом:

try:
            user = Validation(unid=data['userid']).user()
        except ValidationError as exc:
            return json.dumps({'status': 'error', 'error': exc})
        if not validators.email(data['email']):
            return json.dumps({'status': 'error', 'error': "Invalid e-mail"})
        try:
            Validation(unid=user.id).check_node()
            domain = Domains.by_name(data['mainname'], user)
        except ValidationError as exc:
            return json.dumps({'status': 'error', 'error': exc})
        if domain.type != 'unlinked':
            return json.dumps({'status': 'error', 'error': "This domain is already linked to another website"})