HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythondjangoMinor

Bank account model implementation in Django

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
djangoaccountbankimplementationmodel

Problem

There is a draft of my models.py. What can I do for code quality and readability increase?

```
from datetime import timedelta

from django.contrib.auth.models import User
from django.db import models
from django.utils import timezone

class Rule(models.Model):
start_date = models.DateField()
end_date = models.DateField()

deposit_percent = models.FloatField()
credit_percent = models.FloatField()

@staticmethod
def on_date(_date):
return Rule.objects.get(start_date__lte=_date, end_date__gte=_date)

class BankAccount(models.Model):

LEGAL = 1
NATURAL = 2

DEBET = 1
CREDIT = 2

owner_type = models.PositiveSmallIntegerField(choices=(
(LEGAL, 'Физическое лицо'),
(NATURAL, 'Юридическое лицо'),
))

method = models.PositiveSmallIntegerField(choices=(
(DEBET, 'Дебет'),
(CREDIT, 'Кредит'),
))

name = models.CharField(max_length=254)
owner = models.ForeignKey(User, related_name='w_accounts')
spectators = models.ManyToManyField(User, related_name='r_accounts')
when_opened = models.DateField()

@property
def balance(self):
assert self.method == self.DEBET
today = timezone.now()
rule = Rule.on_date(self.when_opened)
result = 0
current_date = self.when_opened
while current_date <= today:
result -= sum(x.total for x in self.cashing_set.on_date(current_date))
result -= sum(x.total for x in self.outcome_transfers.on_date(current_date))
result += sum(x.total for x in self.deposit_set.on_date(current_date))
result += sum(x.total for x in self.income_transfers.on_date(current_date))
result = 1 + rule.deposit_percent 0.01
current_date += timedelta(days=1)
return result

@property
def debt(self):
assert self.method == self.CREDIT
today = timezone.now()
rule = Rule.on_date(self.when_opened)
result = 0

Solution

In general this seems fine, easy to read, you seem to be following PEP8, that's excellent.

At first look, this repeated logic in balance and debt jumps in the eye:

@property
def balance(self):
    # ...
    while current_date <= today:
        result -= sum(x.total for x in self.cashing_set.on_date(current_date))
        result -= sum(x.total for x in self.outcome_transfers.on_date(current_date))
        result += sum(x.total for x in self.deposit_set.on_date(current_date))
        result += sum(x.total for x in self.income_transfers.on_date(current_date))
        # ...

@property
def debt(self):
    # ...
    while current_date <= today:
        result += sum(x.total for x in self.cashing_set.on_date(current_date))
        result += sum(x.total for x in self.outcome_transfers.on_date(current_date))
        result -= sum(x.total for x in self.deposit_set.on_date(current_date))
        result -= sum(x.total for x in self.income_transfers.on_date(current_date))
        # ...


My immediate reaction is to extract those summations to a helper method, for example:

def sum_total_transfers(self):
    result = 0
    result -= sum(x.total for x in self.cashing_set.on_date(current_date))
    result -= sum(x.total for x in self.outcome_transfers.on_date(current_date))
    result += sum(x.total for x in self.deposit_set.on_date(current_date))
    result += sum(x.total for x in self.income_transfers.on_date(current_date))
    return result


So that the above methods can reuse that with less duplication:

@property
def balance(self):
    # ...
    while current_date <= today:
        result += self.sum_total_transfers()
        # ...

@property
def debt(self):
    # ...
    while current_date <= today:
        result -= self.sum_total_transfers()
        # ...


On closer look, these assert lines don't look good:

@property
def balance(self):
    assert self.method == self.DEBET
    # ...

@property
def debt(self):
    assert self.method == self.CREDIT
    # ...


These assertions force the callers to check the value of self.method before using these properties,
or to organize the code in a way that ensures that .debt will only be called when self.method == self.CREDIT, otherwise the program would crash.
It would be better to avoid such unwritten rules.

For example, it might be better to have one public property that applies the correct calculation based on the method:

@property
def balance(self):
    if self.method == self.DEBET:
        return self._credit_balance
    return self._debt_balance

@property
def _credit_balance(self):
    assert self.method == self.DEBET
    # ...

@property
def _debt_balance(self):
    assert self.method == self.CREDIT
    # ...


Lastly, "debt" is misspelled in self.DEBET.

Code Snippets

@property
def balance(self):
    # ...
    while current_date <= today:
        result -= sum(x.total for x in self.cashing_set.on_date(current_date))
        result -= sum(x.total for x in self.outcome_transfers.on_date(current_date))
        result += sum(x.total for x in self.deposit_set.on_date(current_date))
        result += sum(x.total for x in self.income_transfers.on_date(current_date))
        # ...

@property
def debt(self):
    # ...
    while current_date <= today:
        result += sum(x.total for x in self.cashing_set.on_date(current_date))
        result += sum(x.total for x in self.outcome_transfers.on_date(current_date))
        result -= sum(x.total for x in self.deposit_set.on_date(current_date))
        result -= sum(x.total for x in self.income_transfers.on_date(current_date))
        # ...
def sum_total_transfers(self):
    result = 0
    result -= sum(x.total for x in self.cashing_set.on_date(current_date))
    result -= sum(x.total for x in self.outcome_transfers.on_date(current_date))
    result += sum(x.total for x in self.deposit_set.on_date(current_date))
    result += sum(x.total for x in self.income_transfers.on_date(current_date))
    return result
@property
def balance(self):
    # ...
    while current_date <= today:
        result += self.sum_total_transfers()
        # ...

@property
def debt(self):
    # ...
    while current_date <= today:
        result -= self.sum_total_transfers()
        # ...
@property
def balance(self):
    assert self.method == self.DEBET
    # ...

@property
def debt(self):
    assert self.method == self.CREDIT
    # ...
@property
def balance(self):
    if self.method == self.DEBET:
        return self._credit_balance
    return self._debt_balance

@property
def _credit_balance(self):
    assert self.method == self.DEBET
    # ...

@property
def _debt_balance(self):
    assert self.method == self.CREDIT
    # ...

Context

StackExchange Code Review Q#116095, answer score: 7

Revisions (0)

No revisions yet.