lbondaryk/CorrectnessEngine — pull request #2 by seannives (2013-10-18 18:05:25+00:00)
Title: 553 multchoice object

@ysahnpark and @mlippert ...please code review when you have a chance. Here are some things to ponder:

* It was suggested that instead of...
var CE = require('./ce').EngineHandler;
this.ce = new CE;
we do...
var CE = require('./ce');
this.ce = new CE.EngineHandler();

This allows you to export multiple objects/functions from the same file if you want and call them with a single require.

In lib/ce.js:

* rather than enum our various handlers I'm loading them in at the top of this file. An alternative would be to load them in as-needed but I think that may be less performant. An another alternative is rather than listing them in the top of ce.js we can just put an index.js file in /lib/types/ that contains the list. I'm not sure it buys us much.

* I started down the road Mike suggested, loading assessment handlers like he did dynamicValueHandlers in the bricLayer. You'll see that commented out near the botton of the file. Later I read that currying isn't favored in node, although there wasn't a cogent argument supplied.
I also read that it's suggested that modules should either return a simple function or an object that you 'new'...those two approaches seem to be favored in the node community (https://groups.google.com/forum/#!topic/nodejs/xDuZOYyzUA0). While Multiple Choice is relatively straightforward and could certainly be handled as a simple function I think the object creation approach may be more flexible in the long run for more complicated types.

In lib/types/multiplechoice.js:

* I ran into trouble with the way we're keying answers for multiple choice in the context of trying to write a schema for it. Joi validation completely fell down in this regard. I don't know if JSON schema through z-schema will be any better but it can't be worse.

The decision here was to NOT devote any serious time to schema validation of the answer and submission data coming in. As a result, I've got a minimal implementation here and incomplete schemas.

* I had copied validateObj_ from IPS's sequencenodeprovider.validateMessage_ and found that validateMessage_ didn't seem to be working as I expected. Young-Suk, you may just want to double-check that validateMessage_ is working as you want. I'm not sure why there would be a difference needed between the two implementations.

* the assess method I tried to make completely generic so we could use it as a template for other assessment types going forward. I tried to put all the multiplechoice-specific stuff in the private functions assess calls.

Still to do:
* Put in a config flag to allow us to ignore the schema validation in multiplechoice.js
* integration/full-path test, using supertest to fake a post to the correctness engine and return the expected results
* roll my mock data into a mocks file and get it out of ce and multiplechoice tests
* consider the healthInfo return of this server. We may want to roll that into ECOURSES-692 (server robustness)
* I think I've got a logger bug in here somewhere