search

christzenleonardy / messenger-bot

0 stars 0 forks source link

Submission review #1

Open ashwintiwari88 opened 4 years ago

ashwintiwari88 commented 4 years ago

[+] work on bot before chatbiz.id (but only 2 month) and also he wrote some LINE bot code [+] great commit [+] use linting and test [+] use variety tech (heroku, ngrok, postman doc) [+] use messenger bot built-in-nlp in here

[-] no structure, all in written single big file [-] ignore package-lock.json [-] inconsistent and strange way of use module.export / export here and here [-] use environment variable but don’t mention / push to create .env

ashwintiwari88 commented 4 years ago

@christzenleonardy here is review , any thoughts ?

christzenleonardy commented 4 years ago

Thank you for your review! Before I give my thoughts on your review, I want to thank you again for your time and for giving me a thorough review of my project. I will explain some reasons for the bad practices in my project.

About the project structure, I couldn't agree more that mine has no structure. I didn't plan carefully enough before I started the project and time were running out, so I decided to focus more on the unit tests because I feel that showing I could do unit tests would be better. After all, a project structure is a must thing in all company and I had the experience working on some companies. However, I regretted it because it was a bad choice as it's a bad practice that I don't want to repeat. In the end, it even affected my unit test implementations which further made me realize I really should not have done that.

As for ignoring package-lock.json, I knew the importance of package-lock.json because it allows us to avoid the general behavior of installing an updated minor version when someone run npm install. It was my bad... I will pay more attention to it and not repeat it.

About the inconsistent and strange way of use module.export is one of the trade-offs I chose to use, I knew it could produce some unwanted behavior before decided. However, it was the best choice I had last week as I had some other things to attend to during the week. I needed to do that to mock my function for unit tests. I knew I could have changed the project structure and isolated some functions to mock my unit tests. I'm truly sorry for not doing that and ended up using such a bad practice.

For the last point, I am sorry for not pushing the .env file. I was using windows for the development of the project and on windows, I needed to add it manually on windows environmental variables settings. As it was added to my system, I might have missed the .env file for the project. I will be more careful and pay more attention to it.

I will continue to learn and improve myself because that's just how important lifelong learning is. And the fastest way to improve is by going to the open sea and swam through it. I wish I can improve with your company and bring a positive impact on both of us. Thank you for your time. I am looking forward to hearing from you.

Sincerely, Christzen Leonardy Teknik Informatika Institut Teknologi Bandung On 15/03/2020 13:59:48, Ashwin Tiwari notifications@github.com wrote: @christzenleonardy [https://github.com/christzenleonardy] here is review , any thoughts ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [https://github.com/christzenleonardy/messenger-bot/issues/1#issuecomment-599175472], or unsubscribe [https://github.com/notifications/unsubscribe-auth/AJCXYFQTJBXPVVFMCXZVR2TRHR4GHANCNFSM4LKSEZWQ].