Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Feb 13, 2016

I chose to create a static helper class to perform the actual counting. Its count method accepts a file as an argument and will raise a custom exception if it's absent.

P.S., that bit of French was a nice twist.

@ghost
Copy link
Author

ghost commented Feb 13, 2016

I realized after PR'ing that I forgot to sort the hash; it's fixed in the last commit.

@sjreich
Copy link

sjreich commented Feb 13, 2016

Looks good. I like that you're putting some real thought into how to structure this whole thing. When you're doing so, I'd focus your attention on separating things by their function. For instance, #self.to_word_array does both reading and parsing: those are two pretty different sorts of operations; they should probably be split into different methods. At the same time, you've got the reading function split into two different places - one to open the file, and another to read it; those should probably go together. Also, check out File.read in the docs.

word_counter.rb Outdated
def self.count file
raise 'WordCounter: no file given' unless file
# use inject to create a hash of word counts
to_word_array(file).inject({}) { |h,w| h[w] ? h[w] += 1 : h[w] = 1; h }
Copy link

Choose a reason for hiding this comment

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

This is a bit hard to follow with all these methods chained together - maybe split them out? Also, more illustrative variable names (than h and v) will improve clarity.

Copy link
Author

Choose a reason for hiding this comment

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

I purposefully chose to chain method calls to avoid the cost of unnecessary variable assignment. This is a practice that is encouraged by the developers on my current team, to avoid assigning a variable you will immediately discard.

I do agree that more verbose loop variable naming makes for more readable code though, and that I should break my two static methods into more fine-grained methods.

Copy link

Choose a reason for hiding this comment

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

I hear you. But multi-line chaining is generally frowned upon for readability reasons. The solution is to turn the complicated parts into separate methods; then you can make a nice, brief chain that includes that method.

Copy link
Member

Choose a reason for hiding this comment

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

A couple tips for making this more readable:

  • Hashs can be instantiated with a default value. (0 would be good choice here.)
  • If you switch from inject to each_with_object, you don't need to return the hash at the end of the block.

@ghost
Copy link
Author

ghost commented Feb 14, 2016

@mikegee and @sjreich thanks for the feedback. I honestly forgot that you could instantiate a hash with a default value, I'll definitely reactor to include that change. I was also unaware of each_with_object, I'll check it out.

@sjreich I also completely agree that my class methods can (and should) be broken into smaller methods which perform exactly one task. For example, you mentioned one method for reading and the other for parsing; that's a good practice with which I agree. I'll make these changes.

@ghost
Copy link
Author

ghost commented Feb 14, 2016

When I began breaking my two static methods into smaller methods, I realized that a non-static class was the right way to solve the problem (in my opinion). I transformed my static methods into instance methods, but designed the WordCounter class so that one instance could be used to count the words of multiple files, instead of having a single instance of WordCounter locked to a single file forever.

I also chose to use each_with_object over my previous call to inject, which is definitely a much more elegant solution, thanks @mikegee.

Overall great exercise, and I appreciate the feedback. Discussing different approaches is very insightful.

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.

2 participants