Transactionally create something, if allowed

Question

I have been looking over this block of code for hours trying to simplify it. Would there be a better way to check all those conditions before creating a transaction object without using so many if-elses?

def create(self, validated_data):

    user = self.context['request'].user
    if user.role == 'super_admin':  # ref PR 25.1
        return Transaction.objects.create(**validated_data)
    elif user.role == 'user' or user.role == 'org_admin' or user.role == 'site_admin':  # ref PR 25.4
        if check_user_institution_exists(validated_data['user_institution'].id):
            if check_user_belongs_to_institution(validated_data['user_institution'].id, None, user.id) > 0:
                if check_upload_permission(validated_data['user_institution'].id):
                    return Transaction.objects.create(**validated_data)
                else:
                    raise serializers.ValidationError({'Error': 'You do not have upload permission for this '
                                                                'institution.'})
            else:
                raise serializers.ValidationError({'Error': 'You do not belong to this institution.'})
        else:
            raise serializers.ValidationError({'Error': 'This user institution does not exist.'})
    else:
        raise serializers.ValidationError({'Error': 'You are not assigned a role.'})

Are you possibly missing an else for the outer-most if, elif? — Peilonrayz
– Peilonrayz ♦, Commented Oct 1, 2018 at 14:20

Peilonrayz · Accepted Answer · 2018-10-01 14:35:51Z

You don't need to use elif you can change that to just an if.
You can simplify code blocks by using guard clauses that have the following layout:
```
if a:
    ...
else:
    raise b
```
To:
```
if not a:
    raise b

...
```
You can use a in [b, c] rather than a == b or a == c.
As commented by Michel Billaud, "The code will be shorter with an auxiliary variable id = validated_data['user_institution'].id".

Employing the above would get the following which I find to be much easier to read.

def create(self, validated_data):
    user = self.context['request'].user
    if user.role == 'super_admin':  # ref PR 25.1
        return Transaction.objects.create(**validated_data)

    if user.role not in ['user', 'org_admin', 'site_admin']:  # ref PR 25.4
        raise serializers.ValidationError({'Error': 'You are not assigned a role.'})

    id_ = validated_data['user_institution'].id
    if not check_user_institution_exists(id_):
        raise serializers.ValidationError({'Error': 'This user institution does not exist.'})

    if check_user_belongs_to_institution(id_, None, user.id) <= 0:
        raise serializers.ValidationError({'Error': 'You do not belong to this institution.'})

    if not check_upload_permission(id_):
        raise serializers.ValidationError({'Error': 'You do not have upload permission for this '
                                                    'institution.'})

    return Transaction.objects.create(**validated_data)

The code will be shorter with an auxiliary variable id = validated_data['user_institution'].id — Michel Billaud
– Michel Billaud, Commented Oct 1, 2018 at 14:32
also, the last three tests could be delegated to a function call assert_user_institution_allows_upload(id_,institution_) whose role is to raise some exception if user is not allowed. — Michel Billaud
– Michel Billaud, Commented Oct 3, 2018 at 14:56

Stack Exchange Network

Transactionally create something, if allowed

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Transactionally create something, if allowed

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions