Photo by Rajesh Appalla on Unsplash
After working with the same code base for a while, there comes a time when a developer starts noticing ways th...
For further actions, you may consider blocking this person and/or reporting abuse
Hey Gabi! It's cool to see you posting on Dev.to!
Thank you, Andrew! Good to see you on here 👋🏻
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
SendNotificationMixinclass 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.Is there a reason for SendNotificationMixin to be a class or a mixin? It's methods do not use 'self' at all (except
self.send_email, and that should just be a function call). Classes which do not have any state, and do not useself, are just namespaces for functions, except they are harder to use (e.g. you have to instantiate them or inherit from them etc.). Python normally uses modules for namespaces, not classes. youtube.com/watch?v=o9pEzgHorH0I still think there's a major issue with your code, The view code is
too coupled with the notification functionality. You shouldn't be using
a Mixin (at least in the context of a view). A view's major purpose is
to receive a request and return a response.
Also, the
SendNotificationMixinclass is too huge, if you use aservice like codeclimate it should flag it
as a "Code smell".
Shameless plug here: I created a django library that might be of help it uses django signals to decouple the notification functionality from your application. The various types of notifications can easily be written as "delivery channels" instead of mixins.
It also processes notifications asynchronously using
Celerywhich IMO is better thandjango-channelsbecause you can monitor tasks, retry them if they fail etc.github.com/danidee10/django-notifs