UdacityMobileWebScholarship / guess-quote

This application is a collaborative project made by the Google Udacity Mobile Web Specialist Scholars.
MIT License
22 stars 48 forks source link

Develop Branch: Adding question model #40

Closed anurag-majumdar closed 6 years ago

anurag-majumdar commented 6 years ago

The initial question model has been setup. The model has been tested and is working fine.

anurag-majumdar commented 6 years ago

Made the respective changes in model. Please review.

anurag-majumdar commented 6 years ago

@twishasaraiya @skyshader @danivijay This is how the questions collection looks after a question has been submitted to it:

image

twishasaraiya commented 6 years ago

Great @anurag-majumdar Looks good to me. Since all PRs will now be reviewed by at least 2 mods lets wait till another moderator approves it. Once approved the moderator will merge it.

skyshader commented 6 years ago

@anurag-majumdar let's keep the answer as id of the options instead of string. Better to match and fetch.

EDIT: Just saw that options are array of strings. Would recommend to keep options as a separate schema (and not create a model) and simply link here to the question schema like so:

options: {
    type: [OptionSchema],
    required: true,
  },

and OptionSchema would by default have a mongoose generated id. Doing this will make querying much easier as the options will still be in same document but we would have id's for every option we have. Otherwise we have to do text comparisons.

What are your thoughts @twishasaraiya and @anurag-majumdar ?

anurag-majumdar commented 6 years ago

@skyshader Great suggestion. But I want to add that since the size of each document will not exceed 16MB with the string values and other data we have, it won't be necessary to create a separate schema for options. In the UI, the question will be directly fetched along with the correct answer and random options from the REST API. This will keep the model simple and easy to query too. I don't think we are going to do separate work on the options with each id, so its better we keep it as Array of strings instead of doing extra work.

If there are any changes required in schema later on, that can be fixed. But for this model, I am pretty sure its final. @skyshader @twishasaraiya @ashwani99 @danivijay Let me know your thoughts on this.

ashwani99 commented 6 years ago

I agree with @anurag-majumdar Creating a schema for options is an overhead IMO. We will still be comparing ids in place of strings if we do this, which has no improvements to the current model.

skyshader commented 6 years ago

So, just to make it clear. We are still using the same document of question and not creating a separate document for options. So 16MB thing is not applicable here because options are still part of that document as a sub document.You will be able to query them like you can query them now. Read more about them here: http://mongoosejs.com/docs/subdocs.html

The only advantage we are getting by going with sub documents is that we get identifiers for each options that we don't have to manage separately. And we can reference the option by identifier in answer instead of having value of option as answer.

I'm kind of against matching strings to get the results as lots of things can go wrong (spelling mistakes, ignoring cases, updating an answer option will make it mandatory to update the answer itself and more).

I'm also okay if we put a custom identifier instead of array for storing options, so we can reference using that.

I hope I am not confusing any one. @anurag-majumdar and @ashwani99 Do let me know your thoughts. If you feel we can manage with String comparisons then we can go ahead with this PR.

anurag-majumdar commented 6 years ago

I think lets have a vote, @twishasaraiya @danivijay @ashwani99 @skyshader , if changes are required will do them. That won't be a problem. But let's do this fast. Let's merge the PR by tonight and not wait till tomorrow. There are many other issues which need to be opened and we should move ahead quickly. 😃

ashwani99 commented 6 years ago

Let's keep things simple, we can always make changes in future if needed. I think this PR is good to merge. Let the moderators decide the final decision

skyshader commented 6 years ago

Let's keep this voting thing aside. Though this application is of small scale this won't matter much. My only concern is that people are submitting PRs for following better practices whether it's html/css or it's how to manage components properly, I wanted this to also be aligned to that standard.

But yeah, for now we can merge it. If someone submits a PR for a better model which avoids string comparison for answers, we can merge that too later.

anurag-majumdar commented 6 years ago

@skyshader I can understand. Its really helpful. But sometimes best practices also tend to be keeping it simple. It totally depends on the use case of the application. Thanks for providing the inputs bro. 😃