← Back to Upcase

Question about Shakespeare Analyzer


(Yang Chung) #1

Hi,

I just completed the challenge and made a pull request. Looking at other pull requests, though, it looks like I was the only one who used one class, instead of splitting it up to multiple classes.

I thought the task was simple enough to write a simple class: 36 lines with four methods and each taking less than 5 lines. I initially had two classes, but it felt like making simple things more complicated.

Was my approach completely wrong? Can someone please review my code and leave some comments? What would be the sure sign (or smell) that the class should be broken up?

My pull request: https://github.com/thoughtbot/shakespeare_analyzer/pull/19/files

Thank you in advance!


(Charlieanna) #2

Read POODR book and try understanding what SRP is about and try to use SRP principle in it and if possible other patterns. Even I did the exercise long back but no one commented on mine.


(Charlieanna) #3

Have a look at this.


(Matthew Sumner) #4

Your approach doesn’t seem unreasonable. I notice that everything is public, do you really want other developers to use every method? It seems like we should only use parse_speeches and print_result. I would make all the other methods private.

Otherwise that looks like a clean solution. Well done!