search

prasanth94 / Amortization-schedule

Rails application to generate an amortization schedule
1 stars 0 forks source link

Including libraries in controller is not the recommended way of doing things - Can be done in a better way #4

Open JAYKRISHNAN opened 7 years ago

prasanth94 commented 7 years ago

I have fixed this issue in the commit - https://github.com/prasanth94/Amortization-schedule/commit/08c7c20d1cd6265aef2205284918595318565b6f

Let me know if any further improvements have to be made.

m-arav commented 7 years ago

Hi Prasanth,

Can you take a look at this : https://www.viget.com/articles/slimming-down-your-models-and-controllers , and relook at this issue.

If you have any questions regarding the issue feel free to reach out to us.

prasanth94 commented 7 years ago

Thanks for sharing the blog, It was really informative. I got the idea, for the first change I made on this issue, from 'fat model skinny controller concept'. I came to know about service class concept from this blog and have implemented the same in this commit - https://github.com/prasanth94/Amortization-schedule/commit/f6ea43accaba1b6ea7c6ff06debc817c1b9888b3.

Let me know your thoughts.

m-arav commented 7 years ago

Good to see you have implemented service objects. I have one question though, Why did you choose to go with the module approach for amortisation_schedule_generation and use lib/ to store the code ?

prasanth94 commented 7 years ago

Initially I was not aware of the service object concept and also it was clear to me that the amortisation_schedule_generation does not belong to the model, controller and helpers. Hence I chose to go with the module approach and also I got to know that the best way to integrate a module to my application is to store it in the lib/ directory. Initially I made the mistake of having my lib/ code model dependent, I fixed that issue when you pointed it out. Since the current implementation of amortisation_schedule_generation in the lib/ is in a generic way , I made the choice of not moving it out from lib/ .

Hope this answers your question.

m-arav commented 7 years ago

Yes this answers my question.