Coding Styles and Code Formatting

[lirantal]

Topic

Coding Styles and Code Formatting across the MEAN.JS project code base.

Conventions

• Named functions (no anonymous functions allowed)

Tool

We chose ESLint as the upcoming tool to perform the job, if you would like to propose anything else please comment with the pros/cons (see reference for PR request: #763)

[lirantal]

@ilanbiala @mleanos @codydaig @trainerbill @rhutchison we've got this place to discuss items now related to coding conventions. You're welcome to raise all the formatting and code style that were raised in other PRs here and we'll add them to the main issue description as this grows.

[codydaig]

No nested functions! It should be a closure instead: ie.

Incorrect:

function foo() {
  var user = 'cody';

  someFunctionReturningAPromise()
    .then(bar);

  function bar() {
    // uses the user variable
  }
}

Correct:

function foo() {
  var user = 'cody';

  someFunctionReturningAPromise()
    .then(bar(user));
}

function bar(user) {
  return function () {
    // Yay Closures!!
  }
}

[ilanbiala]

It should actually just be .then(bar) to keep it simplest.

[codydaig]

@ilanbiala so you prefer nested functions?

[trainerbill]

This is bound to ruffle some feathers, but for the client side / angular, can we just put conform to john papa style guide? Or are we creating a whole new style guide for this project? I personally like the john papa style guide, and they already have it documented to the T. No need to reinvent the wheel IMO.

[ilanbiala]

@codydaig not nested functions. I'm not sure how you get that. Look at this example with bluebird (the concept holds for promise as well):

// orders.between
exports.between = function findOrdersBetween(d1, d2) {
  var p = Promise.defer();

  Order
        .find({})
        .where('created').gte(d1).lte(d2)
        .sort('created')
        .populate('items')
        .exec(function(err, orders) {
            if (err) {
                p.reject(err);
            } else {
                p.resolve(orders);
            }
        });

    return p.promise;
};

function createDay(orders) {
    var p = Promise.defer();

    var today;

    today = new Day() // options omitted

    today.save(function(err, today) {
        if (err) {
            p.reject(err);
            console.error(err);
        } else {
            p.resolve(today);
        }
    });

    return p.promise;
}

function emailCSV(csvContents) {
    var today = new Date();
    var options = {
        attachments: [{
            filename: 'HTHS-bcc-' + today.toDateString().replace(/\s/g, '-') + '.csv',
            content: csvContents.oldOrderCSV
        }, {
            filename: 'HTHS-orders-' + today.toDateString().replace(/\s/g, '-') + '.csv',
            content: csvContents.customerCSV
        }, {
            filename: 'new-' + today.toDateString().replace(/\s/g, '-') + '.csv',
            content: csvContents.orderCSV
        }]
    };

    /* sending email code here */
}

orders
  .between(yesterday, today)
  .then(createDay)
  .then(emailCSV)
  .catch(function(err) {
    console.error('Error: ', err);
  });

I've removed some of the code for brevity, but look at the final promise chain. All I'm doing is returning the promise from each function and resolving/rejecting with something, and the following then/catch has access to it.

[ilanbiala]

@trainerbill I think we've already justified why certain things don't apply regarding John Papa's style guide.

[trainerbill]

@ilanbiala Then can we please link to the pieces we are using? A couple that I would put in there to start:

https://github.com/johnpapa/angular-styleguide#iife
https://github.com/johnpapa/angular-styleguide#modules
https://github.com/johnpapa/angular-styleguide#controllers
https://github.com/johnpapa/angular-styleguide#factories
https://github.com/johnpapa/angular-styleguide#route-resolve-promises
https://github.com/johnpapa/angular-styleguide#routing

[mleanos]

@ilanbiala In your example the emailCSV function is expecting the parameter csvContents to be the resolved data from the previous promise in the chain (createDay which resolves the today data).

Closures can be useful for when you need to pass additional parameters to your functions that are being called in a chain of promises.

To keep with your example, you may actually need to pass csvContents into the emailCSV function, and you will also need access to the today data that's being resolved. Using a closure, your function declaration would now look like this..

function emailCSV(csvContents) { // passed in from the chain
  return function (today) { // passed in from the previous resolved promise
    // do all your work here
  }
}

And then your chain would be this..

orders
  .between(yesterday, today)
  .then(createDay)
  .then(emailCSV(csvContents))
  .catch(function(err) {
    console.error('Error: ', err);
  });

[ilanbiala]

@mleanos while slightly less efficient, it's better in my opinion to pass around more data in case you ever need it and end up with clearer code. Closures in my opinion are nowhere near as clear as having an object result = {csvContents: "", today: ""} with a little extra info and ending up with a very simple promise chain that is clearer because it is consistent in its execution setup.

[mleanos]

@ilanbiala The problem with that strategy is that you end up having to pass around this extra info into each of your functions that are returning promises. They become too dependent upon data that it shouldn't even be concerned with. The only data that the promisified functions should be concerned with, is the data that it is calculating & resolving. This way they can act autonomously. Any extra data that needs to be passed around is almost always coming from the scope that the promise chain exists in, and should remain there.

[mleanos]

@trainerbill I'm fine with your proposed list of styles from John Papa. However, I'm still not convinced of the IIFE's; but I won't fight it. It would be nice to know the exact benefits of using them; with examples.

I'm not sure if this was implied by your list but I would add this as well...
https://github.com/johnpapa/angular-styleguide#bindable-members-up-top