On Tue, Jul 22, 2014 at 1:28 PM, Samuel Galarneau <sgalarneau@xxxxxxxxxx> wrote:
> Greetings,
>
> We are eliciting feedback on a prototype voicemail application implemented
> with
> ARI and Node.js.
>
> Specifically, we are interested to hear what you think regarding the
> following:
I freely admit that my JS-foo is weak, so take the following with a
large grain of salt.
In the interest of full disclosure: Sam and I discussed a number of
things while he worked on the prototype, but do to said weak JS-foo, I
haven't had a good chance to sit down and digest the progress until he
put it up on github.
> - a pluggable data abstraction layer using provider aware SQL generators
> instead of a full fledged ORM.
I like the general approach, as I think it is more than flexible
enough 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).
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).
> - a finite state machine driven menu that transforms dtmf events into
> commands
> emitted by an EventEmitter that can be used to define command handlers.
> (please
> note that this currently only transforms dtmf events into commands via an
> EventEmitter. The finite state machine has not been implemented yet)
EventEmitters: works for me. I think state transitions will occur
beyond 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.
> - a single application listening to two Stasis applications to support the
> functionality of both the Voicemail and Voicemail Main dialplan
> applications.
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).
> Please consult the project's documentation for installation and usage
> instructions (https://github.com/asterisk/node-voicemail-js).
>
> Please feel free to provide feedback. Please note that this application is
> in
> prototyping phase. We do not expect it to be production ready and will most
> likely be rewriting large portions of the existing code base in the future.
>
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?
- 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?
--
Matthew Jordan
Digium, Inc. | Engineering Manager
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: http://digium.com & http://asterisk.org
_______________________________________________
asterisk-app-dev mailing list
asterisk-app-dev@xxxxxxxxxxxxxxxx
http://lists.digium.com/cgi-bin/mailman/listinfo/asterisk-app-dev
