code doctor test #55
code doctor test #55
Conversation
![code-review-doctor[bot]](https://avatars.githubusercontent.com/in/83430?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good. Worth considering though. View full project report here.
| avatar_image_url = models.CharField( | ||
| max_length=1024, null=True, blank=True, default=DEFAULT_AVATAR_URL | ||
| ) |
There was a problem hiding this comment.
| avatar_image_url = models.CharField( | |
| max_length=1024, null=True, blank=True, default=DEFAULT_AVATAR_URL | |
| ) | |
| avatar_image_url = models.TextField(blank=True, default="") |
TextField might be better used here, instead of CharField with a huge max_length. More info.
null=True on a string field causes inconsistent data types because the value can be either str or None. This adds complexity and maybe bugs, but can be solved by replacing null=True with default="". More.
| """A membership is a mapping of a user to a collective.""" | ||
|
|
||
| member = models.ForeignKey("auth.User", on_delete=models.CASCADE) | ||
| collective = models.ForeignKey("purchases.Collective", on_delete=models.CASCADE) |
There was a problem hiding this comment.
Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. Read more.
| name = models.CharField(max_length=100) | ||
| price = MoneyField(max_digits=10, decimal_places=2, default_currency="EUR") | ||
| buyer = models.ForeignKey("auth.User", on_delete=models.CASCADE) | ||
| collective = models.ForeignKey("purchases.Collective", on_delete=models.CASCADE) |
There was a problem hiding this comment.
Again, with an explicit related_name would be better.
| creditor = models.ForeignKey( | ||
| "auth.User", related_name="creditor", on_delete=models.CASCADE | ||
| ) | ||
| collective = models.ForeignKey("purchases.Collective", on_delete=models.CASCADE) |
There was a problem hiding this comment.
Similarly, with an explicit related_name would be better.
| unique_together = ("member", "collective") | ||
|
|
||
| def __str__(self): | ||
| return u"{} in {}".format(self.member.username, self.collective.name) |
There was a problem hiding this comment.
f-string is easier to read, write, and less computationally expensive than legacy string formatting. More details.
| reactions = GenericRelation(Reaction) | ||
|
|
||
| def __str__(self): | ||
| return u"{} for {} by {} in {}".format( |
There was a problem hiding this comment.
Again, Consider using f-string instead.
| reactions = GenericRelation(Reaction) | ||
|
|
||
| def __str__(self): | ||
| return u"{} from {} to {} in {}".format( |
There was a problem hiding this comment.
As above, Consider using f-string instead.
Feature/#64 split purchase by weights
|
|
||
|
|
||
| class PurchaseWeight(TimestampMixin, models.Model): | ||
| purchase = models.ForeignKey("purchases.Purchase", on_delete=models.CASCADE) |
There was a problem hiding this comment.
Django automatically creates a related_name if it's not set. If it were set then a more readable and explicit relationship is set up. More details.
|
|
||
| class PurchaseWeight(TimestampMixin, models.Model): | ||
| purchase = models.ForeignKey("purchases.Purchase", on_delete=models.CASCADE) | ||
| member = models.ForeignKey("auth.User", on_delete=models.CASCADE) |
There was a problem hiding this comment.
Again, with an explicit related_name would be better.
Makes it easier to e.g. use 3 for everyone except one person.
No description provided.