LouisaBarrett / runline-2

0 stars 2 forks source link

Refactor formatter #1

Closed breethomas closed 10 years ago

breethomas commented 10 years ago

@jayzes @mikepack Hi. I needed to regain sanity by getting back to green on tests and seeing the app run without errors. So for now, I put the presenter stuff on hold. Also, I left the Formatting class in our application_helper intact. Then, in parallel, I decided to peel off one method and break it up into several parts and see if I could get it working all the way through to the view. Not sure if this is the best approach, but thus far, it feels like good practice in getting used to how things are being juggled around. :)

I took the "total distance" method from a user's aggregate runs, which is needed on the dashboard show view and broke it down like this: (my commits from Jan 31 - Feb 1)

I have specs for the three models (which probably need some work), but admittedly, did not write a spec for the controller. If this approach seems viable for Louisa and I to follow through with the other methods like pace, longest run, etc - then we'll definitely make controller tests. (or feature tests that use the controller? I don't know, I hear people say you shouldn't unit test your controllers - but maybe for noobs, it is good practice?)

Let me know what you think. We just need to make some progress and I think in the beginning, we were trying to change too much at once. And since the current calculator of the app (run_stat_calculator) is full of class methods that get passed a user on each one, the refactor of that piece was overwhelming. It had formatting mixed into it as well, which was messy. Creating separate pieces in the app that do the same thing, and are several little objects that build on one another, feels easier to me right now. But let me know if I am potentially making a bigger mess! It might be the long way around, but it registers in my head a little better this way...

(apologies for the novel)

mikepack commented 10 years ago

Overall, I think this is a positive refactor and step in the right direction. I definitely like the work that's been done here as it breaks up responsibilities a bit better. I think the boundaries between those responsibilities could be further refined but I don't think that's necessary right now. I'll make some further comments inline.