<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">I like the general approach, as I think it is more than flexible<br>
</div>
enough for our needs (and less bulky than a full ORM as well). I do<br>
think the SQLGenerator function could be broken up into separate<br>
functions. Having all of the tables in a single SQLGenerator feels<br>
like it mixes purposes slightly, particular when the prototype ends up<br>
having specific functions that only apply to a single table/data<br>
structure.<br>
<br>
Maybe something like:<br>
<br>
function VMContextGenerator(dialect) {<br>
    this.generator = new sql.Sql(dialect);<br>
    this.context = this.generator.define({<br>
        ...<br>
    });<br>
}<br>
<br>
I'm also curious why you went with the approach of having all of the<br>
functions defined on the prototype, instead of in the generator<br>
function (this may be just a general style question, as this technique<br>
is generally used throughout the library).<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On that same note, have you thought about using constructor<br>
functions/closures to build up objects with private data? I noticed<br>
that a lot of the data models use a ._ notation, which, unlike with<br>
Python, doesn't guarantee much (that I'm aware of).<br></blockquote><div><br></div><div>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).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">EventEmitters: works for me. I think state transitions will occur<br></div>
beyond just DTMF key presses: for example, you could have someone<br>
press '#' to terminate recording a message. They can also just hang<br>
up.<br>
<br>
On that same note, some events that cause transitions in your finite<br>
state machine should have 'precedence'. For example, if I mash the<br>
keypad while traversing through VoiceMailMain menus and then hangup,<br>
you probably don't want to try and process all of those key presses.<br>
<br>
Having things be not just DTMF specific also lets you tie in other<br>
mechanisms to 'traverse' the VoiceMail applications, such as messages<br>
from some external source.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">The only problem I foresee with this approach is the explosion of<br></div>
'ancillary' applications that can occur:<br>
* Authentication<br>
* Recording greetings<br>
* Changing which greeting is the default greeting<br>
* Putting your voicemail mailbox into 'away' mode<br>
<br>
Some of these are silly, but some aren't. Having a single WS<br>
connection for all of that has some drawbacks:<br>
* Complications dealing with state transitions (making sure we don't<br>
transition incorrectly due to thinking we are in the wrong<br>
application)<br>
* Potential limitations with scaling. For example, I may want one<br>
node.js instance to handle all VoiceMailMain apps, but Authentication<br>
to be handled by something other node.js instance (or even another<br>
server).<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">A few other questions:<br></div>
<br>
- There's a fair amount of inheritance in use in the data models. As<br>
much of an OO-guy as I happen to be, I'm not sure it's warranted for<br>
the config objects (or that it's really even needed with a weakly<br>
(non?) typed language like JS). Why not go with a more duck-typed<br>
approach?<br></blockquote><div><br></div><div>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. :)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- The VoiceMailMain function is a little ... long. I'd break up that<br>
function into separate functions as soon as possible, and give some<br>
thought to how you want to be managed for the long run. (Yes,<br>
prototype - but in the 'real' one, this is where things will start to<br>
break down). Generally, I have a feeling that app_voicemail's<br>
vm_execmain wasn't originally the Stygian horror it has become either,<br>
but as Mark wrote:<br>
    /* XXX This is, admittedly, some pretty horrendous code.  For some<br>
       reason it just seemed a lot easier to do with GOTO's.  I feel<br>
       like I'm back in my GWBASIC days. XXX */<br>
This is probably the easiest place to fall back into that trap. What<br>
thoughts do you have on managing the complexity growth there?<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br></blockquote></div>-- <br><div dir="ltr"><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><pre style="white-space:pre-wrap">
Samuel Fortier-Galarneau
Digium, Inc. | Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at:  <a href="http://www.digium.com" target="_blank">www.digium.com</a>  & <a href="http://www.asterisk.org" target="_blank">www.asterisk.org</a></pre></pre></div>
</div></div>