Refactored global functions into modules

[boborbt]

Hi Casey,
I'm a long time listener of the show and I got curious about your newly open sourced camel project.

I'm not a node expert (I started studying node only recently), but I thought that your code albeit not bad could use some refactoring. Here is what I've produced. As you will see I've moved all functions into separate modules and moved all global vars into several fields of the app object.

This is not perfect and some more work could be spent to improve several things, but I thought that you may prefer camel the old way and so I did not bother to do the work not knowing if you were interested.

Several advantages I find in the new version:

• more modular (of course ;-))
• smaller code units, improves readability (and you do not need the section dividers that John hates)
• easier to test (no test done right now)

I tested the engine with the sample post included in the project and it works. I highly suspect that something would break on a larger site, but I don't have one to test it. If you find any problem feel free to email me the whole thing (or just the files that make the thing to break) and I will fix it (most likely it only needs to refer the 'scoped' variables/functions instead of the global ones.

In no way this message is meant to make any offence or blame you or your code, if you find anything offensive please take into consideration that I'm not a native english speaker and so I may have misspoken.

Say hello to Marco and John, you guys are the best.

Roberto Esposito

[cliss]

Hi, Roberto. Thanks for the pull request!

First, let me point out that I wouldn't have known you're not a native speaker had you not said so. :)

Secondly, thanks for taking the time to do all this. It's extremely flattering and I greatly appreciate it.

Regarding the commit, I have mixed feelings. I like some of the ideas, but not all of them.

What I like

• Splitting things out into different files
• Splitting those files functionally
• Only require()ing those modules that each file needs
• I really like the idea of exporting modules, which is something I don't know much about, and now know I should read up on. :)

What I don't like

• I don't like having several subdirectories. If anything, I'd expect to see (more on this below):

  camel/
  +-- camel.js
  |      +-- routes.js
  |      |   Contains everything route-related
  |      `-- helpers.js
  |          Contains everything that isn't route-related
  +-- posts/
  `-- ...
  

• The app crashes when loading a year page, such as this one. Seems like it's just a missing require('fs').
  

• There are some stylistic things I'm not in love with, but those are minor. Most especially, chaining the require() calls as one statement.

Pragmatism

I may turn this into a blog post, but one thing I'm wrestling with is simplicity. Specifically, since camel (as checked in) is only ~650 lines, is it really worth splitting things up into segregated files? If so, how many? Or is it worth it to just keep things simple and easier to understand, by including them in one file (even if that requires ugly flower boxes)? :)

I think, as with all things, the answer is in the middle, as I described above. I think the right answer is probably three files:

• camel.js bootstrapper/entry
• routes.js for routes
• helpers.js for any other methods

Sitting here now, that feels like the right amount of separation of concerns while still keeping things easy to understand.

[boborbt]

Hi Casey,
thanks for the prompt and kind answer. I reworked the directory structure to
satisfy your comments and I fixed the remaining bugs. I've tried the page you
pointed to (and several others whose urls I guessed from the routes in the app),
I can't be sure, but I do believe that the app should work now.

I may turn this into a blog post, but one thing I'm wrestling with is 
simplicity. Specifically, since camel (as checked in) is only ~650 lines, 
is it really worth splitting things up into segregated files? If so, 
how many? Or is it worth it to just keep things simple and easier to 
understand, by including them in one file (even if that requires ugly 
flower boxes)? :)

My take regarding this point is that splitting a 650 line files into meaningful
smaller units is striving for simplicity, and your 'desire' for 'flower boxes'
is a good hint that the file is too large and complex. To what I got from the
atp podcast you are used to microsoft IDEs. I think that part of the problem
is there. MS IDEs are fantastic, but by hiding complexity under the GUI they
allow for more complex units to be effectively managed. For languaged served by lesser
IDEs, splitting the code base into smaller unit is much more important
(my personal opinion is that this is the best way of doing it, and that
similar practices should be followed even when good IDEs are available, but
this is probably a much more debatable matter).

Clearly I'm not advocating for splitting files for the sake of the split, nor
that the more the number of the files the better. As you were implicitly saying
concluding your response, it is a matter of striking a good balance and I agree
with you that for the time being the structure you suggested is a good compromise.

Further refactorings

As I mentioned in my first post, I think the new structure is a good start, but
that the project could still be improved. The main problems I see are the
following:

• some of the functions are too long and complex. I would try to simplify them
  by creating some more helpers;
• the above point is particularly problematic for route handlers. Coming from
  Ruby on Rails I cannot avoid thinking at them as controller methods. The
  fact that they are so long and complex hints that these functions are probably
  mixing some model and controller logic;
• the whole code should be refactored into smaller classes each one with a
  specific concern. I did not have time to think it through, but a thing that
  comes to mind is that all the caching logic should belong to its own class.
• tests

Let me know if you feel the same. If you agree with me, I will try to help.

Idioms

Coming from a number of other languages I agree with you about the weirdness of
"chaining the require() calls as one statement", but my current understanding
(based on an admittedly limited experience) is that it is the node 'idiomatic'
way of doing it.
update: I just found a style guide for node and the correct (™) way of doing it is one var per line. You were right and I'm on my way to updating the source.

New Features

The current way of handling pages not found is quite crude (If I'm correct it is a JSON response saying that the page is not found). The project needs a better 404 handler.

[boborbt]

Hi Casey,
I've taken the time the review a little more the code in camel. I've started (in a separate branch you can't see right now) a basic refactoring separating the caching code into its own file (which also cleaned up a little more the main script) and started looking at the implementation more carefully. One thing it came to mind is that all the caching should be insulated into its own middleware: the middleware would intercept the request and respond to it if it finds the result in the cache, otherwise it would call the rest of the application.

If I'm not much mistaken, this would have several advantages:

• it would clean up things a good bit (all the rest of the application can totally avoid to be concerned with caching),
• it would (probably) clean up the caching code as well,
• it would (probably) improve the performances (the caching code would kick in before the router even gets a chance to look at the request).

How do you feel about it?

If you feel you want to keep the things as they are let me know and I will stop stalking you. All the best.

[boborbt]

I know that you have been through an exciting time and I wish you all the best. I guess that you are not interested in the changes I proposed you so I'm closing this request.