-
Notifications
You must be signed in to change notification settings - Fork 0
/
refactoring-this-flask-view.html
165 lines (140 loc) · 15.7 KB
/
refactoring-this-flask-view.html
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<title>Refactoring this Flask View</title>
<link rel="stylesheet" href="./theme/css/main.css" />
</head>
<body id="index" class="home">
<header id="banner" class="body">
<h1><a href="./">The Train Station</a></h1>
<nav><ul>
<li class="active"><a href="./category/development.html">Development</a></li>
<li><a href="./category/general.html">General</a></li>
</ul></nav>
</header><!-- /#banner -->
<section id="content" class="body">
<article>
<header>
<h1 class="entry-title">
<a href="./refactoring-this-flask-view.html" rel="bookmark"
title="Permalink to Refactoring this Flask View">Refactoring this Flask View</a></h1>
</header>
<div class="entry-content">
<footer class="post-info">
<abbr class="published" title="2019-06-15T12:00:00-05:00">
Published: Sat 15 June 2019
</abbr>
<address class="vcard author">
By <a class="url fn" href="./author/seth-buntin.html">Seth Buntin</a>
</address>
<p>In <a href="./category/development.html">Development</a>.</p>
<p>tags: <a href="./tag/python.html">python</a> <a href="./tag/refactoring.html">refactoring</a> <a href="./tag/design.html">design</a> </p>
</footer><!-- /.post-info --> <p>Recently, <a href="http://myemma.com">at my day job</a>, I ran across a situation that I wondered if I could make better. <em>"Better"</em> to a software
engineer is definitely subjective. Hopefully I make the case as to why I think it is better.</p>
<h2>Change</h2>
<p>If there is one thing I've learned as a software engineer it is that change is inevitable. There are <a href="https://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052">smart</a>
<a href="https://www.youtube.com/watch?v=rI8tNMsozo0">people</a> who have made it a part of their books and conference talks. So my main focus
is not truly to implement some design pattern as much as it is to get a handle on dependencies and optimize for the <em>potential</em> ability
to change. <em>Potential</em> is important here. My changes could be a waste of time, I don't think they are as you will see but, there is
always room for "good enough" and move on.</p>
<p><em>Note</em> - While I don't go into the details of actually refactoring this view I did have to ensure that this functionality was
tested so that I had the confidence to be able to <a href="https://twitter.com/kentbeck/status/250733358307500032?lang=en">make the simple change</a>.</p>
<h2>The view</h2>
<div class="highlight"><pre><span></span><code><span class="nd">@blueprint</span><span class="o">.</span><span class="n">route</span><span class="p">(</span><span class="s2">""</span><span class="p">,</span> <span class="n">methods</span><span class="o">=</span><span class="p">[</span><span class="s2">"POST"</span><span class="p">])</span>
<span class="k">def</span> <span class="nf">create_preferences</span><span class="p">(</span><span class="n">account_id</span><span class="p">):</span>
<span class="c1"># ... some general setup</span>
<span class="n">subscriptions</span> <span class="o">=</span> <span class="n">payload</span><span class="p">[</span><span class="s2">"subscriptions"</span><span class="p">]</span>
<span class="n">optouts</span> <span class="o">=</span> <span class="n">payload</span><span class="p">[</span><span class="s2">"optouts"</span><span class="p">]</span>
<span class="c1"># ... the core functionality</span>
<span class="k">for</span> <span class="n">subscription_id</span> <span class="ow">in</span> <span class="n">subscriptions</span><span class="p">:</span>
<span class="k">try</span><span class="p">:</span>
<span class="n">message</span> <span class="o">=</span> <span class="n">make_member_subscribed</span><span class="p">(</span><span class="n">account_id</span><span class="p">,</span> <span class="n">member_id</span><span class="p">,</span> <span class="n">subscription_id</span><span class="p">)</span>
<span class="n">subscriptions_topic</span><span class="o">.</span><span class="n">send</span><span class="p">(</span><span class="n">message</span><span class="p">)</span>
<span class="k">except</span> <span class="ne">Exception</span> <span class="k">as</span> <span class="n">exc</span><span class="p">:</span>
<span class="n">failure</span> <span class="o">=</span> <span class="s2">"Failed to send subscription message for member </span><span class="si">%s</span><span class="s2">: </span><span class="si">%s</span><span class="s2">"</span> <span class="o">%</span> <span class="p">(</span><span class="n">member_id</span><span class="p">,</span> <span class="n">exc</span><span class="p">)</span>
<span class="n">logger</span><span class="o">.</span><span class="n">warning</span><span class="p">(</span><span class="n">failure</span><span class="p">)</span>
<span class="k">for</span> <span class="n">subscription_id</span> <span class="ow">in</span> <span class="n">optouts</span><span class="p">:</span>
<span class="k">try</span><span class="p">:</span>
<span class="n">message</span> <span class="o">=</span> <span class="n">make_member_opted_out</span><span class="p">(</span><span class="n">account_id</span><span class="p">,</span> <span class="n">member_id</span><span class="p">,</span> <span class="n">subscription_id</span><span class="p">)</span>
<span class="n">subscriptions_topic</span><span class="o">.</span><span class="n">send</span><span class="p">(</span><span class="n">message</span><span class="p">)</span>
<span class="k">except</span> <span class="ne">Exception</span> <span class="k">as</span> <span class="n">exc</span><span class="p">:</span>
<span class="n">failure</span> <span class="o">=</span> <span class="s2">"Failed to send optout message for member </span><span class="si">%s</span><span class="s2">: </span><span class="si">%s</span><span class="s2">"</span> <span class="o">%</span> <span class="p">(</span><span class="n">member_id</span><span class="p">,</span> <span class="n">exc</span><span class="p">)</span>
<span class="n">logger</span><span class="o">.</span><span class="n">warning</span><span class="p">(</span><span class="n">failure</span><span class="p">)</span>
<span class="c1"># ... return successful response</span>
</code></pre></div>
<p>This view function is already quite long but my main focus was to be able to test this view without having to test the message
dispatching, which caused me to think <em>"maybe this is doing too much"</em>.</p>
<h2>The Dispatcher</h2>
<p>The first step I took was to pull out this functionality into a class. I wanted to try and keep this consistent with the <a href="https://en.wikipedia.org/wiki/Single_responsibility_principle">Single
Responsibility principle</a> and, as <a href="https://www.deconstructconf.com/2018/sandi-metz-polly-want-a-message">Sandi Metz</a> teaches,
be able to explain it's role with one sentence.</p>
<div class="highlight"><pre><span></span><code><span class="k">class</span> <span class="nc">Dispatcher</span><span class="p">(</span><span class="nb">object</span><span class="p">):</span>
<span class="k">def</span> <span class="fm">__init__</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> <span class="n">pipe</span><span class="p">,</span> <span class="n">logger</span><span class="p">):</span>
<span class="bp">self</span><span class="o">.</span><span class="n">pipe</span> <span class="o">=</span> <span class="n">pipe</span>
<span class="bp">self</span><span class="o">.</span><span class="n">logger</span> <span class="o">=</span> <span class="n">logger</span>
<span class="k">def</span> <span class="nf">send</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> <span class="n">data</span><span class="p">):</span>
<span class="k">raise</span> <span class="ne">NotImplementedError</span><span class="p">(</span><span class="s2">"dispatchers must implement send"</span><span class="p">)</span>
</code></pre></div>
<p>I know <em>pipe</em> isn't potentially the best name but it is good enough for now. This "interface" class allowed me to begin my core
implementation for the functionality in the view.</p>
<div class="highlight"><pre><span></span><code><span class="k">class</span> <span class="nc">MemberSubscribedDispatcher</span><span class="p">(</span><span class="n">Dispatcher</span><span class="p">):</span>
<span class="k">def</span> <span class="nf">send</span><span class="p">(</span><span class="bp">self</span><span class="p">,</span> <span class="n">data</span><span class="p">):</span>
<span class="k">try</span><span class="p">:</span>
<span class="bp">self</span><span class="o">.</span><span class="n">pipe</span><span class="o">.</span><span class="n">send</span><span class="p">({</span>
<span class="s2">"type"</span><span class="p">:</span> <span class="s2">"subscriptions-member-subscribed"</span><span class="p">,</span>
<span class="s2">"account_id"</span><span class="p">:</span> <span class="n">data</span><span class="p">[</span><span class="s2">"account_id"</span><span class="p">],</span>
<span class="s2">"member_id"</span><span class="p">:</span> <span class="n">data</span><span class="p">[</span><span class="s2">"member_id"</span><span class="p">],</span>
<span class="s2">"subscription_id"</span><span class="p">:</span> <span class="n">data</span><span class="p">[</span><span class="s2">"subscription_id"</span><span class="p">],</span>
<span class="p">})</span>
<span class="k">except</span> <span class="ne">Exception</span> <span class="k">as</span> <span class="n">exc</span><span class="p">:</span>
<span class="n">failure</span> <span class="o">=</span> <span class="s2">"Failed to send subscribed message for member </span><span class="si">%s</span><span class="s2">: </span><span class="si">%s</span><span class="s2">"</span> <span class="o">%</span> <span class="p">(</span><span class="n">data</span><span class="p">[</span><span class="s2">"member_id"</span><span class="p">],</span> <span class="n">exc</span><span class="p">)</span>
<span class="bp">self</span><span class="o">.</span><span class="n">logger</span><span class="o">.</span><span class="n">warning</span><span class="p">(</span><span class="n">failure</span><span class="p">)</span>
</code></pre></div>
<p>By <a href="https://en.wikipedia.org/wiki/Dependency_injection">injecting</a> the <em>pipe</em> and <em>logger</em> above I now have a class that is only
dependent on the objects passed to it. I have also defined the contract for how dispatching works. As long as the <em>pipe</em> implements
<code>send</code> and the <em>logger</em> implements <code>warning</code> it can successfully be injecting into this implementation. I have also remove the
factory function that generated the message format previously and brought that message format closer to the behavior.</p>
<p>I submit that this class is quite simple and does just one thing (two if you count handling the error of a pipe),
<em>"dispatch a member subscribed message to a given pipe"</em>.</p>
<p>This view functionality can now change. We could change the AWS topic we are using to be a Kinesis Firehose as long as we implement
the functionality in a manner that meets this "interface".</p>
<h2>The refactored view</h2>
<p>After all that, here is where we landed.</p>
<div class="highlight"><pre><span></span><code><span class="nd">@blueprint</span><span class="o">.</span><span class="n">route</span><span class="p">(</span><span class="s2">""</span><span class="p">,</span> <span class="n">methods</span><span class="o">=</span><span class="p">[</span><span class="s2">"POST"</span><span class="p">])</span>
<span class="k">def</span> <span class="nf">create_preferences</span><span class="p">(</span><span class="n">account_id</span><span class="p">):</span>
<span class="c1"># ... some general setup</span>
<span class="n">subscriptions</span> <span class="o">=</span> <span class="n">payload</span><span class="p">[</span><span class="s2">"subscriptions"</span><span class="p">]</span>
<span class="n">optouts</span> <span class="o">=</span> <span class="n">payload</span><span class="p">[</span><span class="s2">"optouts"</span><span class="p">]</span>
<span class="c1"># ... the core functionality</span>
<span class="n">subscribed_dispatcher</span> <span class="o">=</span> <span class="n">MemberSubscribedDispatcher</span><span class="p">(</span><span class="n">subscription_topic</span><span class="p">,</span> <span class="n">logger</span><span class="p">)</span>
<span class="k">for</span> <span class="n">subscription_id</span> <span class="ow">in</span> <span class="n">subscriptions</span><span class="p">:</span>
<span class="n">subscribed_dispatcher</span><span class="o">.</span><span class="n">send</span><span class="p">({</span><span class="s2">"account_id"</span><span class="p">:</span> <span class="n">account_id</span><span class="p">,</span>
<span class="s2">"member_id"</span><span class="p">:</span> <span class="n">member_id</span><span class="p">,</span>
<span class="s2">"subscription_id"</span><span class="p">:</span> <span class="n">subscription_id</span><span class="p">})</span>
<span class="k">for</span> <span class="n">subscription_id</span> <span class="ow">in</span> <span class="n">optouts</span><span class="p">:</span>
<span class="c1"># ... MemberOptOutDispatcher</span>
<span class="c1"># ... return successful response</span>
</code></pre></div>
<p>This view is now easier to test. If I feel so inclined I could test that these functions are called within the test. All the
functionality is tested in the <code>MemberSubscribedDispatcherTestCase</code> that I implemented as a part of this refactor so I only focused
on ensuring that these dispatchers actually dispatched the amount of times I expected given the rest of the view functionality.</p>
<h2>Better</h2>
<p>So why is this better? I propose that this implementation is better for a couple of reasons. </p>
<ul>
<li>I have pulled the implementation into a class that is only dependent on what is sent to it. We could go so far as to change how we log and still not have to change this implementation. Sure, we might have to implement some form of adapter if we ever decided to do that but I'm okay with that.</li>
<li>It is now easier to test this functionality. I no longer have to depend on mocking a request or anything like that. I definitely could get this benefit from a simple function. I just chose not to go that direction here.</li>
</ul>
</div><!-- /.entry-content -->
</article>
</section>
<section id="extras" class="body">
</section><!-- /#extras -->
<footer id="contentinfo" class="body">
<address id="about" class="vcard body">
Proudly powered by <a href="http://getpelican.com/">Pelican</a>, which takes great advantage of <a href="http://python.org">Python</a>.
</address><!-- /#about -->
<p>The theme is by <a href="http://coding.smashingmagazine.com/2009/08/04/designing-a-html-5-layout-from-scratch/">Smashing Magazine</a>, thanks!</p>
</footer><!-- /#contentinfo -->
</body>
</html>