Dear Rubyists,
I just lost a contract because of my code in a Rails project.
The specific code in question is related to a "posting a comment" feature. Here are the details:
In this project, "posting a comment" does not simply entail inserting a row into the database. It involves a procedure to yes, insert a row, but also detect its language, check for spam, send emails, and "share" it to Twitter and Facebook. I believe this algorithm should be encapsulated. I do not believe it belongs in a controller or a model. I do not believe Active Record callbacks should be used.
The "senior developer", whom is the stake holder's right hand man, said this:
All that's being accomplished is moving a few statements from the controller to the a new class and adding even more code to support the new construct. We don't have the problems that this would solve like comments being posted from multiple controllers.
What do you guys think? Am I doing it wrong?
Please leave a comment, good or bad. My motivation to continue programming is at an all time low.
Thank you.
@justinko But that approach couples
LanguageDetector
with an object with that requirements for no good reason. You want to guess the language of the text represented by that string. So, take just the string. If you don't need baggage, travel light. :PThis has other benefits. For example, what if you had an object were two of it's attributes where to be checked for language?
LanguageDetector
would need to be informed of the fields.Something as simple as this:
would need to alter
LanguageDetector
's implementation to support that kind of logic. So, a true design nazi could argue that it's an SRP violation. ExceptPostToFacebook
(would have to read it to see what the argument's for), I could say this for any other class you're using.However, do note this is tuning a very fine point. You already have the responsibilities reasonably well separated. Using @avdi's terms, changing this would be refactoring, not refurbishing.