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

Make parser run idiomatic ruby #204

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thatrubylove
Copy link

Love the project!

I would like to do a bunch of refactoring and test cleanup to make this more idiomatic. What do you guys think?

I will likely be making blog posts about the changes and possibly screencasts. There is some very nice code here, it just isn't very 'ruby'

Used the extract method pattern to pull out 2 new helper methods

2 new tests in parser to test the methods I extracted. The tests
        were written against the original code before I made the
        refactor.

* removal of _ prefix on local variables
* #run is now shorter and more expressive
* opt for reduce over each instead of declaring a local collection
        and then 'stuffing' it on each iteration
* opt for an inline hash and declarations
@nhmood
Copy link
Owner

nhmood commented Dec 4, 2013

I'm all for refactoring and learning how to do things the proper Ruby way so this sounds good.
@alpaca-tc has also submitted some refactoring pull requests (#118) and has also suggested a refactor branch where these changes could be submitted (#127). I'm not sure how but I'll have to figure out a way to manage two separate refactorings going on at once.

I am definitely interested in the blog posts / screencast idea though, I think that would be a great resource not only for people but me as well :)

I have some functionality changes I'd like to get in and then I will revisit this refactoring issue.

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.

3 participants