search

bounswe / bounswe2015group4

Automatically exported from code.google.com/p/bounswe2015group4
4 stars 1 forks source link

Update request after code review #118

Closed erdemtor closed 8 years ago

erdemtor commented 8 years ago

Dear Oguz,

I have reviewed your commit. Firstly, for the next time I suggest you to commit partially. So that every commit could represent some particular task.(I think, I should do this as well.) Besides, there was literally zero documentation, no comment no explanation. Overally, your code is easily modified and understandable. As javascript- angular.js is not a object oriented and compiled language, most of the methods and implementations don't vary so easily. Anyways, you may find some of my views and cases I think needed to be enhanced and completely re-written if necessary.

1) Adding documentation 2) Errors must be explained to user at least in a reasonable manner in which they can understand something went wrong. Writing into the console is a good solution but for us. Not for the user, if there is a problem with database or some requests,then send an error message. 3)Very important issue: no input checking. 3.1 ) For example in event creation you must be synced with back end side. There is no input checking for tags( you know our assumption that lowercase and separated by commas). You cannot trust in the user, always check what it is given and warn them if necessary. 3.2) Same thing is valid for group creation. 4)Indentation is very good. I still suggest you to use some kind of spelling and indentation checker such as Eslint. In webstorm IDE there are settings and I can assist you to set it up if you want me. 5) In groups I noticed a small bug, it shows more than the number of members. (About backend as well, I now automatically add the owner as member, but still requires an update :) )

May the ease be with you :)
ghost commented 8 years ago

All points that you mentioned are crucial and I will fix them.

Thank you Erdem.

ghost commented 8 years ago

1) Documentation is started.

Other parts are also in progress and will be ready with milestone 3 presentation.

ghost commented 8 years ago

2) To show reasonable error messages to a user, I should get a response that contains error message. I am now getting exception that says, for example, there is a data violation in request body. Because I cannot know all data constraints, I cannot show reasonable error messages.

3) I am checking most of them but throwing exception with explanotary error message from backend for such controls is more important for both security and reliability.

4) I am know using that, thanks.

5) Fixed, deploy soon.