Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use transform class to extract away Proc/Klass logic #3

Closed
wants to merge 2 commits into from
Closed

Use transform class to extract away Proc/Klass logic #3

wants to merge 2 commits into from

Conversation

ollie
Copy link
Contributor

@ollie ollie commented Apr 10, 2015

Hi,

I'm sorry but I couldn't resist it. I wrapped your transforms into another class so that it exposes the same interface for class or proc, which has the benefit of removing the if/else logic from control and runner files. I also extracted away the transform application into a separate method so that it looks a bit cleaner.

Feel free to use or discard this entirely, I was just curious if I could improve it and if so would like to share the result. :-)

Ollie

@thbar
Copy link
Owner

thbar commented Apr 10, 2015

No problem! Thanks for sharing 😄

I have been thinking about this, but wanted to take the time to think about how to abstract without having an extra indirection cost for the block form. As well, I'm considering writing a "runner" that will actually unroll the code completely, using some method generation.

Thanks for your code, this gives me food for thought nonetheless! I will digest all this and do some benchmarks (with large processing) to make an informed decision here.

@ollie
Copy link
Contributor Author

ollie commented Apr 20, 2015

Hello, should I close this? Or not yet? :-)

@thbar
Copy link
Owner

thbar commented Apr 20, 2015

No, please keep this opened (unless this bugs you, I can recreate separate issues)! This acts as a reminder for me to apply the whitespaces fixes to the whole codebase, and to think about the wrapper aspect. Thanks!

@ollie
Copy link
Contributor Author

ollie commented Apr 20, 2015

Okey, no problem. :)

@ollie
Copy link
Contributor Author

ollie commented Apr 20, 2015

I don't know if you know Rubocop or not, so please forgive me if you do, but if you don't, I strongly recommend it, it's a great helper (but do not follow it blindly). Here are two rake tasks that I often use to check things in one go: https://github.com/ollie/after_ship/blob/master/Rakefile#L3-L13, feel free to steal 'em if you are so inclined. :-)

@thbar
Copy link
Owner

thbar commented Apr 20, 2015

I know about Rubocop but haven't started using it here! (I'm using CodeClimate though). I will most likely start using it soon though. Thanks for your rake tasks, I'll use them as inspiration!

@thbar
Copy link
Owner

thbar commented Jun 21, 2015

Closing, a similar refactoring is ongoing in #14. Thanks!

@thbar thbar closed this Jun 21, 2015
thbar added a commit that referenced this pull request Jun 21, 2015
Hat-tip to @ollie for pushing for this (see #3).
@ollie
Copy link
Contributor Author

ollie commented Jun 22, 2015

No problem man! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants