Skip to content


Subversion checkout URL

You can clone with
Download ZIP
100644 87 lines (74 sloc) 3.5 KB
1af85b9 @steveklabnik Adding another refactoring note, thanks to @ryanb
1 ---
2 layout: post
79a145d @steveklabnik Derp. Fix typo in title.
3 title: "More Refactoring"
1af85b9 @steveklabnik Adding another refactoring note, thanks to @ryanb
4 date: 2011-09-23 17:03
5 comments: true
6 categories:
7 ---
9 Hey everyone! I just wanted to share One More Thing with you about this
10 refactoring.
12 The main thrust of the last article I posted was to show you a technique for
13 extracting a class, getting it under some kind of test, and then refactoring it
14 a bit. Refactoring is always an iterative process, and Ryan Bates from the
15 always awesome [Railscasts]( asked me why I made the
16 Salmon class into a module, especially given my recent [rant](/2011/09/06/the-secret-to-rails-oo-design.html)
17 against modules. The answer was, 'because it's simpler, and the first thing
18 I thought of.' He shared with me an alternate implementation that I like too,
19 and I wanted to share with you. Check it:
c45b3df @steveklabnik derp
21 ``` ruby
1af85b9 @steveklabnik Adding another refactoring note, thanks to @ryanb
22 class SalmonNotifier
23 def initialize(user, feed)
24 @user = user
25 @feed = feed
26 end
28 def mention(update)
29 deliver"http://#{}/"))
30 end
32 def follow
33 deliver OStatus::Salmon.from_follow(,
34 end
36 def unfollow
37 deliver OStatus::Salmon.from_unfollow(,
38 end
40 protected
42 def deliver(salmon)
43 send_envelope_to_salmon_endpoint salmon.to_xml(@user.to_rsa_keypair)
44 end
46 def send_envelope_to_salmon_endpoint(envelope)
47 uri = URI.parse(
48 http =, uri.port)
49, envelope, {"Content-Type" => "application/magic-envelope+xml"})
50 end
51 end
52 ```
54 This follows a more OO style, and is a bit more well-factored. Here's his
55 description of what he did:
ba888f5 @steveklabnik {% blockquote %} => >
57 > 1. Switched to a class and gave it a name that is a noun (maybe name
58 > should be different though).
59 > 2. Moved common method params into instance variables.
60 > 3. This simplified the mention/follow/unfollow methods enough to be on one line.
61 > 4. Renamed "send_to_salmon_endpoint" to "deliver" because it feels
62 > nicer, the "salmon endpoint" can be assumed due to the name of the
63 > class. I generally don't like to put the class name in the method
64 > name.
65 > 5. Extracted commented out into its own method with the same name. I
66 > don't know if this is really necessary though (same reason as above).
67 >
68 > The only thing that bothers me now is the constant access of
69 > and This makes me think it should be
70 > interacting directly with authors. However you still need access to
71 > @user.to_rsa_keypair but maybe that method could be moved elsewhere
72 > more accessible.
1af85b9 @steveklabnik Adding another refactoring note, thanks to @ryanb
74 All of these changes are pretty good. Ryan's suggestions for moving forward are
75 pretty good, as well, but I'd add one more thing: we're starting to collect a
76 bunch of methods all related to delivering more generic HTTP requests in the
77 protected section. This also might be worth pulling out, too. Protected/private
78 in general is a minor smell that indicates there's code being used multiple
79 times that's not part of the main objective of the class. It might not be worth
80 it yet, but then again, it might. Speaking of protected methods, you might
81 wonder why I ended up mocking out my protected method in the last post, as well.
82 There are no more tests that actually test the method is working properly. This
83 is true, and it's because I was leading into thinking of something like this.
84 Mocked tests are almost always unit tests, and two methods are two separate
85 units, so I had just ended up doing it out of habit. This further shows that
86 maybe an extraction of another class is worthwhile.
Something went wrong with that request. Please try again.