← Back to Upcase

Refactoring ApplicationHelper


(Aaron Mc Adam) #1

Hi guys, I was wondering if anyone had any tips on refactoring a largish ApplicationHelper - 121 LOC, with a 12 private methods - obviously lots of concerns! It’s not as straight forward to refactor as most things are, so I was hoping someone had experience of doing it, thanks!

Here’s a gist: https://gist.github.com/11377792


(Geoff Harcourt) #2

Nice short methods, so you’re off to a decent start. I try to use helpers as little as possible, preferring service objects and decorators/presenters, but you’re picking the right time to refactor here, as you’re not into junk drawer territory yet.

I think the first thing I’d try to do here is extract all the stuff that relies on tenant_configuration out into its own object. That will help you pull a large portion of the logic out of this class, and from there the next refactoring choices will be clearer.


(Aaron Mc Adam) #3

First of all, thanks a lot for looking!

I’ve taken a first stab at cleaning the helper up, extracting the dependent methods in #display_header? into a policy object called HeaderDisplayPolicy: https://gist.github.com/aaronmcadam/11394346#file-application_helper-rb-L27

As an aside, how do you test class methods? I don’t know if the way I do it is a bit clunky.

I’m trying to follow Sandi Metz’s rules about mocking outgoing command messages only, but if I don’t mock this outgoing query message and assert the result of calling it, it feels like I’m duplicating the test that’s in the PORO spec inside the spec of the client of it (in this case ApplicationHelper).

Also, in this case, the .call spec ( https://gist.github.com/aaronmcadam/11394346#file-header_display_policy_spec-rb-L4 ) is almost exactly the same as the #display_header? spec ( https://gist.github.com/aaronmcadam/11394346#file-application_helper_spec-rb-L5 ), which doesn’t sit well with me.

Thanks again!


(Tim Habermaas) #4

I would skip testing HeaderDisplayPolicy and think of it as an implementation detail to ApplicationHelper. The public interface your application uses is just the ApplicationHelper#display_header?, so I would test this as you did before and call it a day.

One might argue that ApplicationHelper isn’t fully isolated[1] anymore, but as long as the referenced constants aren’t doing anything evil (like having side effects, referencing ActiveRecord, …) I don’t think there’s any harm done. I don’t stub/mock value objects for the same reason. By not stubbing you also don’t have to fear your tests getting out of sync with the rest of the application. It’s also easier to refactor later.

btw. I don’t like your naming for HeaderDisplayPolicy#call. To me none of the names imply a boolean being returned. Either name it CanDisplayHeader#call or something like HeaderDisplayPolicy#display?.

[1] There’s also going a rumor around, that “fully isolated” doesn’t actually mean the objects under test, but the tests themselves. Haven’t looked into that, yet.


(Aaron Mc Adam) #5

Thanks for looking @timhabermaas!

I named the method #call because it’s a one-public-method object (http://www.rubytapas.com/episodes/35-Callable), but yeah, I’ll alias it to a boolean method too to make it clearer.

I actually want to end up removing the methods from ApplicationHelper, this is just me doing it in small steps. But the problem then becomes, where do I mixin the methods? Unless I add a public method to ApplicationController that instanciates the object in question or something. I’m not sure what the next step is with this, I want to SRP up my ApplicationController, but maybe extracting separate modules at least would help. What do you think?