I like the general approach, as I think it is more than flexibleenough for our needs (and less bulky than a full ORM as well). I do
think the SQLGenerator function could be broken up into separate
functions. Having all of the tables in a single SQLGenerator feels
like it mixes purposes slightly, particular when the prototype ends up
having specific functions that only apply to a single table/data
structure.
Maybe something like:
function VMContextGenerator(dialect) {
this.generator = new sql.Sql(dialect);
this.context = this.generator.define({
...
});
}
I'm also curious why you went with the approach of having all of the
functions defined on the prototype, instead of in the generator
function (this may be just a general style question, as this technique
is generally used throughout the library).
Having a function defined on the prototype makes it so all instances share that function. Defining a function within a constructor creates a new copy of the function for each instance. Since this application will be long running with many copies of these objects, it's better to define functions on the prototype rather than inside constructors.
I do agree that the SQLGenerator object does too much currently. It should be broken up into generators for each table. We could then rename functions such as getMailbox to get within a MailboxSqlGenerator object. Note that this would involve duplicating some of the table definitions to make joins work with the node-sql library responsible for generating the final SQL text but we could look at defining the tables in a separate module to remove this problem.
On that same note, have you thought about using constructor
functions/closures to build up objects with private data? I noticed
that a lot of the data models use a ._ notation, which, unlike with
Python, doesn't guarantee much (that I'm aware of).
Using _ as a prefix before a member is a convention only. It's typical of many libraries. Having said that, some of the members I currently have defined this way ended up being needed outside of the objects themselves and should be redefined to show this. For members that are truly internal to an object only, we could easily hide them using a closure, but then they would only be accessible within a constructor. Since we are using prototype functions, this is less than ideal. We could however add read only functions in the constructor for these members and at least prohibit writing to them (we could also define them as properties with a read only accessor).
EventEmitters: works for me. I think state transitions will occurbeyond just DTMF key presses: for example, you could have someone
press '#' to terminate recording a message. They can also just hang
up.
On that same note, some events that cause transitions in your finite
state machine should have 'precedence'. For example, if I mash the
keypad while traversing through VoiceMailMain menus and then hangup,
you probably don't want to try and process all of those key presses.
Having things be not just DTMF specific also lets you tie in other
mechanisms to 'traverse' the VoiceMail applications, such as messages
from some external source.
The idea is to define a state machine with known states and pre-defined "events" that will cause a transition between those states. An event in this case could be a DTMF event, another WebSocket event, or any external application/library. The state machine exposes a handle function for handling an event, it does not pre define where those events come from.
The only problem I foresee with this approach is the explosion of'ancillary' applications that can occur:
* Authentication
* Recording greetings
* Changing which greeting is the default greeting
* Putting your voicemail mailbox into 'away' mode
Some of these are silly, but some aren't. Having a single WS
connection for all of that has some drawbacks:
* Complications dealing with state transitions (making sure we don't
transition incorrectly due to thinking we are in the wrong
application)
* Potential limitations with scaling. For example, I may want one
node.js instance to handle all VoiceMailMain apps, but Authentication
to be handled by something other node.js instance (or even another
server).
Currently the application spins up a Voicemail and VoicemailMain handler. I like the idea of that being the default option, with CLI arguments for spinning up a given one if an end user of the application would like to split the handlers to 2 processes/servers/. Events will be tied to a given channel that is either interacting with a Voicemail or VoicemailMain handler, so thinking we are in the wrong application will not be a problem. As for ancillary applications, my thought was to have those be Node.js modules packaged with the application.
A few other questions:
- There's a fair amount of inheritance in use in the data models. As
much of an OO-guy as I happen to be, I'm not sure it's warranted for
the config objects (or that it's really even needed with a weakly
(non?) typed language like JS). Why not go with a more duck-typed
approach?
I agree with you here. Most of the data abstraction layer is overly abstracted using base/derived objects and could be simplified to make the application more easily digestible. I may have went overboard here and there to try and reduce duplication. :)
- The VoiceMailMain function is a little ... long. I'd break up that
function into separate functions as soon as possible, and give some
thought to how you want to be managed for the long run. (Yes,
prototype - but in the 'real' one, this is where things will start to
break down). Generally, I have a feeling that app_voicemail's
vm_execmain wasn't originally the Stygian horror it has become either,
but as Mark wrote:
/* XXX This is, admittedly, some pretty horrendous code. For some
reason it just seemed a lot easier to do with GOTO's. I feel
like I'm back in my GWBASIC days. XXX */
This is probably the easiest place to fall back into that trap. What
thoughts do you have on managing the complexity growth there?
Both the Voicemail and VoicemailMain menus were written quickly to be able to test the message handling capabilities. They are not meant to reflect where that portion of the application will go in the future.
I think the main thing we can do to reduce complexity is to work hard to make use of small, reusable Node.js modules. These modules can be reused by Voicemail and VoicemailMain as well as potentially taken out and used by other applications. Not that a Node.js module does not necessarily mean an NPM published module is this case.
Samuel Fortier-Galarneau Digium, Inc. | Software Developer 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: www.digium.com & www.asterisk.org
_______________________________________________ asterisk-app-dev mailing list asterisk-app-dev@xxxxxxxxxxxxxxxx http://lists.digium.com/cgi-bin/mailman/listinfo/asterisk-app-dev
