Skip to content

Conversation

@robert-smith-07
Copy link

@mikegee
Copy link
Member

mikegee commented Oct 17, 2017

I think this could be a bit more "object oriented". Consider the initializer taking the filename, preprocessing it to some degree, and a getter method to finish the work and retrieve the data you want.


# count all the words and add them to a hash
def countAllWords(cleanedText)
wordCount = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anytime you initialize a collection to then iteratively build it up is a clue to maybe use a method like .map or .reduce which are common in a lot of languages.

here i'd recommend using a method like .reduce or .each_with_object.
lines 37-46 could also be written:

cleaned_text.reduce(Hash.new(0)) { |hash,word| hash[word] += 1; hash }

or

cleaned_text.each_with_object(Hash.new(0)) { |word, hash| hash[word] += 1}

# parse text
def run(filename)

# read the text file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the pipelining. Also see Mike Gee's comment on sticking some of this stuff into the constructor.

require 'count_words'

describe CountWords do
it 'should be a Class' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

watch your spaces ;)

end

it 'should read a file that exists' do
countWords = CountWords.new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See betterspecs.org documentation on let
http://www.betterspecs.org/

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