DEV Community

Discussion on: Refactoring an Overgrown Notifications Class in Django

Collapse
 
theodesp profile image
Theofanis Despoudis

My remarks:

  • You wrap everything with transaction.atomic(): I think it is too risky and too generic as a lot of things can go wrong in the code below. Consider making it more specific.

  • Why do you need a mixin while you can delegate notifications using a service or a signal? The request-response cycle should now be aware of actually doing something that can be delegated as a task that can happen in the future.

  • The SendNotificationMixin class it too big(huge I would say) and has too many methods. What if you needed to add another 100 types of notifications? You would need to refactor it again. The methods need to be re-engineered to resist that future change.